Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1986093002:
Propagate muted info from VoE Channel to AudioConferenceMixer (Closed)
|
Created:
4 years, 7 months ago by hlundin-webrtc Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, minyue-webrtc, the sun Base URL:
https://chromium.googlesource.com/external/webrtc.git@mixer-mod-3 Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionPropagate muted info from VoE Channel to AudioConferenceMixer
Required updating of a few related classes and tests.
BUG=webrtc:5609
NOTRY=True
Committed: https://crrev.com/42dda50860ed348f26d8d4f74ba50190a067ee65
Cr-Commit-Position: refs/heads/master@{#12794}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : Addressing kwiberg's comments #
Total comments: 21
Patch Set 3 : Addressing comments from minyue and kwiberg #Patch Set 4 : Updating comments #Patch Set 5 : Fixing win build #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 32 (11 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
henrik.lundin@webrtc.org changed reviewers: + henrika@webrtc.org, minyue@webrtc.org
Hi, PTAL at this change. henrika: voice_engine minyue: all Thanks!
henrik.lundin@webrtc.org changed reviewers: + kwiberg@webrtc.org
+kwiberg for the API-dance around GetAudioFrame(WithMuted).
https://codereview.webrtc.org/1986093002/diff/40001/webrtc/modules/audio_conf... File webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h (right): https://codereview.webrtc.org/1986093002/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:29: AudioFrame* audioFrame) = 0; You need to add a default implementation of this one too. Until you do, subclasses arent't allowed to stop implementing it. You could take the implementation that this patch set puts in Channel. Or, even better if it works, just have an implementation that calls CHECK(false). It should work, since we assumed there were no out-of-tree callers... https://codereview.webrtc.org/1986093002/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:37: // If it returns -1, the frame will not be added to the mix. Mmmmm... maybe take the time to de-stone-age the return value while you're at it. Such as by having the function return a bool that says whether the frame should be added to the mix or not. Returning 0 and -1 when it's not even for error signalling is... inventive. Or even better, return a struct GotAudioFrame { bool muted; bool add_to_mix; }; It'll probably add a line or two to the callers, but it'll be easy to see what's happening.
Addressing kwiberg's comments
Addressing kwiberg's comments. PTAL again. https://codereview.webrtc.org/1986093002/diff/40001/webrtc/modules/audio_conf... File webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h (right): https://codereview.webrtc.org/1986093002/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:29: AudioFrame* audioFrame) = 0; On 2016/05/17 11:00:28, kwiberg-webrtc wrote: > You need to add a default implementation of this one too. Until you do, > subclasses arent't allowed to stop implementing it. > > You could take the implementation that this patch set puts in Channel. Or, even > better if it works, just have an implementation that calls CHECK(false). It > should work, since we assumed there were no out-of-tree callers... Done. I removed the implementation of this one in VoE Channel, since it was only there to avoid abstract class problems. https://codereview.webrtc.org/1986093002/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:37: // If it returns -1, the frame will not be added to the mix. On 2016/05/17 11:00:27, kwiberg-webrtc wrote: > Mmmmm... maybe take the time to de-stone-age the return value while you're at > it. Such as by having the function return a bool that says whether the frame > should be added to the mix or not. Returning 0 and -1 when it's not even for > error signalling is... inventive. > > Or even better, return a > > struct GotAudioFrame { bool muted; bool add_to_mix; }; > > It'll probably add a line or two to the callers, but it'll be easy to see what's > happening. Per offline discussion, I implemented this as an enum class.
lgtm, but some comments https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... File webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h (right): https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:37: RTC_CHECK(false); Add a comment such as // This function should not be called. It will remain for a short // time so that subclasses can override it without getting warnings. https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:42: // as follows. Grammar. https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:47: // - kError: audio_frame will not be used. To avoid having to keep comments and enum in sync, move enum here and annotate each enum case?
https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... File webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h (right): https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:26: kUnmuted, Maybe kNormal is better than kUnmuted https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:37: RTC_CHECK(false); 1. maybe DCHECK? 2. should still have a "return" line or no need? https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:44: // - kMuted: the samples in audio_frame will not be read, but will be "will not be read" or "will not be overwritten"? https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:50: return GetAudioFrame(id, audio_frame) != 0 ? "!=0" => "== -1" from what is written at line 34 https://codereview.webrtc.org/1986093002/diff/60001/webrtc/voice_engine/voe_e... File webrtc/voice_engine/voe_external_media_impl.cc (right): https://codereview.webrtc.org/1986093002/diff/60001/webrtc/voice_engine/voe_e... webrtc/voice_engine/voe_external_media_impl.cc:158: frame->Mute(); I am confused, isn't this memset frame's samples to zeros? is this the end of the story?
Addressing comments from minyue and kwiberg
Thanks for the comments! PTAL again. https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... File webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h (right): https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:26: kUnmuted, On 2016/05/17 14:59:16, minyue-webrtc wrote: > Maybe kNormal is better than kUnmuted Done. https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:37: RTC_CHECK(false); On 2016/05/17 12:57:06, kwiberg-webrtc wrote: > Add a comment such as > > // This function should not be called. It will remain for a short > // time so that subclasses can override it without getting warnings. Done. https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:37: RTC_CHECK(false); On 2016/05/17 14:59:16, minyue-webrtc wrote: > 1. maybe DCHECK? I followed kwiberg's recommendation to use RTC_CHECK. :) > > 2. should still have a "return" line or no need? If DCHECK, yes. But with CHECK, we'll see what the try bots say. https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:42: // as follows. On 2016/05/17 12:57:06, kwiberg-webrtc wrote: > Grammar. Done. https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:44: // - kMuted: the samples in audio_frame will not be read, but will be On 2016/05/17 14:59:16, minyue-webrtc wrote: > "will not be read" or "will not be overwritten"? "will not be read". This is the definition of a callback interface, and the comment promises what the AudioConferenceMixer will do with the callback and the results of it. https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:47: // - kError: audio_frame will not be used. On 2016/05/17 12:57:06, kwiberg-webrtc wrote: > To avoid having to keep comments and enum in sync, move enum here and annotate > each enum case? Style guide says no. But I did it anyway, since we will be back in compliance when GetAudioFrame is removed. https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:50: return GetAudioFrame(id, audio_frame) != 0 ? On 2016/05/17 14:59:16, minyue-webrtc wrote: > "!=0" => "== -1" from what is written at line 34 Done. https://codereview.webrtc.org/1986093002/diff/60001/webrtc/voice_engine/voe_e... File webrtc/voice_engine/voe_external_media_impl.cc (right): https://codereview.webrtc.org/1986093002/diff/60001/webrtc/voice_engine/voe_e... webrtc/voice_engine/voe_external_media_impl.cc:158: frame->Mute(); On 2016/05/17 14:59:17, minyue-webrtc wrote: > I am confused, isn't this memset frame's samples to zeros? is this the end of > the story? This is only the case in the VoEExternalMediaImpl, which is not part of our main audio flow (iiuc). So, in this special case, it is the end of the story, but in the "normal" case, the story ends in the AudioConferenceMixer, where the muted information is used to avoid calculating energies, and in most cases not generate any zero-samples.
lgtm https://codereview.webrtc.org/1986093002/diff/60001/webrtc/voice_engine/voe_e... File webrtc/voice_engine/voe_external_media_impl.cc (right): https://codereview.webrtc.org/1986093002/diff/60001/webrtc/voice_engine/voe_e... webrtc/voice_engine/voe_external_media_impl.cc:158: frame->Mute(); On 2016/05/17 19:57:51, hlundin-webrtc wrote: > On 2016/05/17 14:59:17, minyue-webrtc wrote: > > I am confused, isn't this memset frame's samples to zeros? is this the end of > > the story? > > This is only the case in the VoEExternalMediaImpl, which is not part of our main > audio flow (iiuc). So, in this special case, it is the end of the story, but in > the "normal" case, the story ends in the AudioConferenceMixer, where the muted > information is used to avoid calculating energies, and in most cases not > generate any zero-samples. Acknowledged.
lgtm I strongly suspect the trybots will force you to de-inline some virtual methods, though. https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... File webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h (right): https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:37: RTC_CHECK(false); On 2016/05/17 19:57:51, hlundin-webrtc wrote: > On 2016/05/17 14:59:16, minyue-webrtc wrote: > > 1. maybe DCHECK? > I followed kwiberg's recommendation to use RTC_CHECK. :) > > > > 2. should still have a "return" line or no need? > If DCHECK, yes. But with CHECK, we'll see what the try bots say. This line should never be reached, so there's no reason to optimize by using DCHECK instead of CHECK. The Windows bots used to require a return, even though all the other compilers did not because they recognized that CHECK(false) doesn't return. I haven't tried it since we updated from VS 2013 to 2015, though. If it does work without a return statement, go for it! https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:44: // - kMuted: the samples in audio_frame will not be read, but will be On 2016/05/17 19:57:51, hlundin-webrtc wrote: > On 2016/05/17 14:59:16, minyue-webrtc wrote: > > "will not be read" or "will not be overwritten"? > > "will not be read". This is the definition of a callback interface, and the > comment promises what the AudioConferenceMixer will do with the callback and the > results of it. It may be better to say "valid and should be used", and "should not be used". Because (1) this is instructions for how to use the interface from both ends, not assertions about how a specific implementation works, and (2) "should not be read" implies that the caller may not even peek at the values before ignoring them (or is this what you intended?).
https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... File webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h (right): https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:44: // - kMuted: the samples in audio_frame will not be read, but will be On 2016/05/18 02:45:06, kwiberg-webrtc wrote: > On 2016/05/17 19:57:51, hlundin-webrtc wrote: > > On 2016/05/17 14:59:16, minyue-webrtc wrote: > > > "will not be read" or "will not be overwritten"? > > > > "will not be read". This is the definition of a callback interface, and the > > comment promises what the AudioConferenceMixer will do with the callback and > the > > results of it. > > It may be better to say "valid and should be used", and "should not be used". > Because (1) this is instructions for how to use the interface from both ends, +1 > not assertions about how a specific implementation works, and (2) "should not be > read" implies that the caller may not even peek at the values before ignoring > them (or is this what you intended?).
Updating comments
We'll see about the de-inlining and missing return statement. My local linux-GN build works without problems at leeast. https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... File webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h (right): https://codereview.webrtc.org/1986093002/diff/60001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h:44: // - kMuted: the samples in audio_frame will not be read, but will be On 2016/05/18 02:53:51, minyue-webrtc wrote: > On 2016/05/18 02:45:06, kwiberg-webrtc wrote: > > On 2016/05/17 19:57:51, hlundin-webrtc wrote: > > > On 2016/05/17 14:59:16, minyue-webrtc wrote: > > > > "will not be read" or "will not be overwritten"? > > > > > > "will not be read". This is the definition of a callback interface, and the > > > comment promises what the AudioConferenceMixer will do with the callback and > > the > > > results of it. > > > > It may be better to say "valid and should be used", and "should not be used". > > Because (1) this is instructions for how to use the interface from both ends, > +1 > > > not assertions about how a specific implementation works, and (2) "should not > be > > read" implies that the caller may not even peek at the values before ignoring > > them (or is this what you intended?). > Done.
The CQ bit was checked by henrik.lundin@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1986093002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1986093002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/15123)
Fixing win build
LGTM
Description was changed from ========== Propagate muted info from VoE Channel to AudioConferenceMixer Required updating of a few related classes and tests. BUG=webrtc:5609 ========== to ========== Propagate muted info from VoE Channel to AudioConferenceMixer Required updating of a few related classes and tests. BUG=webrtc:5609 NOTRY=True ==========
The CQ bit was checked by henrik.lundin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1986093002/#ps120001 (title: "Fixing win build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1986093002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1986093002/120001
Message was sent while issue was closed.
Description was changed from ========== Propagate muted info from VoE Channel to AudioConferenceMixer Required updating of a few related classes and tests. BUG=webrtc:5609 NOTRY=True ========== to ========== Propagate muted info from VoE Channel to AudioConferenceMixer Required updating of a few related classes and tests. BUG=webrtc:5609 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 ========== Propagate muted info from VoE Channel to AudioConferenceMixer Required updating of a few related classes and tests. BUG=webrtc:5609 NOTRY=True ========== to ========== Propagate muted info from VoE Channel to AudioConferenceMixer Required updating of a few related classes and tests. BUG=webrtc:5609 NOTRY=True Committed: https://crrev.com/42dda50860ed348f26d8d4f74ba50190a067ee65 Cr-Commit-Position: refs/heads/master@{#12794} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/42dda50860ed348f26d8d4f74ba50190a067ee65 Cr-Commit-Position: refs/heads/master@{#12794} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
