Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2253153004:
Updated mixer unittests and fixed a related bug in the new mixer. (Closed)
|
Created:
4 years, 4 months ago by aleloi Modified:
4 years, 4 months ago Reviewers:
ivoc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@remove_comments_add_level_indicator_dep Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUpdated mixer unittests and fixed a related bug in the new mixer.
Changes to the mixer unittests:
Removed the tests related to the former 'OutputMixer', as it's going
to be removed. Removed incorrect comparison tests with the old mixer
because doing identical mixing decisions with the old mixer proved
unviable.
When the new mixer went from kMaximumAmountOfMixedAudioSources in the
last iteration to kMaximumAmountOfMixedAudioSources+1, it could hit an
RTC_NOTREACHED(); Added fix to mixer and test
AudioMixer.RampedOutSourcesShouldNotBeMarkedMixed that covers that
case.
NOTRY=True
Committed: https://crrev.com/30be5d7cf43036fb3c1bc68a07d9c0dc28fec65a
Cr-Commit-Position: refs/heads/master@{#13880}
Patch Set 1 : . #
Total comments: 7
Patch Set 2 : Forgot to remove VoE/AudioMixer include from unittests. #
Total comments: 19
Patch Set 3 : Changes in response to comments. #Patch Set 4 : Rebase from upstream. #Patch Set 5 : Rebase from upstream. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 19 (10 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
aleloi@webrtc.org changed reviewers: + ivoc@webrtc.org
Ivoc@, ptal! https://codereview.webrtc.org/2253153004/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc (left): https://codereview.webrtc.org/2253153004/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc:222: remainingAudioSourcesAllowedToMix -= mixList.size(); The changes above are a simplification of the handling for the number of participants. With the new mixer, it may happen that mixList.size() > kMaximumAmountOfMixedAudioSources, because RampOut participants are also placed in mixList. https://codereview.webrtc.org/2253153004/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/new_audio_conference_mixer_impl.cc:537: } It may happen that audioFrameList.size() > kMax..Sources, because RampOut participants are placed in mixList. https://codereview.webrtc.org/2253153004/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (left): https://codereview.webrtc.org/2253153004/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:59: }; This class was used for testing against the old AudioConferenceMixer. https://codereview.webrtc.org/2253153004/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:180: }; Removed test class that compared mixing decisions made by the old and new mixer. It was incorrect since it didn't add the participants to the mixer. The mixing decisions made by the old mixer turned out to be very hard to copy with a sorting function. (It depended on input order and could mix muted zero-frames before un-muted loud passive frames.) https://codereview.webrtc.org/2253153004/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:220: }; Removed the former OutputMixer (now AudioMixer) tests, since most AudioMixing functionality is added to NewAudioConferenceMixer and AudioMixer is soon going to be removed. https://codereview.webrtc.org/2253153004/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2253153004/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:76: fake_info(), // audio_frame_info Instead of returning the fake frame, now returns a copy of the fake frame. This was a bug in the tests, because the mixer modifies the incoming frame by dividing the samples by 2 every iteration. https://codereview.webrtc.org/2253153004/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:113: } Utility function for testing mixing priority order. Writing functions and classes interleaved doesn't feel right. I couldn't find anything about class and function order in the style guide, though. Do you think it's worth it to put the test class in its own header?
I'd like to merge this last change with the patch set above, but that would remove my comments on the last pset...
Nice changes, see some comments/questions below. https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:43: AudioFrame frame_for_mixing; Is there any chance that the values that one test writes in here can affect the result of another test? https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:86: frame_info.size() == expected_status.size()); I would suggest to split this into two DCHECKS so that it's more clear what's going wrong if we ever hit one of them. https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:92: for (size_t i = 0; i < num_audio_sources; i++) { The style guide says we should use int for "normal" variables. See https://google.github.io/styleguide/cppguide.html#Integer_Types This also happens a few times in the code below. https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:292: // Test that the reported volume is maximal as full when the mixer I think "as full" can be removed here. https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:293: // input comprises frames with maximal values. Where is the reported volume checked? https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:300: ResetFrame(participant2.fake_frame()); Since this pattern is repeating in many tests, you can consider to make a class that sets up a mixer with a configurable number of participants. That class could also have functions for other common things like setting mixability status on all participants, mixing, etc. This is not really essential, but it would reduce code duplication in the tests. https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:302: // Fill participant1 frame data with maximal sound. It looks like participant2 is getting "maximal" sound here. https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:311: for (size_t i = 0; i < 11; i++) { Why does this loop go to 11? https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:332: participants[i].fake_frame()->data_[0] = 100 * i; Please add a comment to explain why only the first sample is being set here and why to these values. https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:335: for (size_t i = 0; i < kAudioSources - 1; i++) { Please use a range-based for loop whenever possible (i.e. for (auto& p: participants)), in some of the loops below it looks possible as well.
New patch set with responses to comments. https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:43: AudioFrame frame_for_mixing; On 2016/08/18 14:35:27, ivoc wrote: > Is there any chance that the values that one test writes in here can affect the > result of another test? It's only used as an argument to mixer::Mix and that just writes to the frame. The alternative is to have a local mixing frame in each test. https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:86: frame_info.size() == expected_status.size()); On 2016/08/18 14:35:27, ivoc wrote: > I would suggest to split this into two DCHECKS so that it's more clear what's > going wrong if we ever hit one of them. Done. https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:92: for (size_t i = 0; i < num_audio_sources; i++) { On 2016/08/18 14:35:27, ivoc wrote: > The style guide says we should use int for "normal" variables. See > https://google.github.io/styleguide/cppguide.html#Integer_Types > This also happens a few times in the code below. I have a deja-vu feeling about this. I think I have had this pointed out before. And even written it down somewhere to avoid forgetting :) Now fixed. https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:292: // Test that the reported volume is maximal as full when the mixer On 2016/08/18 14:35:26, ivoc wrote: > I think "as full" can be removed here. Done. https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:293: // input comprises frames with maximal values. On 2016/08/18 14:35:27, ivoc wrote: > Where is the reported volume checked? My bad again. VolumeTwoParticipants wasn't intended as a real test, it was local test code to answer a question of minyue@'s. I didn't notice it when I went over the diff... https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:300: ResetFrame(participant2.fake_frame()); On 2016/08/18 14:35:27, ivoc wrote: > Since this pattern is repeating in many tests, you can consider to make a class > that sets up a mixer with a configurable number of participants. That class > could also have functions for other common things like setting mixability status > on all participants, mixing, etc. This is not really essential, but it would > reduce code duplication in the tests. Thanks for the suggestion, I'll consider it! https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:311: for (size_t i = 0; i < 11; i++) { On 2016/08/18 14:35:27, ivoc wrote: > Why does this loop go to 11? Added comment. https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:332: participants[i].fake_frame()->data_[0] = 100 * i; On 2016/08/18 14:35:27, ivoc wrote: > Please add a comment to explain why only the first sample is being set here and > why to these values. Done.
LGTM. https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2253153004/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:43: AudioFrame frame_for_mixing; On 2016/08/19 11:36:21, aleloi wrote: > On 2016/08/18 14:35:27, ivoc wrote: > > Is there any chance that the values that one test writes in here can affect > the > > result of another test? > > It's only used as an argument to mixer::Mix and that just writes to the frame. > The alternative is to have a local mixing frame in each test. Acknowledged.
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org Link to the patchset: https://codereview.webrtc.org/2253153004/#ps120001 (title: "Rebase from upstream.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Updated mixer unittests and fixed a related bug in the new mixer. Changes to the mixer unittests: Removed the tests related to the former 'OutputMixer', as it's going to be removed. Removed incorrect comparison tests with the old mixer because doing identical mixing decisions with the old mixer proved unviable. When the new mixer went from kMaximumAmountOfMixedAudioSources in the last iteration to kMaximumAmountOfMixedAudioSources+1, it could hit an RTC_NOTREACHED(); Added fix to mixer and test AudioMixer.RampedOutSourcesShouldNotBeMarkedMixed that covers that case. ========== to ========== Updated mixer unittests and fixed a related bug in the new mixer. Changes to the mixer unittests: Removed the tests related to the former 'OutputMixer', as it's going to be removed. Removed incorrect comparison tests with the old mixer because doing identical mixing decisions with the old mixer proved unviable. When the new mixer went from kMaximumAmountOfMixedAudioSources in the last iteration to kMaximumAmountOfMixedAudioSources+1, it could hit an RTC_NOTREACHED(); Added fix to mixer and test AudioMixer.RampedOutSourcesShouldNotBeMarkedMixed that covers that case. NOTRY=True ==========
The CQ bit was unchecked by aleloi@webrtc.org
The CQ bit was checked by aleloi@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 ========== Updated mixer unittests and fixed a related bug in the new mixer. Changes to the mixer unittests: Removed the tests related to the former 'OutputMixer', as it's going to be removed. Removed incorrect comparison tests with the old mixer because doing identical mixing decisions with the old mixer proved unviable. When the new mixer went from kMaximumAmountOfMixedAudioSources in the last iteration to kMaximumAmountOfMixedAudioSources+1, it could hit an RTC_NOTREACHED(); Added fix to mixer and test AudioMixer.RampedOutSourcesShouldNotBeMarkedMixed that covers that case. NOTRY=True ========== to ========== Updated mixer unittests and fixed a related bug in the new mixer. Changes to the mixer unittests: Removed the tests related to the former 'OutputMixer', as it's going to be removed. Removed incorrect comparison tests with the old mixer because doing identical mixing decisions with the old mixer proved unviable. When the new mixer went from kMaximumAmountOfMixedAudioSources in the last iteration to kMaximumAmountOfMixedAudioSources+1, it could hit an RTC_NOTREACHED(); Added fix to mixer and test AudioMixer.RampedOutSourcesShouldNotBeMarkedMixed that covers that case. NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Updated mixer unittests and fixed a related bug in the new mixer. Changes to the mixer unittests: Removed the tests related to the former 'OutputMixer', as it's going to be removed. Removed incorrect comparison tests with the old mixer because doing identical mixing decisions with the old mixer proved unviable. When the new mixer went from kMaximumAmountOfMixedAudioSources in the last iteration to kMaximumAmountOfMixedAudioSources+1, it could hit an RTC_NOTREACHED(); Added fix to mixer and test AudioMixer.RampedOutSourcesShouldNotBeMarkedMixed that covers that case. NOTRY=True ========== to ========== Updated mixer unittests and fixed a related bug in the new mixer. Changes to the mixer unittests: Removed the tests related to the former 'OutputMixer', as it's going to be removed. Removed incorrect comparison tests with the old mixer because doing identical mixing decisions with the old mixer proved unviable. When the new mixer went from kMaximumAmountOfMixedAudioSources in the last iteration to kMaximumAmountOfMixedAudioSources+1, it could hit an RTC_NOTREACHED(); Added fix to mixer and test AudioMixer.RampedOutSourcesShouldNotBeMarkedMixed that covers that case. NOTRY=True Committed: https://crrev.com/30be5d7cf43036fb3c1bc68a07d9c0dc28fec65a Cr-Commit-Position: refs/heads/master@{#13880} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/30be5d7cf43036fb3c1bc68a07d9c0dc28fec65a Cr-Commit-Position: refs/heads/master@{#13880} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
