Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1901633002:
Adding 120 ms frame length support in NetEq. (Closed)
|
Created:
4 years, 8 months ago by minyue-webrtc Modified:
4 years, 7 months ago Reviewers:
hlundin-webrtc CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding 120 ms frame length support in NetEq.
BUG=webrtc:1015
Committed: https://crrev.com/5bd3397e53b96f73d1dcc120a50baf594aee3eb5
Cr-Commit-Position: refs/heads/master@{#12592}
Patch Set 1 #Patch Set 2 : solving a chromium-style complaint #Patch Set 3 : fixing a legacy error #Patch Set 4 : rebasing #Patch Set 5 : redo patch set 2 (did not merge in patch set 4) #Patch Set 6 : fixing two errors #
Total comments: 8
Messages
Total messages: 38 (17 generated)
Description was changed from ========== neteq 120ms BUG= ========== to ========== Adding 120 ms frame length support in NetEq. BUG=webrtc:1015 ==========
minyue@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
Henrik, Please take a look at this CL. Thanks!
lgtm
The CQ bit was checked by minyue@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1901633002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1901633002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/10190)
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1901633002/#ps20001 (title: "solving a chromium-style complaint")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1901633002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1901633002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_ubsan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/1351)
Patchset #3 (id:40001) has been deleted
On 2016/04/19 07:42:27, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_ubsan on tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/1351) Hi Henrik, This seems to reveal a bug in our code. I will discuss with you before fixing it.
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1901633002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1901633002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/2725)
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1901633002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1901633002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/1620)
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1901633002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1901633002/120001
This turns green eventually. Since beginning, there has been UBsan problems encountered. But the bots could catch one at a time. One of them is fixed in a separate CL. Patch set 6 fixed a remaining one + a Msan error. Henrik, it would be good that you take a look at the patch set 6. https://codereview.webrtc.org/1901633002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/merge.cc (right): https://codereview.webrtc.org/1901633002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/merge.cc:212: int32_t factor = (expanded_max * expanded_max) / first fix: old code does give enough shifts, since log_fs_mult is not right for 48000 Hz https://codereview.webrtc.org/1901633002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/1901633002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_impl.cc:61: delay_peak_detector(new DelayPeakDetector(tick_timer.get())), this is due to rebase https://codereview.webrtc.org/1901633002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right): https://codereview.webrtc.org/1901633002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:85: new MockDelayPeakDetector(tick_timer_)); due to rebase https://codereview.webrtc.org/1901633002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:787: // methods don't use any override declarations, and we want to avoid due to rebase https://codereview.webrtc.org/1901633002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/normal_unittest.cc (right): https://codereview.webrtc.org/1901633002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/normal_unittest.cc:152: for (size_t i = 0; i < kChannels; ++i) { this is the second fix
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.webrtc.org/1901633002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/merge.cc (right): https://codereview.webrtc.org/1901633002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/merge.cc:212: int32_t factor = (expanded_max * expanded_max) / On 2016/05/02 10:44:52, minyue-webrtc wrote: > first fix: > > old code does give enough shifts, since log_fs_mult is not right for 48000 Hz Acknowledged. https://codereview.webrtc.org/1901633002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/1901633002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_impl.cc:61: delay_peak_detector(new DelayPeakDetector(tick_timer.get())), On 2016/05/02 10:44:52, minyue-webrtc wrote: > this is due to rebase Acknowledged.
https://codereview.webrtc.org/1901633002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/merge.cc (right): https://codereview.webrtc.org/1901633002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/merge.cc:212: int32_t factor = (expanded_max * expanded_max) / On 2016/05/02 11:28:15, hlundin-webrtc wrote: > On 2016/05/02 10:44:52, minyue-webrtc wrote: > > first fix: > > > > old code does give enough shifts, since log_fs_mult is not right for 48000 Hz > > Acknowledged. ok. but I ought to modify my sentence as "old code does NOT give enough shifts". I think you have realized it.
The CQ bit was checked by minyue@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1901633002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1901633002/120001
Message was sent while issue was closed.
Description was changed from ========== Adding 120 ms frame length support in NetEq. BUG=webrtc:1015 ========== to ========== Adding 120 ms frame length support in NetEq. BUG=webrtc:1015 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Adding 120 ms frame length support in NetEq. BUG=webrtc:1015 ========== to ========== Adding 120 ms frame length support in NetEq. BUG=webrtc:1015 Committed: https://crrev.com/5bd3397e53b96f73d1dcc120a50baf594aee3eb5 Cr-Commit-Position: refs/heads/master@{#12592} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5bd3397e53b96f73d1dcc120a50baf594aee3eb5 Cr-Commit-Position: refs/heads/master@{#12592}
Message was sent while issue was closed.
On 2016/05/02 11:46:24, commit-bot: I haz the power wrote: > Patchset 6 (id:??) landed as > https://crrev.com/5bd3397e53b96f73d1dcc120a50baf594aee3eb5 > Cr-Commit-Position: refs/heads/master@{#12592} It would be better if unit test kNetEqMaxFrameSize = kMaxFrameSize instead of hardcoding it. static const size_t kMaxFrameSize = 5760; // 120 ms @ 48 kHz. TEST_F(NetEqImplTest, UnsupportedDecoder) { static const size_t kNetEqMaxFrameSize = 5760; // 120 ms @ 48 kHz.
Message was sent while issue was closed.
On 2016/05/02 17:54:38, Dani Kirov wrote: > On 2016/05/02 11:46:24, commit-bot: I haz the power wrote: > > Patchset 6 (id:??) landed as > > https://crrev.com/5bd3397e53b96f73d1dcc120a50baf594aee3eb5 > > Cr-Commit-Position: refs/heads/master@{#12592} > > It would be better if unit test kNetEqMaxFrameSize = kMaxFrameSize instead of > hardcoding it. > > static const size_t kMaxFrameSize = 5760; // 120 ms @ 48 kHz. > > TEST_F(NetEqImplTest, UnsupportedDecoder) { > static const size_t kNetEqMaxFrameSize = 5760; // 120 ms @ 48 kHz. Thanks! I agree. We can fix it in a separate patch. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
