Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2136573002:
Adding audio to video_quality_test. (Closed)
|
Created:
4 years, 5 months ago by minyue-webrtc Modified:
4 years, 4 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, pbos-webrtc, perkj_webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding audio to video_quality_test.
This CL adds an audio loopback to video_quality_test (only RunWithVideoRenderer)
BUG=
Committed: https://crrev.com/65a6578e339f52eb5bc400c5715e60498e4af2c1
Cr-Commit-Position: refs/heads/master@{#13784}
Patch Set 1 #Patch Set 2 : using fake audio device #
Total comments: 10
Patch Set 3 : adding flags. #
Total comments: 5
Patch Set 4 : rebasing #Patch Set 5 : Hook up Audio BWE and small changes. #Patch Set 6 : renaming #
Total comments: 4
Patch Set 7 : fixing nits #Patch Set 8 : rebasing #Patch Set 9 : fixing after rebasing and a nit bug #Patch Set 10 : rebasing again due to a revert in ToT #
Messages
Total messages: 45 (22 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== adding audio to video_quality_test BUG= ========== to ========== Adding audio to video_quality_test. This CL adds an audio loopback to video_quality_test (only RunWithVideoRenderer) BUG= ==========
minyue@webrtc.org changed reviewers: + stefan@webrtc.org, terelius@webrtc.org
Hi Stefan and Bjorn, Please help review this CL. Patch set 1 uses real audio device to be able to monitor the audio. Patch set 2 switches to fake audio device. Patch set 2 may not be necessary.
https://codereview.webrtc.org/2136573002/diff/50001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2136573002/diff/50001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1091: void VideoQualityTest::RunWithVideoRenderer(const Params& params) { The name of this method is no longer good. We should probably rename it. https://codereview.webrtc.org/2136573002/diff/50001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1116: CreateVoiceEngine(); I think we should add an option to configure with or without audio in loopback.cc which uses this class. https://codereview.webrtc.org/2136573002/diff/50001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1135: // video_receive_configs_[stream_id].sync_group = kSyncGroup; Would be good to be able to test sync. Maybe have an option to enable sync and use the mic?
Hi, Thanks for the comments! Please take a look at the new patch. https://codereview.webrtc.org/2136573002/diff/50001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2136573002/diff/50001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1091: void VideoQualityTest::RunWithVideoRenderer(const Params& params) { On 2016/07/11 15:01:10, stefan-webrtc (holmer) wrote: > The name of this method is no longer good. We should probably rename it. any suggestions? https://codereview.webrtc.org/2136573002/diff/50001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1116: CreateVoiceEngine(); On 2016/07/11 15:01:10, stefan-webrtc (holmer) wrote: > I think we should add an option to configure with or without audio in > loopback.cc which uses this class. Done. https://codereview.webrtc.org/2136573002/diff/50001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1135: // video_receive_configs_[stream_id].sync_group = kSyncGroup; On 2016/07/11 15:01:10, stefan-webrtc (holmer) wrote: > Would be good to be able to test sync. Maybe have an option to enable sync and > use the mic? Done. So we can use the actually device instead of fake_audio_device_ https://codereview.webrtc.org/2136573002/diff/50001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.h (right): https://codereview.webrtc.org/2136573002/diff/50001/webrtc/video/video_qualit... webrtc/video/video_quality_test.h:119: struct VoiceEngineState { I think this, and CreateVoiceEngine/DestroyVoiceEngine belongs to RunWithVideoRenderer only, therefore I moved them as static in .cc https://codereview.webrtc.org/2136573002/diff/70001/webrtc/video/video_loopba... File webrtc/video/video_loopback.cc (right): https://codereview.webrtc.org/2136573002/diff/70001/webrtc/video/video_loopba... webrtc/video/video_loopback.cc:194: DEFINE_bool(audio, false, "Add audio stream (no effect if duration defined)"); I do not know what is best to do with RunWithAnalyzer, therefore it does not pick this flag at the moment.
https://codereview.webrtc.org/2136573002/diff/50001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2136573002/diff/50001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1091: void VideoQualityTest::RunWithVideoRenderer(const Params& params) { On 2016/07/12 10:09:36, minyue-webrtc wrote: > On 2016/07/11 15:01:10, stefan-webrtc (holmer) wrote: > > The name of this method is no longer good. We should probably rename it. > > any suggestions? RunWithRenderer https://codereview.webrtc.org/2136573002/diff/70001/webrtc/video/video_loopba... File webrtc/video/video_loopback.cc (right): https://codereview.webrtc.org/2136573002/diff/70001/webrtc/video/video_loopba... webrtc/video/video_loopback.cc:194: DEFINE_bool(audio, false, "Add audio stream (no effect if duration defined)"); On 2016/07/12 10:09:36, minyue-webrtc wrote: > I do not know what is best to do with RunWithAnalyzer, therefore it does not > pick this flag at the moment. I think we should make sure it fails if run with an analyzer. https://codereview.webrtc.org/2136573002/diff/70001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2136573002/diff/70001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1217: audio_send_stream_ = call->CreateAudioSendStream(audio_send_config_); Is audio configured with BWE?
Patchset #5 (id:110001) has been deleted
Hi Stefan, PTAL, thanks! https://codereview.webrtc.org/2136573002/diff/50001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2136573002/diff/50001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1091: void VideoQualityTest::RunWithVideoRenderer(const Params& params) { On 2016/08/08 14:11:18, stefan-webrtc (holmer) wrote: > On 2016/07/12 10:09:36, minyue-webrtc wrote: > > On 2016/07/11 15:01:10, stefan-webrtc (holmer) wrote: > > > The name of this method is no longer good. We should probably rename it. > > > > any suggestions? > > RunWithRenderer RunWithRenderer does not imply that it may contain audio. VideoQualityTest is a bit limiting already. But I don't think we should change that for now. https://codereview.webrtc.org/2136573002/diff/70001/webrtc/video/video_loopba... File webrtc/video/video_loopback.cc (right): https://codereview.webrtc.org/2136573002/diff/70001/webrtc/video/video_loopba... webrtc/video/video_loopback.cc:194: DEFINE_bool(audio, false, "Add audio stream (no effect if duration defined)"); On 2016/08/08 14:11:18, stefan-webrtc (holmer) wrote: > On 2016/07/12 10:09:36, minyue-webrtc wrote: > > I do not know what is best to do with RunWithAnalyzer, therefore it does not > > pick this flag at the moment. > > I think we should make sure it fails if run with an analyzer. You are right. Done. https://codereview.webrtc.org/2136573002/diff/70001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2136573002/diff/70001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1217: audio_send_stream_ = call->CreateAudioSendStream(audio_send_config_); On 2016/08/08 14:11:18, stefan-webrtc (holmer) wrote: > Is audio configured with BWE? The initial CL was made before AudioBWE landed. So it did not have it. Now it does. See the latest patch set.
Lg https://codereview.webrtc.org/2136573002/diff/50001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2136573002/diff/50001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:1091: void VideoQualityTest::RunWithVideoRenderer(const Params& params) { On 2016/08/15 15:34:31, minyue-webrtc wrote: > On 2016/08/08 14:11:18, stefan-webrtc (holmer) wrote: > > On 2016/07/12 10:09:36, minyue-webrtc wrote: > > > On 2016/07/11 15:01:10, stefan-webrtc (holmer) wrote: > > > > The name of this method is no longer good. We should probably rename it. > > > > > > any suggestions? > > > > RunWithRenderer > > RunWithRenderer does not imply that it may contain audio. VideoQualityTest is a > bit limiting already. But I don't think we should change that for now. I think RunWithRenderers is better since it doesn't explicitly mentioned video. Btw, will you follow up with a change which renames this file loopback.cc or similar? Also make it possible to run with audio only? The latter will be important, as I mentioned offline.
I renamed the test, for which I found another place where the test is used, and there is a need for change at there.
lgtm % nits https://codereview.webrtc.org/2136573002/diff/150001/webrtc/video/video_quali... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2136573002/diff/150001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:1058: ASSERT_FALSE(params_.audio); RTC_CHECK_FALSE https://codereview.webrtc.org/2136573002/diff/150001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:1203: if (params_.audio && params_.audio_video_sync) { Remove {}
Thanks. Forgot to answer your previous question, but yes, I will follow up audio-only mode. https://codereview.webrtc.org/2136573002/diff/150001/webrtc/video/video_quali... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2136573002/diff/150001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:1058: ASSERT_FALSE(params_.audio); On 2016/08/16 08:54:42, stefan-webrtc (holmer) wrote: > RTC_CHECK_FALSE Done. https://codereview.webrtc.org/2136573002/diff/150001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:1203: if (params_.audio && params_.audio_video_sync) { On 2016/08/16 08:54:42, stefan-webrtc (holmer) wrote: > Remove {} Done.
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2136573002/#ps170001 (title: "fixing nits")
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_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...) android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) android_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_dbg/builds/1...) ios64_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/1842) ios64_gn_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/1862) ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/9579) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/11875) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/11809) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/17119) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/15769) linux_arm on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_arm/builds/230) linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...) linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/14197) linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/4069) linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/5269) mac_gn_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/11506)
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2136573002/#ps190001 (title: "rebasing")
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: ios64_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/1844) linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) mac_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/5271)
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2136573002/#ps210001 (title: "fixing after rebasing and a nit bug")
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: ios64_gn_rel on master.tryserver.webrtc (JOB_FAILED, no build URL)
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2136573002/#ps230001 (title: "rebasing again due to a revert in ToT")
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: linux_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/17244)
The CQ bit was checked by minyue@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, no build URL)
The CQ bit was checked by minyue@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.
Description was changed from ========== Adding audio to video_quality_test. This CL adds an audio loopback to video_quality_test (only RunWithVideoRenderer) BUG= ========== to ========== Adding audio to video_quality_test. This CL adds an audio loopback to video_quality_test (only RunWithVideoRenderer) BUG= ==========
Message was sent while issue was closed.
Committed patchset #10 (id:230001)
Message was sent while issue was closed.
Description was changed from ========== Adding audio to video_quality_test. This CL adds an audio loopback to video_quality_test (only RunWithVideoRenderer) BUG= ========== to ========== Adding audio to video_quality_test. This CL adds an audio loopback to video_quality_test (only RunWithVideoRenderer) BUG= Committed: https://crrev.com/65a6578e339f52eb5bc400c5715e60498e4af2c1 Cr-Commit-Position: refs/heads/master@{#13784} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/65a6578e339f52eb5bc400c5715e60498e4af2c1 Cr-Commit-Position: refs/heads/master@{#13784}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:230001) has been created in https://codereview.webrtc.org/2249163002/ by minyue@webrtc.org. The reason for reverting is: This CL breaks https://build.chromium.org/p/client.webrtc/waterfall?builder=Win64%20Debug%20... Need to align values to struct Params {} in a proper way. Relanding will follow.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
