Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2408683002:
Cleanup of the mixer interface. (Closed)
DescriptionImplemented further mixer interface change suggestions from https://codereview.webrtc.org/2386383003/
Changed mixability status into AddSource/RemoveSource. Added 'ssrc()'
method to the MixerSource interface. Removed unnecessary member 'num_audio_sources_' and made the mixer be refcounted.
BUG=webrtc:6346
NOTRY=True
Committed: https://crrev.com/116ec6da50c7c82b2341b97e2d6d512ade3f6e00
Cr-Commit-Position: refs/heads/master@{#14612}
Patch Set 1 #
Total comments: 24
Patch Set 2 : Added checks in Add/Remove source, double add or nonexistent remove is an error. #
Total comments: 7
Patch Set 3 : Lock <-> DCHECK. #Patch Set 4 : Rebase #
Total comments: 9
Patch Set 5 : Moved audio_mixer_unittest from test/ #Patch Set 6 : Added clarifying comments for adding/removing sources and threading aspects of the interface. #
Depends on Patchset: Messages
Total messages: 28 (14 generated)
Description was changed from ========== Implemented further mixer interface change suggestions. Changed mixability status into AddSource/RemoveSource. Added 'ssrc()' method to the MixerSource interface. BUG=webrtc:6346 ========== to ========== Implemented further mixer interface change suggestions from https://codereview.webrtc.org/2386383003/ Changed mixability status into AddSource/RemoveSource. Added 'ssrc()' method to the MixerSource interface. BUG=webrtc:6346 ==========
aleloi@webrtc.org changed reviewers: + solenberg@webrtc.org
Description was changed from ========== Implemented further mixer interface change suggestions from https://codereview.webrtc.org/2386383003/ Changed mixability status into AddSource/RemoveSource. Added 'ssrc()' method to the MixerSource interface. BUG=webrtc:6346 ========== to ========== Implemented further mixer interface change suggestions from https://codereview.webrtc.org/2386383003/ Changed mixability status into AddSource/RemoveSource. Added 'ssrc()' method to the MixerSource interface. Removed unnecessary member 'num_audio_sources_'. BUG=webrtc:6346 ==========
Description was changed from ========== Implemented further mixer interface change suggestions from https://codereview.webrtc.org/2386383003/ Changed mixability status into AddSource/RemoveSource. Added 'ssrc()' method to the MixerSource interface. Removed unnecessary member 'num_audio_sources_'. BUG=webrtc:6346 ========== to ========== Implemented further mixer interface change suggestions from https://codereview.webrtc.org/2386383003/ Changed mixability status into AddSource/RemoveSource. Added 'ssrc()' method to the MixerSource interface. Removed unnecessary member 'num_audio_sources_' and made the mixer be refcounted. BUG=webrtc:6346 ==========
Here is part 2. Please take a look! https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer.h (left): https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer.h:22: static const int kMaximumAmountOfMixedAudioSources = 3; This is moved to AudioMixerImpl. https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer.h:52: static std::unique_ptr<AudioMixer> Create(); Factory methods are moved to the implementing class. https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer.h (right): https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer.h:54: ~AudioMixer() override {} Since RefCountInterface already specifies a virtual destructor. https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.cc:215: use_limiter_ = mix_list.size() > 1; This change slightly alters the mixing behavior. If a frame is muted, it contributed to num_mixed_audio_sources, but not to mix_list.size(). The change is safe, because this is how the old mixer works: exactly num_mixed_audio_sources are always mixed. https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:48: static rtc::scoped_refptr<AudioMixerImpl> Create(); When we move the interface to webrtc/api, we can add a 'create_mixer.h' with 'rtc::scoped_refptr<AudioMixer> CreateTopTreeMixer()' that calls this function. Alternatively, I can add it to this CL. https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:50: explicit AudioMixerImpl(std::unique_ptr<AudioProcessing> limiter); A refcounted object must have a public constructor. https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/te... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/te... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:160: const auto mixer = AudioMixerImpl::Create(); Since there is no AudioMixer::Create any longer.
https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer.h (left): https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer.h:22: static const int kMaximumAmountOfMixedAudioSources = 3; On 2016/10/10 13:06:31, aleloi wrote: > This is moved to AudioMixerImpl. Acknowledged. https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer.h:52: static std::unique_ptr<AudioMixer> Create(); On 2016/10/10 13:06:31, aleloi wrote: > Factory methods are moved to the implementing class. Acknowledged. https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer.h (right): https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer.h:54: ~AudioMixer() override {} On 2016/10/10 13:06:31, aleloi wrote: > Since RefCountInterface already specifies a virtual destructor. Acknowledged. https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.cc:215: use_limiter_ = mix_list.size() > 1; On 2016/10/10 13:06:31, aleloi wrote: > This change slightly alters the mixing behavior. If a frame is muted, it > contributed to num_mixed_audio_sources, but not to mix_list.size(). The change > is safe, because this is how the old mixer works: exactly > num_mixed_audio_sources are always mixed. Acknowledged. https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.cc:243: bool AudioMixerImpl::AddSource(Source* audio_source) { RTC_DCHECK(audio_source); https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.cc:249: return false; I think you should just DCHECK(!is_mixed) and always return true from this method: RTC_CHECK_EQ(FindSourceInList(audio_source, &audio_source_list_), audio_source_list_.end()); Because this is a logic error. The client code should never call the interface like that. Note that since we're making this an api/ interface, we will still need the return code though, as there may be other reasons the method may fail, for other implementations. https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.cc:255: bool AudioMixerImpl::RemoveSource(Source* audio_source) { Same comments apply as in AddSource() https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:19: #include "webrtc/base/scoped_ref_ptr.h" nit: s comes before t in western european alphabets https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:48: static rtc::scoped_refptr<AudioMixerImpl> Create(); On 2016/10/10 13:06:31, aleloi wrote: > When we move the interface to webrtc/api, we can add a 'create_mixer.h' with > 'rtc::scoped_refptr<AudioMixer> CreateTopTreeMixer()' that calls this function. > Alternatively, I can add it to this CL. It's ok to do it separately. Once you get around to it, for consistency, name the file "builtin_audio_mixer.h", and also you will want to throw in an 'h' at a strategic point in the function name. :) https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:50: explicit AudioMixerImpl(std::unique_ptr<AudioProcessing> limiter); On 2016/10/10 13:06:31, aleloi wrote: > A refcounted object must have a public constructor. No, protected should be enough if you're using rtc::RefCountedObject https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/te... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/te... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:101: EXPECT_TRUE(mixer->AddSource(&participants[i])); wow, look at that! it's so easy to understand now! https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/te... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:160: const auto mixer = AudioMixerImpl::Create(); On 2016/10/10 13:06:31, aleloi wrote: > Since there is no AudioMixer::Create any longer. Acknowledged.
aleloi@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
Uploaded new patch set. https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.cc:249: return false; On 2016/10/10 13:35:49, the sun wrote: > I think you should just DCHECK(!is_mixed) and always return true from this > method: > > RTC_CHECK_EQ(FindSourceInList(audio_source, &audio_source_list_), > audio_source_list_.end()); > > Because this is a logic error. The client code should never call the interface > like that. Note that since we're making this an api/ interface, we will still > need the return code though, as there may be other reasons the method may fail, > for other implementations. I agree: it's better to see this as an error instead of having clients silently ignore a returned 'false'. https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.cc:255: bool AudioMixerImpl::RemoveSource(Source* audio_source) { On 2016/10/10 13:35:49, the sun wrote: > Same comments apply as in AddSource() Done. https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:19: #include "webrtc/base/scoped_ref_ptr.h" On 2016/10/10 13:35:49, the sun wrote: > nit: s comes before t in western european alphabets Done. https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:48: static rtc::scoped_refptr<AudioMixerImpl> Create(); On 2016/10/10 13:35:49, the sun wrote: > On 2016/10/10 13:06:31, aleloi wrote: > > When we move the interface to webrtc/api, we can add a 'create_mixer.h' with > > 'rtc::scoped_refptr<AudioMixer> CreateTopTreeMixer()' that calls this > function. > > Alternatively, I can add it to this CL. > > It's ok to do it separately. Once you get around to it, for consistency, name > the file "builtin_audio_mixer.h", and also you will want to throw in an 'h' at a > strategic point in the function name. :) Acknowledged. https://codereview.webrtc.org/2408683002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:50: explicit AudioMixerImpl(std::unique_ptr<AudioProcessing> limiter); On 2016/10/10 13:35:49, the sun wrote: > On 2016/10/10 13:06:31, aleloi wrote: > > A refcounted object must have a public constructor. > > No, protected should be enough if you're using rtc::RefCountedObject Done.
lgtm % nits https://codereview.webrtc.org/2408683002/diff/20001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2408683002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:245: RTC_DCHECK(audio_source); nit: check arguments before claiming lock https://codereview.webrtc.org/2408683002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:246: RTC_DCHECK(FindSourceInList(audio_source, &audio_source_list_) == Use the _EQ, _NE etc versions of the macros as you'll get nicer printouts whenever it hits. https://codereview.webrtc.org/2408683002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:255: RTC_DCHECK(audio_source); same nits as above
https://codereview.webrtc.org/2408683002/diff/20001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2408683002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:245: RTC_DCHECK(audio_source); On 2016/10/10 14:16:27, the sun wrote: > nit: check arguments before claiming lock Done. https://codereview.webrtc.org/2408683002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:246: RTC_DCHECK(FindSourceInList(audio_source, &audio_source_list_) == On 2016/10/10 14:16:27, the sun wrote: > Use the _EQ, _NE etc versions of the macros as you'll get nicer printouts > whenever it hits. RTC_DCHECK_EQ seems to be implemented with template specializations for some of primitive types. It can't be used to compare iterators. Therefore I went with DCHECK https://codereview.webrtc.org/2408683002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:255: RTC_DCHECK(audio_source); On 2016/10/10 14:16:27, the sun wrote: > same nits as above Done.
https://codereview.webrtc.org/2408683002/diff/20001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2408683002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:246: RTC_DCHECK(FindSourceInList(audio_source, &audio_source_list_) == On 2016/10/10 14:50:35, aleloi wrote: > On 2016/10/10 14:16:27, the sun wrote: > > Use the _EQ, _NE etc versions of the macros as you'll get nicer printouts > > whenever it hits. > > RTC_DCHECK_EQ seems to be implemented with template specializations for some of > primitive types. It can't be used to compare iterators. Therefore I went with > DCHECK Ah, of course, I guess it is stringstream::operator<< is not implemented for iterators of arbitrary types. Good call.
https://codereview.webrtc.org/2408683002/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer.h (right): https://codereview.webrtc.org/2408683002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.h:57: // source twice is forbidden, as is removing a source that is not Does it have to be "forbidden" in all implementations of this interface? I would suggest a re-write to something like "Returns true if adding/removing was successful. Adding the same source twice or removing a source that is not present would make a typical implementation return false." WDYT? https://codereview.webrtc.org/2408683002/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2408683002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:246: RTC_DCHECK(FindSourceInList(audio_source, &audio_source_list_) == The argument of DCHECK is not evaluated in release builds. I don't think you should DCHECK here, but rather simply return false as the interface promises. https://codereview.webrtc.org/2408683002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:257: RTC_DCHECK(iter != audio_source_list_.end()) << "Source not present in mixer"; Same here.
https://codereview.webrtc.org/2408683002/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer.h (right): https://codereview.webrtc.org/2408683002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.h:57: // source twice is forbidden, as is removing a source that is not On 2016/10/11 11:50:21, hlundin-webrtc wrote: > Does it have to be "forbidden" in all implementations of this interface? I would > suggest a re-write to something like > "Returns true if adding/removing was successful. Adding the same source twice or > removing a source that is not present would make a typical implementation return > false." > WDYT? It's important to not mix up the interface and the framework. We own the framework, and the canonical implementation. That's why the latter should not be defensive and return false if a source is added twice. Instead it should blow up (assert) as quickly as possible. This way we can check our framework implementation. In the *interface* however, we need to provide the possibility for the implementer to return false, because they may depend on some resource which may or may not be available. We can state in the comment that the method will never be called multiple times for the same source. But to say "forbidden" in this context, to me, is mixing up the viewpoints. https://codereview.webrtc.org/2408683002/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2408683002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:246: RTC_DCHECK(FindSourceInList(audio_source, &audio_source_list_) == On 2016/10/11 11:50:21, hlundin-webrtc wrote: > The argument of DCHECK is not evaluated in release builds. I don't think you > should DCHECK here, but rather simply return false as the interface promises. That's exactly the point. Don't evaluate since it isn't needed. https://codereview.webrtc.org/2408683002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:257: RTC_DCHECK(iter != audio_source_list_.end()) << "Source not present in mixer"; On 2016/10/11 11:50:21, hlundin-webrtc wrote: > Same here. But here "iter" is used for the deletion.
lgtm https://codereview.webrtc.org/2408683002/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer.h (right): https://codereview.webrtc.org/2408683002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.h:57: // source twice is forbidden, as is removing a source that is not On 2016/10/11 12:10:55, the sun wrote: > On 2016/10/11 11:50:21, hlundin-webrtc wrote: > > Does it have to be "forbidden" in all implementations of this interface? I > would > > suggest a re-write to something like > > "Returns true if adding/removing was successful. Adding the same source twice > or > > removing a source that is not present would make a typical implementation > return > > false." > > WDYT? > > It's important to not mix up the interface and the framework. We own the > framework, and the canonical implementation. That's why the latter should not be > defensive and return false if a source is added twice. Instead it should blow up > (assert) as quickly as possible. This way we can check our framework > implementation. > > In the *interface* however, we need to provide the possibility for the > implementer to return false, because they may depend on some resource which may > or may not be available. We can state in the comment that the method will never > be called multiple times for the same source. But to say "forbidden" in this > context, to me, is mixing up the viewpoints. OK, solenberg, your turn to have a go at formulating the comment. :) https://codereview.webrtc.org/2408683002/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2408683002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:257: RTC_DCHECK(iter != audio_source_list_.end()) << "Source not present in mixer"; On 2016/10/11 12:10:56, the sun wrote: > On 2016/10/11 11:50:21, hlundin-webrtc wrote: > > Same here. > > But here "iter" is used for the deletion. Yes, iter is needed in any case. I was referring to the DCHECK of the "not found" case. But since we promise not to remove a nonexisting source, this is fine.
Patchset #7 (id:120001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:140001) has been deleted
https://codereview.webrtc.org/2408683002/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer.h (right): https://codereview.webrtc.org/2408683002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.h:57: // source twice is forbidden, as is removing a source that is not On 2016/10/11 12:49:54, hlundin-webrtc wrote: > On 2016/10/11 12:10:55, the sun wrote: > > On 2016/10/11 11:50:21, hlundin-webrtc wrote: > > > Does it have to be "forbidden" in all implementations of this interface? I > > would > > > suggest a re-write to something like > > > "Returns true if adding/removing was successful. Adding the same source > twice > > or > > > removing a source that is not present would make a typical implementation > > return > > > false." > > > WDYT? > > > > It's important to not mix up the interface and the framework. We own the > > framework, and the canonical implementation. That's why the latter should not > be > > defensive and return false if a source is added twice. Instead it should blow > up > > (assert) as quickly as possible. This way we can check our framework > > implementation. > > > > In the *interface* however, we need to provide the possibility for the > > implementer to return false, because they may depend on some resource which > may > > or may not be available. We can state in the comment that the method will > never > > be called multiple times for the same source. But to say "forbidden" in this > > context, to me, is mixing up the viewpoints. > > OK, solenberg, your turn to have a go at formulating the comment. :) We want to point out that Add / remove mustn't be called twice and maybe that the default implementation ensures that by asserting. We want to allow an implementation to return a 'success' flag, but not to reserve the flag for when the condition above is violated. Does this correctly summarizes the discussion so far?
Description was changed from ========== Implemented further mixer interface change suggestions from https://codereview.webrtc.org/2386383003/ Changed mixability status into AddSource/RemoveSource. Added 'ssrc()' method to the MixerSource interface. Removed unnecessary member 'num_audio_sources_' and made the mixer be refcounted. BUG=webrtc:6346 ========== to ========== Implemented further mixer interface change suggestions from https://codereview.webrtc.org/2386383003/ Changed mixability status into AddSource/RemoveSource. Added 'ssrc()' method to the MixerSource interface. Removed unnecessary member 'num_audio_sources_' and made the mixer be refcounted. BUG=webrtc:6346 NOTRY=True ==========
NOTRY=True since all tests have passed recently.
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2408683002/#ps180001 (title: "Added clarifying comments for adding/removing sources and threading aspects of the interface.")
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 ========== Implemented further mixer interface change suggestions from https://codereview.webrtc.org/2386383003/ Changed mixability status into AddSource/RemoveSource. Added 'ssrc()' method to the MixerSource interface. Removed unnecessary member 'num_audio_sources_' and made the mixer be refcounted. BUG=webrtc:6346 NOTRY=True ========== to ========== Implemented further mixer interface change suggestions from https://codereview.webrtc.org/2386383003/ Changed mixability status into AddSource/RemoveSource. Added 'ssrc()' method to the MixerSource interface. Removed unnecessary member 'num_audio_sources_' and made the mixer be refcounted. BUG=webrtc:6346 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Implemented further mixer interface change suggestions from https://codereview.webrtc.org/2386383003/ Changed mixability status into AddSource/RemoveSource. Added 'ssrc()' method to the MixerSource interface. Removed unnecessary member 'num_audio_sources_' and made the mixer be refcounted. BUG=webrtc:6346 NOTRY=True ========== to ========== Implemented further mixer interface change suggestions from https://codereview.webrtc.org/2386383003/ Changed mixability status into AddSource/RemoveSource. Added 'ssrc()' method to the MixerSource interface. Removed unnecessary member 'num_audio_sources_' and made the mixer be refcounted. BUG=webrtc:6346 NOTRY=True Committed: https://crrev.com/116ec6da50c7c82b2341b97e2d6d512ade3f6e00 Cr-Commit-Position: refs/heads/master@{#14612} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/116ec6da50c7c82b2341b97e2d6d512ade3f6e00 Cr-Commit-Position: refs/heads/master@{#14612} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
