Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2985633002:
Avoid that previous settings in APM are overwritten by WebRtcVoiceEngine (Closed)
|
Created:
3 years, 5 months ago by peah-webrtc Modified:
3 years, 5 months ago Reviewers:
Taylor Brandstetter CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAvoid that previous settings in APM are overwritten by WebRtcVoiceEngine
This CL ensures that any previously set nondefault settings in the
audio processing module are not overwritten by the ApplyOptions
method in WebRtcVoiceEngine
BUG=webrtc:8018
Review-Url: https://codereview.webrtc.org/2985633002
Cr-Commit-Position: refs/heads/master@{#19144}
Committed: https://chromium.googlesource.com/external/webrtc/+/b1c9d1de362d1a8ecc66a4de958197c5f40e54b1
Patch Set 1 #Patch Set 2 : Corrected unittest behavior relying on sticky settings #
Total comments: 2
Patch Set 3 : Added solution based on ReturnPointee and SaveArg #
Total comments: 10
Messages
Total messages: 28 (18 generated)
The CQ bit was checked by peah@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/...
peah@webrtc.org changed reviewers: + deadbeef@webrtc.org
Hi, Would you be able to review this CL?
Description was changed from ========== Avoids that previous settings in APM are overwritten by WebRtcVoiceEngine This CL ensures that any previously set nondefault settings in the audio processing module are not overwritten by the ApplyOptions method in WebRtcVoiceEngine BUG=webrtc:8018 ========== to ========== Avoid that previous settings in APM are overwritten by WebRtcVoiceEngine This CL ensures that any previously set nondefault settings in the audio processing module are not overwritten by the ApplyOptions method in WebRtcVoiceEngine BUG=webrtc:8018 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/26520)
lgtm. Would be nice to have a test, but that may be overkill so I trust your judgement.
On 2017/07/20 23:44:56, Taylor Brandstetter wrote: > lgtm. Would be nice to have a test, but that may be overkill so I trust your > judgement. Thanks for the review! It turned out that the tests were indeed an issue. Therefore I needed to upload a new patch which makes sure that the settings in the AudioProcessing mock used in the tests are sticky, which previously was ensured by the config stored in webrtcvoiceengine.cc. Would you be able to take a look on the changes added in the new patch?
The CQ bit was checked by peah@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: This issue passed the CQ dry run.
https://codereview.webrtc.org/2985633002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/mock_audio_processing.h (right): https://codereview.webrtc.org/2985633002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/mock_audio_processing.h:144: } This is now partly a fake and partly a mock; I think it would be better if the test itself configured the mock's behavior, for example using "testing::SaveArg" and "testing::ReturnPointee" to save the previously set value and return it.
Patchset #3 (id:40001) has been deleted
https://codereview.webrtc.org/2985633002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/mock_audio_processing.h (right): https://codereview.webrtc.org/2985633002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/mock_audio_processing.h:144: } On 2017/07/25 00:04:37, Taylor Brandstetter wrote: > This is now partly a fake and partly a mock; I think it would be better if the > test itself configured the mock's behavior, for example using "testing::SaveArg" > and "testing::ReturnPointee" to save the previously set value and return it. Very valid point! And thanks for the suggestions about SaveArg and ReturnPointee, I did not know about those! I have updated the CL with a patch with that change. Please take a look! https://codereview.webrtc.org/2985633002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left): https://codereview.webrtc.org/2985633002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:207: EXPECT_CALL(*apm_, ApplyConfig(testing::_)); This EXPECT_CALL is actually redundant (and thereby misleading), as it is also placed in the constructor. I removed it as the new testing code otherwise would have required this redundant code to be extended (in a redundant manner). https://codereview.webrtc.org/2985633002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:281: EXPECT_CALL(*apm_, ApplyConfig(testing::_)); Same comment as above regarding redundant code. https://codereview.webrtc.org/2985633002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:288: EXPECT_CALL(*apm_, ApplyConfig(testing::_)); Same comment as above regarding redundant code. https://codereview.webrtc.org/2985633002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:299: EXPECT_CALL(*apm_, ApplyConfig(testing::_)); Same comment as above regarding redundant code.
Thanks for the great comment! I've uploaded a new patch. PTAL!
The CQ bit was checked by peah@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: This issue passed the CQ dry run.
lgtm with nits; thanks for taking the time to improve tests! https://codereview.webrtc.org/2985633002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2985633002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:666: return engine_->GetApmConfigForTest().high_pass_filter.enabled; nit: Can this now just use "apm_config_" directly, removing GetApmConfigForTest? Getting rid of "for test" methods makes me happy. https://codereview.webrtc.org/2985633002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2811: .Times(10) nit: I think this should be a "WillRepeatedly"; if WebRtcVoiceEngine is refactored such that it ends up calling GetConfig twice per cycle for some reason, this test shouldn't fail. https://codereview.webrtc.org/2985633002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2928: .Times(3) Same comment here.
Thanks for the quick review and the comments! https://codereview.webrtc.org/2985633002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2985633002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:666: return engine_->GetApmConfigForTest().high_pass_filter.enabled; On 2017/07/25 22:24:00, Taylor Brandstetter wrote: > nit: Can this now just use "apm_config_" directly, removing GetApmConfigForTest? > Getting rid of "for test" methods makes me happy. That sounds great! I'll do it in another CL though. I created an issue for it. https://codereview.webrtc.org/2985633002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2811: .Times(10) On 2017/07/25 22:23:59, Taylor Brandstetter wrote: > nit: I think this should be a "WillRepeatedly"; if WebRtcVoiceEngine is > refactored such that it ends up calling GetConfig twice per cycle for some > reason, this test shouldn't fail. I partly (or mostly) agree. I think that a main part of the mock usage in this test is to hardcode the behavior of webrtcvoiceengine by specifying the number of calls. I like the aspect of this which is like a soft bitexactness test, but I don't at all like that there is apparent logic in the test to the number of methods calls that is specified. As the rest of the code in these tests is like this, I'll keep it as it is. https://codereview.webrtc.org/2985633002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2928: .Times(3) On 2017/07/25 22:23:59, Taylor Brandstetter wrote: > Same comment here. I agree, but as described above I'll keep it as it is for now at least.
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1501022390604800,
"parent_rev": "0748277005f5e891134ca3ab3be837428e85895b", "commit_rev":
"b1c9d1de362d1a8ecc66a4de958197c5f40e54b1"}
Message was sent while issue was closed.
Description was changed from ========== Avoid that previous settings in APM are overwritten by WebRtcVoiceEngine This CL ensures that any previously set nondefault settings in the audio processing module are not overwritten by the ApplyOptions method in WebRtcVoiceEngine BUG=webrtc:8018 ========== to ========== Avoid that previous settings in APM are overwritten by WebRtcVoiceEngine This CL ensures that any previously set nondefault settings in the audio processing module are not overwritten by the ApplyOptions method in WebRtcVoiceEngine BUG=webrtc:8018 Review-Url: https://codereview.webrtc.org/2985633002 Cr-Commit-Position: refs/heads/master@{#19144} Committed: https://chromium.googlesource.com/external/webrtc/+/b1c9d1de362d1a8ecc66a4de9... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/b1c9d1de362d1a8ecc66a4de9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
