Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2111293003:
Removed callback between old AudioConferenceMixer and OutputMixer. The audio frame with mixed audio… (Closed)
DescriptionRemoved callback in old AudioConferenceMixer.
OutputMixer and AudioConferenceMixer communicated via a callback. OutputMixer implemented an AudioMixerOutputReceiver interface, which defines the callback function NewMixedAudio. This has been removed and replaced by a simple function in the new mixer. The audio frame with mixed audio is now copied one time less. I have also removed one forward declaration.
Committed: https://crrev.com/09f45108c2eab3c6b94c10f28fdbababf52c25c3
Cr-Commit-Position: refs/heads/master@{#13550}
Patch Set 1 #Patch Set 2 : Format and header guards from parent CL #Patch Set 3 : Added the mixer to the BUILD.gn file for modules; added a test and removed the (hopefully) last par… #Patch Set 4 : Forgot to add test file. #Patch Set 5 : From upstream: renamed files, added gyp #Patch Set 6 : Added a test with both mixers that compares the results with expected for one participant and two m… #Patch Set 7 : Fixed style guide issues in unit test. #Patch Set 8 : Forgot git cl format #Patch Set 9 : Added variable name. #
Total comments: 24
Patch Set 10 : Removed asserts, decreased lock holding time & minor changes. #
Total comments: 4
Patch Set 11 : Minor things: specialized DCHECK macros, rearranged methods in header, etc. #
Total comments: 8
Patch Set 12 : Renamed 'Participant' into 'AudioSource' everywhere. #
Total comments: 2
Patch Set 13 : Renamed variables, removed DCHECK(false), changed back copyright years. #Dependent Patchsets: Messages
Total messages: 55 (33 generated)
Description was changed from ========== Removed callback between old AudioConferenceMixer and OutputMixer. The audio frame with mixed audio is now copied one time less. Also removed one forward declaration. ========== to ========== Removed callback between old AudioConferenceMixer and OutputMixer. The audio frame with mixed audio is now copied one time less. Also removed one forward declaration. Tests that check that changes to the mixer do not modify behavior are planned in this CL. See the design doc: https://docs.google.com/document/d/13WbsqJJXCOvS4Pq8lr2BmdQGtFWnvbVpvipOHVH2k... NOTRY=True ==========
Description was changed from ========== Removed callback between old AudioConferenceMixer and OutputMixer. The audio frame with mixed audio is now copied one time less. Also removed one forward declaration. Tests that check that changes to the mixer do not modify behavior are planned in this CL. See the design doc: https://docs.google.com/document/d/13WbsqJJXCOvS4Pq8lr2BmdQGtFWnvbVpvipOHVH2k... NOTRY=True ========== to ========== Removed callback between old AudioConferenceMixer and OutputMixer. The audio frame with mixed audio is now copied one time less. Also removed one forward declaration. Tests that check that changes to the mixer do not modify behavior are planned in this CL. See the design doc: https://docs.google.com/document/d/13WbsqJJXCOvS4Pq8lr2BmdQGtFWnvbVpvipOHVH2k... ==========
Description was changed from ========== Removed callback between old AudioConferenceMixer and OutputMixer. The audio frame with mixed audio is now copied one time less. Also removed one forward declaration. Tests that check that changes to the mixer do not modify behavior are planned in this CL. See the design doc: https://docs.google.com/document/d/13WbsqJJXCOvS4Pq8lr2BmdQGtFWnvbVpvipOHVH2k... ========== to ========== Removed callback between old AudioConferenceMixer and OutputMixer. The audio frame with mixed audio is now copied one time less. Also removed one forward declaration. Tests that check that changes to the mixer do not modify behavior are planned in this CL. See the design doc: https://docs.google.com/document/d/13WbsqJJXCOvS4Pq8lr2BmdQGtFWnvbVpvipOHVH2k... NOTRY=True ==========
The CQ bit was checked by aleloi@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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/6633)
Description was changed from ========== Removed callback between old AudioConferenceMixer and OutputMixer. The audio frame with mixed audio is now copied one time less. Also removed one forward declaration. Tests that check that changes to the mixer do not modify behavior are planned in this CL. See the design doc: https://docs.google.com/document/d/13WbsqJJXCOvS4Pq8lr2BmdQGtFWnvbVpvipOHVH2k... NOTRY=True ========== to ========== Removed callback in old AudioConferenceMixer. OutputMixer and AudioConferenceMixer communicated via a callback. OutputMixer implemented an AudioMixerOutputReceiver interface, which defines the callback function NewMixedAudio. This has been removed and replaced by a simple function in the new mixer. The audio frame with mixed audio is now copied one time less. I have also removed one forward declaration. TODO (fix/remove before reviewing) Tests that check that changes to the mixer do not modify behavior are planned in this CL. ==========
Description was changed from ========== Removed callback in old AudioConferenceMixer. OutputMixer and AudioConferenceMixer communicated via a callback. OutputMixer implemented an AudioMixerOutputReceiver interface, which defines the callback function NewMixedAudio. This has been removed and replaced by a simple function in the new mixer. The audio frame with mixed audio is now copied one time less. I have also removed one forward declaration. TODO (fix/remove before reviewing) Tests that check that changes to the mixer do not modify behavior are planned in this CL. ========== to ========== Removed callback in old AudioConferenceMixer. OutputMixer and AudioConferenceMixer communicated via a callback. OutputMixer implemented an AudioMixerOutputReceiver interface, which defines the callback function NewMixedAudio. This has been removed and replaced by a simple function in the new mixer. The audio frame with mixed audio is now copied one time less. I have also removed one forward declaration. ==========
aleloi@webrtc.org changed reviewers: + ivoc@webrtc.org
Here comes the first real mixer changes. PTAL! Two of the four tests are not mine: AnonymousAndNamed and LargestEnergyVadActiveMixed were written by mingue@. The other two tests what happens with one participant and one or two rounds of mixing.
aleloi@webrtc.org changed reviewers: + tommi@webrtc.org
https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer.cc (right): https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer.cc:163: _mixerModule.Mix(&_audioFrame); On what thread is this method called? Since there's no lock I'm guessing it's the same as all the other methods that touch _audioFrame, but if there's more to it, it would be good to understand. https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc (right): https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:191: RTC_NOTREACHED(); todo to remove? https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:198: assert(_processCalls == 0); can you change all assert()'s in this file to RTC_DCHECK? https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:277: audio_frame_for_mixing->UpdateFrame( is there a way to avoid calling UpdateFrame() while we hold the above lock? https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:281: _timeStamp += static_cast<uint32_t>(_sampleSize); _timeStamp is only used in this method, so possibly a lock is not required for it. However, if there's a chance that _sampleSize can change while this method is being called, a lock would be needed. If there's no chance of that though, then guarding _sampleSize and _outputFrequency with a lock, isn't needed and instead making sure that this callback cannot occur at the time that those variables may be modified, could replace the lock. https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:783: if (_numMixedParticipants == 1) { btw, it looks like this variable is the only reason why a lock is required to call this method. You could consider changing the method to accept this variable's value as an input param and make it static. That way, where you call it, you can fetch the value while holding the lock and have the chance of not holding the lock while mixing. https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.h (right): https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.h:75: void Mix(AudioFrame*) override; add param name https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. update this header (it's 2016) https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:18: #include "webrtc/modules/audio_mixer/include/audio_mixer_defines.h" fix include order. git cl format could help. https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:92: const int kId = 1; nit: make this 'static const int kId'. For non static const variables (i.e. non-compile-time-const) which might have different values per invocation of the function, they don't follow the kFoo naming style, but rather regular lower_case_with_underscore naming style.
Patchset #10 (id:180001) has been deleted
Patchset #10 (id:200001) has been deleted
Thank you for your comments, tommi@! I have implemented your suggestions and have tried to answer your questions. https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer.cc (right): https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer.cc:163: _mixerModule.Mix(&_audioFrame); On 2016/07/06 19:42:17, tommi-webrtc wrote: > On what thread is this method called? Since there's no lock I'm guessing it's > the same as all the other methods that touch _audioFrame, but if there's more to > it, it would be good to understand. Every method related to mixing in OutputMixer and AudioConferenceMixer (from which this code is copied) runs on the sound card / OS thread. I am planning to add ThreadChecks for this in a later CL. An exception is adding and removing participants (AudioConferenceMixer::SetMixingStatus), which should be possible to do from anywhere. https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc (right): https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:191: RTC_NOTREACHED(); On 2016/07/06 19:42:17, tommi-webrtc wrote: > todo to remove? Done. (There is also a dependent CL https://codereview.webrtc.org/2109333006/ that removes this). https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:198: assert(_processCalls == 0); On 2016/07/06 19:42:17, tommi-webrtc wrote: > can you change all assert()'s in this file to RTC_DCHECK? Done. https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:281: _timeStamp += static_cast<uint32_t>(_sampleSize); On 2016/07/06 19:42:17, tommi-webrtc wrote: > _timeStamp is only used in this method, so possibly a lock is not required for > it. However, if there's a chance that _sampleSize can change while this method > is being called, a lock would be needed. If there's no chance of that though, > then guarding _sampleSize and _outputFrequency with a lock, isn't needed and > instead making sure that this callback cannot occur at the time that those > variables may be modified, could replace the lock. _sampleSize and _outputFrequency are only changed through calls from this method (::Mix), and the frame is only touched by the sound card OS thread, so the lock may be safely removed. https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:783: if (_numMixedParticipants == 1) { On 2016/07/06 19:42:17, tommi-webrtc wrote: > btw, it looks like this variable is the only reason why a lock is required to > call this method. > You could consider changing the method to accept this variable's value as an > input param and make it static. That way, where you call it, you can fetch the > value while holding the lock and have the chance of not holding the lock while > mixing. Thank you for the idea! I think more unnecessary locks can be removed this way. I have implemented your suggestion. Is it OK to wait for a thorough review of the lock usage to another CL, though? https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.h (right): https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.h:75: void Mix(AudioFrame*) override; On 2016/07/06 19:42:17, tommi-webrtc wrote: > add param name Done. https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. On 2016/07/06 19:42:17, tommi-webrtc wrote: > update this header (it's 2016) Done. I also updated the other files (they are copied from the old mixer to this new directory and are planned to go through almost a complete rewrite). https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:18: #include "webrtc/modules/audio_mixer/include/audio_mixer_defines.h" On 2016/07/06 19:42:17, tommi-webrtc wrote: > fix include order. git cl format could help. Done. https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:92: const int kId = 1; On 2016/07/06 19:42:17, tommi-webrtc wrote: > nit: make this 'static const int kId'. For non static const variables (i.e. > non-compile-time-const) which might have different values per invocation of the > function, they don't follow the kFoo naming style, but rather regular > lower_case_with_underscore naming style. Done. The test was mostly copied from modules/audio_conference_mixer/test.
https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.h (right): https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.h:75: void Mix(AudioFrame*) override; Try to keep the order of the functions the same as in the new_audio_conference_mixer.h header. https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:21: namespace webrtc { Move this below the "using" lines below. https://codereview.webrtc.org/2111293003/diff/220001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer.cc (right): https://codereview.webrtc.org/2111293003/diff/220001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer.cc:50: assert(id == _instanceId); Please convert asserts to DCHECKs in this file as well. https://codereview.webrtc.org/2111293003/diff/220001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/include/new_audio_conference_mixer.h (right): https://codereview.webrtc.org/2111293003/diff/220001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/include/new_audio_conference_mixer.h:2: * Copyright (c) 2011 The WebRTC project authors. All Rights Reserved. Still missing some 2016's. Or will this be removed? https://codereview.webrtc.org/2111293003/diff/220001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc (right): https://codereview.webrtc.org/2111293003/diff/220001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:199: RTC_DCHECK(_processCalls == 0); Please use RTC_DCHECK_EQ, RTC_DCHECK_NEQ, RTC_DCHECK_GT, etc., for comparisons, here and elsewhere, as it gives better error messages.
Thank you for your comments, ivoc@! I have answered them and uploaded a new patch set. https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.h (right): https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.h:75: void Mix(AudioFrame*) override; On 2016/07/07 13:46:16, ivoc wrote: > Try to keep the order of the functions the same as in the > new_audio_conference_mixer.h header. Done. https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:21: namespace webrtc { On 2016/07/07 13:46:16, ivoc wrote: > Move this below the "using" lines below. Done. https://codereview.webrtc.org/2111293003/diff/220001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc (right): https://codereview.webrtc.org/2111293003/diff/220001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:199: RTC_DCHECK(_processCalls == 0); On 2016/07/07 13:46:16, ivoc wrote: > Please use RTC_DCHECK_EQ, RTC_DCHECK_NEQ, RTC_DCHECK_GT, etc., for comparisons, > here and elsewhere, as it gives better error messages. Done.
The CQ bit was checked by aleloi@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: CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
Patchset #11 (id:240001) has been deleted
The CQ bit was checked by aleloi@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_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/16500) linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/3373) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
Patchset #11 (id:260001) has been deleted
The CQ bit was checked by aleloi@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: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/12591)
Patchset #12 (id:300001) has been deleted
Patchset #11 (id:280001) has been deleted
The CQ bit was checked by aleloi@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.
Patchset #12 (id:340001) has been deleted
The CQ bit was checked by aleloi@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/...
https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc (right): https://codereview.webrtc.org/2111293003/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:281: _timeStamp += static_cast<uint32_t>(_sampleSize); On 2016/07/07 09:20:16, aleloi wrote: > On 2016/07/06 19:42:17, tommi-webrtc wrote: > > _timeStamp is only used in this method, so possibly a lock is not required for > > it. However, if there's a chance that _sampleSize can change while this > method > > is being called, a lock would be needed. If there's no chance of that though, > > then guarding _sampleSize and _outputFrequency with a lock, isn't needed and > > instead making sure that this callback cannot occur at the time that those > > variables may be modified, could replace the lock. > > _sampleSize and _outputFrequency are only changed through calls from this method > (::Mix), and the frame is only touched by the sound card OS thread, so the lock > may be safely removed. Acknowledged. https://codereview.webrtc.org/2111293003/diff/320001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer.h (right): https://codereview.webrtc.org/2111293003/diff/320001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. copyright year of files that are not new in this changelist, should not be changed. https://codereview.webrtc.org/2111293003/diff/320001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc (right): https://codereview.webrtc.org/2111293003/diff/320001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:252: RTC_DCHECK(false); nit: RTC_NOTREACHED() https://codereview.webrtc.org/2111293003/diff/320001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:267: // TODO(henrike): it might be better to decide the number of channels henrike doesn't work on webrtc anymore. Can you assign this to someone else or remove it if it's no longer relevant? https://codereview.webrtc.org/2111293003/diff/320001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:357: RTC_DCHECK(false); RTC_NOTREACHED() here as well (and below). Down the line, we should consider deleting blocks of code like this (condition + body) and instead do: RTC_CHECK(success); or RTC_DCHECK(success); That is, if the above calls should really never fail. https://codereview.webrtc.org/2111293003/diff/360001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.h (right): https://codereview.webrtc.org/2111293003/diff/360001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.h:166: size_t _numMixedAudioSources; nit: Since you're renaming this variable, you might as well rename it to num_mixed_audio_sources_. Same for the ones above.
I have responded to the comments and uploaded a new patch set. https://codereview.webrtc.org/2111293003/diff/320001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer.h (right): https://codereview.webrtc.org/2111293003/diff/320001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2016/07/08 12:24:17, tommi-webrtc wrote: > copyright year of files that are not new in this changelist, should not be > changed. Done. https://codereview.webrtc.org/2111293003/diff/320001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc (right): https://codereview.webrtc.org/2111293003/diff/320001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:252: RTC_DCHECK(false); On 2016/07/08 12:24:17, tommi-webrtc wrote: > nit: RTC_NOTREACHED() Done. https://codereview.webrtc.org/2111293003/diff/320001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:267: // TODO(henrike): it might be better to decide the number of channels On 2016/07/08 12:24:17, tommi-webrtc wrote: > henrike doesn't work on webrtc anymore. Can you assign this to someone else or > remove it if it's no longer relevant? Done. https://codereview.webrtc.org/2111293003/diff/320001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:357: RTC_DCHECK(false); On 2016/07/08 12:24:17, tommi-webrtc wrote: > RTC_NOTREACHED() here as well (and below). Down the line, we should consider > deleting blocks of code like this (condition + body) and instead do: > > RTC_CHECK(success); > or > RTC_DCHECK(success); > > That is, if the above calls should really never fail. Done. https://codereview.webrtc.org/2111293003/diff/360001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.h (right): https://codereview.webrtc.org/2111293003/diff/360001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.h:166: size_t _numMixedAudioSources; On 2016/07/08 12:24:18, tommi-webrtc wrote: > nit: Since you're renaming this variable, you might as well rename it to > num_mixed_audio_sources_. Same for the ones above. Done.
The CQ bit was checked by aleloi@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/...
lgtm. Thanks for all the extra fixes.
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)
LGTM
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 ========== Removed callback in old AudioConferenceMixer. OutputMixer and AudioConferenceMixer communicated via a callback. OutputMixer implemented an AudioMixerOutputReceiver interface, which defines the callback function NewMixedAudio. This has been removed and replaced by a simple function in the new mixer. The audio frame with mixed audio is now copied one time less. I have also removed one forward declaration. ========== to ========== Removed callback in old AudioConferenceMixer. OutputMixer and AudioConferenceMixer communicated via a callback. OutputMixer implemented an AudioMixerOutputReceiver interface, which defines the callback function NewMixedAudio. This has been removed and replaced by a simple function in the new mixer. The audio frame with mixed audio is now copied one time less. I have also removed one forward declaration. ==========
Message was sent while issue was closed.
Committed patchset #13 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Removed callback in old AudioConferenceMixer. OutputMixer and AudioConferenceMixer communicated via a callback. OutputMixer implemented an AudioMixerOutputReceiver interface, which defines the callback function NewMixedAudio. This has been removed and replaced by a simple function in the new mixer. The audio frame with mixed audio is now copied one time less. I have also removed one forward declaration. ========== to ========== Removed callback in old AudioConferenceMixer. OutputMixer and AudioConferenceMixer communicated via a callback. OutputMixer implemented an AudioMixerOutputReceiver interface, which defines the callback function NewMixedAudio. This has been removed and replaced by a simple function in the new mixer. The audio frame with mixed audio is now copied one time less. I have also removed one forward declaration. Committed: https://crrev.com/09f45108c2eab3c6b94c10f28fdbababf52c25c3 Cr-Commit-Position: refs/heads/master@{#13550} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/09f45108c2eab3c6b94c10f28fdbababf52c25c3 Cr-Commit-Position: refs/heads/master@{#13550} |
