Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1766233003:
Drop the restriction on same forward and reverse sample rate on the AudioFrame interface of the APM (Closed)
|
Created:
4 years, 9 months ago by aluebs-webrtc Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDrop the restriction on same forward and reverse sample rate on the AudioFrame interface of the APM
R=peah@webrtc.org
Committed: https://crrev.com/5b830fed077043df01e162dd9c58dac9b27714ea
Cr-Commit-Position: refs/heads/master@{#11913}
Patch Set 1 #
Messages
Total messages: 21 (7 generated)
aluebs@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, peah@webrtc.org, turaj@webrtc.org
On 2016/03/07 16:26:54, aluebs-webrtc wrote: How has this been tested?
On 2016/03/08 07:04:44, peah-webrtc wrote: > On 2016/03/07 16:26:54, aluebs-webrtc wrote: > > How has this been tested? All the APM unittest plus voe_cmd_test that was actually generating different sampling rates for forward and backward for a few chunks. Any more ideas?
On 2016/03/08 09:26:55, aluebs-webrtc wrote: > On 2016/03/08 07:04:44, peah-webrtc wrote: > > On 2016/03/07 16:26:54, aluebs-webrtc wrote: > > > > How has this been tested? > > All the APM unittest plus voe_cmd_test that was actually generating different > sampling rates for forward and backward for a few chunks. Any more ideas? I don't think the unittests properly tests this. Regarding voe_cmd_test it sounds like a good test but I'm not familiar with what it exactly tests. I think this should at least also be tested using a mobile call with AECM activated where both ends listen for echo and distorted audio. The AudioFrame interface is only used when running WebRTC standalone outside of Chrome, right? Or would it also make sense to test this with an AppRTC call via Chrome?
On 2016/03/08 09:46:12, peah-webrtc wrote: > On 2016/03/08 09:26:55, aluebs-webrtc wrote: > > On 2016/03/08 07:04:44, peah-webrtc wrote: > > > On 2016/03/07 16:26:54, aluebs-webrtc wrote: > > > > > > How has this been tested? > > > > All the APM unittest plus voe_cmd_test that was actually generating different > > sampling rates for forward and backward for a few chunks. Any more ideas? > > I don't think the unittests properly tests this. Regarding voe_cmd_test it > sounds > like a good test but I'm not familiar with what it exactly tests. > > I think this should at least also be tested using a mobile call with AECM > activated where > both ends listen for echo and distorted audio. > > The AudioFrame interface is only used when running WebRTC standalone outside of > Chrome, right? > Or would it also make sense to test this with an AppRTC call via Chrome? The voe_cmd_test runs a call through the VoiceEngine with the specified parameters, similar to what the AppRTCDemo call would be. But I agree it might be a good idea to test with this app as well. Will do and report back. As you say, in Chrome the AudioFrame interface is not used, so it makes no sense to test this patch there.
On 2016/03/08 10:56:15, aluebs-webrtc wrote: > On 2016/03/08 09:46:12, peah-webrtc wrote: > > On 2016/03/08 09:26:55, aluebs-webrtc wrote: > > > On 2016/03/08 07:04:44, peah-webrtc wrote: > > > > On 2016/03/07 16:26:54, aluebs-webrtc wrote: > > > > > > > > How has this been tested? > > > > > > All the APM unittest plus voe_cmd_test that was actually generating > different > > > sampling rates for forward and backward for a few chunks. Any more ideas? > > > > I don't think the unittests properly tests this. Regarding voe_cmd_test it > > sounds > > like a good test but I'm not familiar with what it exactly tests. > > > > I think this should at least also be tested using a mobile call with AECM > > activated where > > both ends listen for echo and distorted audio. > > > > The AudioFrame interface is only used when running WebRTC standalone outside > of > > Chrome, right? > > Or would it also make sense to test this with an AppRTC call via Chrome? > > The voe_cmd_test runs a call through the VoiceEngine with the specified > parameters, similar to what the AppRTCDemo call would be. But I agree it might > be a good idea to test with this app as well. Will do and report back. > As you say, in Chrome the AudioFrame interface is not used, so it makes no sense > to test this patch there. Now I tested this patch on a AppRTCDemo call (with peah) and it seems to work just fine.
On 2016/03/08 13:21:51, aluebs-webrtc wrote: > On 2016/03/08 10:56:15, aluebs-webrtc wrote: > > On 2016/03/08 09:46:12, peah-webrtc wrote: > > > On 2016/03/08 09:26:55, aluebs-webrtc wrote: > > > > On 2016/03/08 07:04:44, peah-webrtc wrote: > > > > > On 2016/03/07 16:26:54, aluebs-webrtc wrote: > > > > > > > > > > How has this been tested? > > > > > > > > All the APM unittest plus voe_cmd_test that was actually generating > > different > > > > sampling rates for forward and backward for a few chunks. Any more ideas? > > > > > > I don't think the unittests properly tests this. Regarding voe_cmd_test it > > > sounds > > > like a good test but I'm not familiar with what it exactly tests. > > > > > > I think this should at least also be tested using a mobile call with AECM > > > activated where > > > both ends listen for echo and distorted audio. > > > > > > The AudioFrame interface is only used when running WebRTC standalone outside > > of > > > Chrome, right? > > > Or would it also make sense to test this with an AppRTC call via Chrome? > > > > The voe_cmd_test runs a call through the VoiceEngine with the specified > > parameters, similar to what the AppRTCDemo call would be. But I agree it might > > be a good idea to test with this app as well. Will do and report back. > > As you say, in Chrome the AudioFrame interface is not used, so it makes no > sense > > to test this patch there. > > Now I tested this patch on a AppRTCDemo call (with peah) and it seems to work > just fine. Supernice! lgtm
Will commit this now, but please let me know if you have any additional comments.
The CQ bit was checked by aluebs@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1766233003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1766233003/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_tsan2 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/10453)
The CQ bit was checked by aluebs@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1766233003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1766233003/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Drop the restriction on same forward and reverse sample rate on the AudioFrame interface of the APM ========== to ========== Drop the restriction on same forward and reverse sample rate on the AudioFrame interface of the APM R=peah@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/5b830fed077043df01e162dd9... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as 5b830fed077043df01e162dd9c58dac9b27714ea (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Drop the restriction on same forward and reverse sample rate on the AudioFrame interface of the APM R=peah@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/5b830fed077043df01e162dd9... ========== to ========== Drop the restriction on same forward and reverse sample rate on the AudioFrame interface of the APM R=peah@webrtc.org Committed: https://crrev.com/5b830fed077043df01e162dd9c58dac9b27714ea Cr-Commit-Position: refs/heads/master@{#11913} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/5b830fed077043df01e162dd9c58dac9b27714ea Cr-Commit-Position: refs/heads/master@{#11913} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
