Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2158293003:
Update tests and DTX check for Opus 1.1.3. (Closed)
|
Created:
4 years, 5 months ago by flim-webrtc Modified:
4 years, 4 months ago Reviewers:
minyue-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. |
DescriptionUpdate tests and DTX check for Opus 1.1.3.
DTX is now indicated by packets that may have a size of up to 2 bytes.
Ref: https://git.xiph.org/?p=opus.git;a=commit;h=1c311423c86b89eba27a494e17c79fefd7d75ab0
BUG=
Committed: https://crrev.com/64a7eab89183845e4a295c512bcf1c87a2424f86
Cr-Commit-Position: refs/heads/master@{#13736}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Fix Opus DTX detection logic #Patch Set 3 : Simplify if/else statement #
Total comments: 2
Patch Set 4 : Make if/else statements consistent #Patch Set 5 : Rebasing #Patch Set 6 : Reenable Opus tests #Patch Set 7 : Rebase #Patch Set 8 : Update bitrate test after changing Opus in Chromium #Patch Set 9 : Add expected result on Android arm64 to Opus bitrate test #
Messages
Total messages: 43 (23 generated)
flim@webrtc.org changed reviewers: + minyue@webrtc.org
Here are the changes to fix tests that will fail when Opus 1.1.3 lands (https://codereview.chromium.org/2165633002/)
https://codereview.webrtc.org/2158293003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/2158293003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:98: if (res <= 2) { Should it be the following if (res == 0) return -1 if (res <= 2) { do DTX stuff } else { inst->in_dtx_mode = 0; return res; }
https://codereview.webrtc.org/2158293003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/2158293003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:98: if (res <= 2) { On 2016/07/26 10:24:42, minyue-webrtc wrote: > Should it be the following > > if (res == 0) > return -1 > > if (res <= 2) { > do DTX stuff > } else { > inst->in_dtx_mode = 0; > return res; > } > Good catch. I've used res <= 0 since Opus may return negative numbers corresponding to different error codes.
https://codereview.webrtc.org/2158293003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/2158293003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:98: if (res <= 2) { On 2016/07/26 11:40:41, flim-webrtc wrote: > On 2016/07/26 10:24:42, minyue-webrtc wrote: > > Should it be the following > > > > if (res == 0) > > return -1 > > > > if (res <= 2) { > > do DTX stuff > > } else { > > inst->in_dtx_mode = 0; > > return res; > > } > > > > Good catch. I've used res <= 0 since Opus may return negative numbers > corresponding to different error codes. Good catch :P https://codereview.webrtc.org/2158293003/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/2158293003/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:98: if (res <= 0) { better to do res in similar manner Alternative 1: if (res <= 0) { } else if (res <=2) { } else { } or alternative 2 if (res <= 0) { return xx } if (res <=2) { return xx } return xx I refer 2
https://codereview.webrtc.org/2158293003/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/2158293003/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:98: if (res <= 0) { On 2016/07/26 11:52:06, minyue-webrtc wrote: > better to do res in similar manner > > Alternative 1: > if (res <= 0) { > } else if (res <=2) { > } else { > } > > or alternative 2 > > if (res <= 0) { > return xx > } > if (res <=2) { > return xx > } > return xx > > I refer 2 Done.
lgtm
On 2016/07/26 12:24:09, minyue-webrtc wrote: > lgtm Thanks for the review! This will be landed when Opus 1.1.3 is updated in Chromium (https://codereview.chromium.org/2165633002/#)
One more small change to re-enable tests after disabling them (https://codereview.webrtc.org/2185673002/) to avoid breaking bots when Chromium with the new Opus is rolled into WebRTC. Thanks!
On 2016/07/26 16:35:13, flim-webrtc wrote: > One more small change to re-enable tests after disabling them > (https://codereview.webrtc.org/2185673002/) to avoid breaking bots when Chromium > with the new Opus is rolled into WebRTC. Thanks! you may need to rebase first, right?
On 2016/07/28 08:19:21, minyue-webrtc wrote: > On 2016/07/26 16:35:13, flim-webrtc wrote: > > One more small change to re-enable tests after disabling them > > (https://codereview.webrtc.org/2185673002/) to avoid breaking bots when > Chromium > > with the new Opus is rolled into WebRTC. Thanks! > > you may need to rebase first, right? Sorry, I forgot to upload a patch after rebasing. I'll remember the next time but not sure now to fix this here? If you look at the actual file diff, not delta, "DISABLED_" has been removed from the affected test names.
On 2016/07/28 08:31:10, flim-webrtc wrote: > On 2016/07/28 08:19:21, minyue-webrtc wrote: > > On 2016/07/26 16:35:13, flim-webrtc wrote: > > > One more small change to re-enable tests after disabling them > > > (https://codereview.webrtc.org/2185673002/) to avoid breaking bots when > > Chromium > > > with the new Opus is rolled into WebRTC. Thanks! > > > > you may need to rebase first, right? > > Sorry, I forgot to upload a patch after rebasing. I'll remember the next time > but not sure now to fix this here? If you look at the actual file diff, not > delta, "DISABLED_" has been removed from the affected test names. can 1) delete patch set 5, 2) rebase (and commit as patch set 5), 3) redo patch set 5 and commit as patch set 6. this only helps reading.
Patchset #5 (id:80001) has been deleted
On 2016/07/28 10:59:42, minyue-webrtc wrote: > On 2016/07/28 08:31:10, flim-webrtc wrote: > > On 2016/07/28 08:19:21, minyue-webrtc wrote: > > > On 2016/07/26 16:35:13, flim-webrtc wrote: > > > > One more small change to re-enable tests after disabling them > > > > (https://codereview.webrtc.org/2185673002/) to avoid breaking bots when > > > Chromium > > > > with the new Opus is rolled into WebRTC. Thanks! > > > > > > you may need to rebase first, right? > > > > Sorry, I forgot to upload a patch after rebasing. I'll remember the next time > > but not sure now to fix this here? If you look at the actual file diff, not > > delta, "DISABLED_" has been removed from the affected test names. > > can 1) delete patch set 5, 2) rebase (and commit as patch set 5), 3) redo patch > set 5 and commit as patch set 6. > > this only helps reading. ok, fixed now
LGTM and thanks!
The CQ bit was checked by flim@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by flim@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
The CQ bit was checked by flim@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
Patchset #7 (id:140001) has been deleted
The CQ bit was checked by flim@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/1714) ios64_gn_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/1736) ios_api_framework on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/11750) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/16992) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/15642)
The CQ bit was checked by flim@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
A local patch to fix an Opus bug has additionally been applied in Chromium and it's been rolled into WebRTC so we can try to land this now. The tests picked up a small change in bitrates when decoding arm64 on Android (there were some new arm64 routines [1]) so I have extended the test. Sorry for yet another change, could you PTAL? [1] https://codereview.chromium.org/2165633002/diff/20001/third_party/opus/BUILD.gn
lgtm
The CQ bit was checked by flim@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Update tests and DTX check for Opus 1.1.3. DTX is now indicated by packets that may have a size of up to 2 bytes. Ref: https://git.xiph.org/?p=opus.git;a=commit;h=1c311423c86b89eba27a494e17c79fefd... BUG= ========== to ========== Update tests and DTX check for Opus 1.1.3. DTX is now indicated by packets that may have a size of up to 2 bytes. Ref: https://git.xiph.org/?p=opus.git;a=commit;h=1c311423c86b89eba27a494e17c79fefd... BUG= Committed: https://crrev.com/64a7eab89183845e4a295c512bcf1c87a2424f86 Cr-Commit-Position: refs/heads/master@{#13736} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/64a7eab89183845e4a295c512bcf1c87a2424f86 Cr-Commit-Position: refs/heads/master@{#13736} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
