Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2396483002:
Made MixerAudioSource a pure interface. (Closed)
DescriptionMade MixerAudioSource a pure interface.
This required quite a few small changes in the mixing algorithm
structure, the mixer interface and the mixer unit tests.
BUG=webrtc:6346
Committed: https://crrev.com/2ae5fdff86b784545cbd724de54bb5ffedde1adf
Cr-Commit-Position: refs/heads/master@{#14567}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Comments describing new AudioMixerImpl methods. #Patch Set 3 : Rebase. #
Total comments: 41
Patch Set 4 : Small changes in response to reviewer comments. #
Total comments: 18
Patch Set 5 : Changes in response to reviewer comments. #Patch Set 6 : rebase. #
Total comments: 6
Patch Set 7 : Removing dependency patch set. #Messages
Total messages: 42 (18 generated)
aleloi@webrtc.org changed reviewers: + ivoc@webrtc.org, solenberg@webrtc.org
https://codereview.webrtc.org/2396483002/diff/1/webrtc/modules/audio_mixer/BU... File webrtc/modules/audio_mixer/BUILD.gn (right): https://codereview.webrtc.org/2396483002/diff/1/webrtc/modules/audio_mixer/BU... webrtc/modules/audio_mixer/BUILD.gn:37: "../../base:gtest_prod", To make the tests friends with AudioMixerImpl::Create so that tests can access GetAudioSourceMixabilityStatus without having to expose it to the mixer interface. https://codereview.webrtc.org/2396483002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (left): https://codereview.webrtc.org/2396483002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.cc:470: audio_source->ResetMixedStatus(); Not needed, since only the mixer has the mixed status information now. The status information was contained in AudioSourecWithMixStatus, which is erased on the line before. https://codereview.webrtc.org/2396483002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2396483002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.cc:60: }; A SourceFrame now holds a pointer audio_source_ to AudioSourceWithMixStatus. was_mixed_before is redundant, because it is audio_source_->WasMixed(). https://codereview.webrtc.org/2396483002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.cc:542: } I couldn't find any way to simplify this. The code now contains four almost identical find_ifs. Does anyone have any suggestions? https://codereview.webrtc.org/2396483002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl.h (left): https://codereview.webrtc.org/2396483002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:64: AudioFrameList GetNonAnonymousAudio() const EXCLUSIVE_LOCKS_REQUIRED(crit_); This function sets the mixed status of an AudioSourceWithStatus that is owned by the mixer. Previously, it called (non-const) SetIsMixed on a participant pointer, but now the mixer owns the status object. https://codereview.webrtc.org/2396483002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:68: AudioFrameList GetAnonymousAudio() const EXCLUSIVE_LOCKS_REQUIRED(crit_); Same for this one. https://codereview.webrtc.org/2396483002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2396483002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:33: typedef std::vector<AudioSourceWithMixStatus> MixerAudioSourceList; Instead of storing lists of participant pointers, we store a list of AudioSourceWithMixStatus. It's new class that is a wrapper around a participant pointer and a flag that is |true| if the participant was mixer last round. https://codereview.webrtc.org/2396483002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:40: static std::unique_ptr<AudioMixerImpl> Create(int id); In order to allow the tests to access an AudioMixerImpl instance without having to cast it. Note: we could make it public to avoid declaring it a friend of the tests. https://codereview.webrtc.org/2396483002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:57: Forgot to add a comment, see the next patch set. https://codereview.webrtc.org/2396483002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:94: Comment added in next patch set. https://codereview.webrtc.org/2396483002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:126: FRIEND_TEST_ALL_PREFIXES(AudioMixerTest, Create); Alternatively, we could make ::Create public.
Description was changed from ========== Made MixerAudioSource a pure interface. This required quite a few small changes in the mixing algorithm structure, the mixer interface and the mixer unit tests. BUG=6346 ========== to ========== Made MixerAudioSource a pure interface. This required quite a few small changes in the mixing algorithm structure, the mixer interface and the mixer unit tests. BUG=webrtc:6346 ==========
On 2016/10/04 13:37:18, aleloi wrote: FYI: remember to prefix the bug # with "webrtc:", otherwise the clickable link goes to the wrong bug tracker (Chromium)
https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_defines.h (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_defines.h:49: virtual ~MixerAudioSource(); - Remove ctor. - Make the dtor inline and move to top of class declaration (among the methods). https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:395: AudioFrameList AudioMixerImpl::GetAnonymousAudio() { Why do we need to treat "anonymous" audio differently? As I see it, "always included in mix" is an attribute of the source (or at least the mixer's internal representation of a source - meta data) and this attribute can be taken into account when selecting the set of streams to mix. Keeping a separate "additional_audio_source_list_" seems like making things more complicated than they need to be. https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:506: AudioSourceWithMixStatus* AudioMixerImpl::GetSourceWithStatus( If this is only used for unit testing, can you mark it as such? Just a sanity check - the unit test is not running multiple threads, is it? https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:59: bool GetAudioSourceMixabilityStatus(MixerAudioSource* participant); Source or participant? Make thy mind up! :) https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:132: FRIEND_TEST_ALL_PREFIXES(AudioMixerTest, Create); Can we test without creating a circular relationship between the mixer impl and the test cases? Please? https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_source_with_mix_status.h (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_source_with_mix_status.h:19: class AudioSourceWithMixStatus { How about making this class internal to the AudioMixerImpl and naming it AudioSourceState or something? https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_source_with_mix_status.h:24: AudioSourceWithMixStatus(const AudioSourceWithMixStatus&) = default; Uhm, really? Who's owning the MixerAudioSource? I don't really like bare pointers... https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_source_with_mix_status.h:40: bool is_mixed_ = false; RTC_DISALLOW_IMPLICIT_CONSTRUCTORS https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:378: EXPECT_TRUE(mixer->GetAudioSourceMixabilityStatus(&participants[i])) This method is public on the mixer? Why then the need for the friend declarations?
https://codereview.webrtc.org/2396483002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2396483002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:40: static std::unique_ptr<AudioMixerImpl> Create(int id); On 2016/10/04 13:37:16, aleloi wrote: > In order to allow the tests to access an AudioMixerImpl instance without having > to cast it. Note: we could make it public to avoid declaring it a friend of the > tests. Isn't this public already? https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:427: return std::find_if(audio_source_list.begin(), audio_source_list.end(), Maybe it's not that important, but I think a for loop would be less code and a bit easier to understand as well. https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:510: auto iter = std::find_if(audio_source_list_.begin(), audio_source_list_.end(), Again, I think this code would look simpler (and possibly shorter?) with some old-fashioned for loops. https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:40: static std::unique_ptr<AudioMixerImpl> Create(int id); Don't you need the old Create function as well? https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_source_with_mix_status.cc (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_source_with_mix_status.cc:28: return IsMixed(); I think this is pretty confusing, I think WasMixed should be removed if it does the same thing as IsMixed. https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_source_with_mix_status.h (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_source_with_mix_status.h:19: class AudioSourceWithMixStatus { This class seems a little complicated to me. How about making is_mixed a public member and removing all the getters and setters? I think it may also make sense to move this class into the audio_mixer_impl.h file, since that's the only place it's used. https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_source_with_mix_status.h:39: MixerAudioSource* audio_source_; I think this can be MixerAudioSource const *
https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_source_with_mix_status.h (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_source_with_mix_status.h:19: class AudioSourceWithMixStatus { I'm thinking about this was_mixed and is_mixed business and can't help but find it unnecessary. What if you instead just have a gain setting per mixer source (you need the internal mixer source struct to store this of course). Your mixer loop figures out the current gain for each source (always 1.0 for "anonymous") and performs fade in/out while it is mixing to the output buffer (delta = (gain_current - gain_prev)/128, there's not even a need to know which way it is ramping), finally latching in the target gain.
Patchset #4 (id:50001) has been deleted
https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_defines.h (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_defines.h:49: virtual ~MixerAudioSource(); On 2016/10/04 20:36:09, the sun wrote: > - Remove ctor. > - Make the dtor inline and move to top of class declaration (among the methods). I don't understand what you mean by moving the dtor to the top. Do you want to make it public? I think a protected virtual destructor helps documenting that something is a pure virtual base class. https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:395: AudioFrameList AudioMixerImpl::GetAnonymousAudio() { On 2016/10/04 20:36:09, the sun wrote: > Why do we need to treat "anonymous" audio differently? > > As I see it, "always included in mix" is an attribute of the source (or at least > the mixer's internal representation of a source - meta data) and this attribute > can be taken into account when selecting the set of streams to mix. Keeping a > separate "additional_audio_source_list_" seems like making things more > complicated than they need to be. Good point! The latest plan seems to be to remove anonymous audio altogether. I think that's better to do in another CL. https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:427: return std::find_if(audio_source_list.begin(), audio_source_list.end(), On 2016/10/04 20:39:29, ivoc wrote: > Maybe it's not that important, but I think a for loop would be less code and a > bit easier to understand as well. Probably. The loop contains about as many characters. Lambdas in C++ add lots of syntactical overhead... https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:506: AudioSourceWithMixStatus* AudioMixerImpl::GetSourceWithStatus( On 2016/10/04 20:36:09, the sun wrote: > If this is only used for unit testing, can you mark it as such? > > Just a sanity check - the unit test is not running multiple threads, is it? I don't understand, how should I mark it? The unit tests do not run multiple threads. https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:510: auto iter = std::find_if(audio_source_list_.begin(), audio_source_list_.end(), On 2016/10/04 20:39:29, ivoc wrote: > Again, I think this code would look simpler (and possibly shorter?) with some > old-fashioned for loops. I tried and it was indeed shorter :) https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:40: static std::unique_ptr<AudioMixerImpl> Create(int id); On 2016/10/04 20:39:29, ivoc wrote: > Don't you need the old Create function as well? There are two Create functions defined in the impl.cc file: std::unique_ptr<AudioMixer> AudioMixer::Create std::unique_ptr<AudioMixerImpl> AudioMixerImpl::Create Both are currently needed, but the latest redesign plan will move the first one somewhere else and rename it into something like std::unique_ptr<AudioMixer> Something::CreateTop3Mixer https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:59: bool GetAudioSourceMixabilityStatus(MixerAudioSource* participant); On 2016/10/04 20:36:10, the sun wrote: > Source or participant? Make thy mind up! :) Done. https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:132: FRIEND_TEST_ALL_PREFIXES(AudioMixerTest, Create); On 2016/10/04 20:36:10, the sun wrote: > Can we test without creating a circular relationship between the mixer impl and > the test cases? Please? MixerImpl::Create is public now. When we split the mixer build targets into API + implementation, we can make the tests depend on the implementation. Then it should be possible to get a gn check warning if some other target includes audio_mixer_impl. https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_source_with_mix_status.cc (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_source_with_mix_status.cc:28: return IsMixed(); On 2016/10/04 20:39:29, ivoc wrote: > I think this is pretty confusing, I think WasMixed should be removed if it does > the same thing as IsMixed. ivoc@, ossu@ and solenberg@ make that three against one. My current plan is to change SourceWithStatus into a simple struct with no getters/setters inside AudioMixerImpl. https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_source_with_mix_status.h (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_source_with_mix_status.h:19: class AudioSourceWithMixStatus { That is a nice simplification! How about this plan: I change this class into a struct and move it in to AudioMixerImpl in this CL. Then I add gain to this struct, make the required changes mixing behavior (it will be lots of changes again, I think) and maybe remove anonymous participants in the next CL? https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_source_with_mix_status.h:24: AudioSourceWithMixStatus(const AudioSourceWithMixStatus&) = default; On 2016/10/04 20:36:10, the sun wrote: > Uhm, really? Who's owning the MixerAudioSource? I don't really like bare > pointers... I think you need a copy constructor to put this class in STL containers: vector copies its contents when it is resized. https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_source_with_mix_status.h:39: MixerAudioSource* audio_source_; On 2016/10/04 20:39:29, ivoc wrote: > I think this can be MixerAudioSource const * You are right, thanks! https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_source_with_mix_status.h:40: bool is_mixed_ = false; On 2016/10/04 20:36:10, the sun wrote: > RTC_DISALLOW_IMPLICIT_CONSTRUCTORS Let's just simplify it to a struct with source, mixed flag and later gain? https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:378: EXPECT_TRUE(mixer->GetAudioSourceMixabilityStatus(&participants[i])) On 2016/10/04 20:36:10, the sun wrote: > This method is public on the mixer? Why then the need for the friend > declarations? I wanted to make MixerImpl::Create private. Instead we can leave GetAudioSourceMixabilityStatus and Create public and skip friends.
Patchset #5 (id:90001) has been deleted
Patchset #5 (id:110001) has been deleted
https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_source_with_mix_status.h (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_source_with_mix_status.h:19: class AudioSourceWithMixStatus { On 2016/10/05 15:18:19, aleloi wrote: > That is a nice simplification! How about this plan: I change this class into a > struct and move it in to AudioMixerImpl in this CL. Then I add gain to this > struct, make the required changes mixing behavior (it will be lots of changes > again, I think) and maybe remove anonymous participants in the next CL? Sounds good. https://codereview.webrtc.org/2396483002/diff/130001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.h (right): https://codereview.webrtc.org/2396483002/diff/130001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.h:21: #include "webrtc/modules/audio_mixer/audio_mixer.h" The order of the includes is wrong here.
https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_defines.h (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_defines.h:49: virtual ~MixerAudioSource(); On 2016/10/05 15:18:18, aleloi wrote: > On 2016/10/04 20:36:09, the sun wrote: > > - Remove ctor. > > - Make the dtor inline and move to top of class declaration (among the > methods). > > I don't understand what you mean by moving the dtor to the top. Do you want to > make it public? I think a protected virtual destructor helps documenting that > something is a pure virtual base class. A protected virtual destructor means that the object cannot be deleted using a MixerAudioSource pointer. I don't think it is up to us to decide here how someone implementing the interface manages objects, so yes, make the dtor public and virtual. But thanks for pushing back - the AudioMixer dtor should indeed be protected, but only once the interface inherits RefCountInterface. The reason of course being that it should only be deleted from Release(). https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:506: AudioSourceWithMixStatus* AudioMixerImpl::GetSourceWithStatus( On 2016/10/05 15:18:19, aleloi wrote: > On 2016/10/04 20:36:09, the sun wrote: > > If this is only used for unit testing, can you mark it as such? > > > > Just a sanity check - the unit test is not running multiple threads, is it? > I don't understand, how should I mark it? Well, you could either add a comment in the .h, or name it ...ForTest(), for example. > The unit tests do not run multiple threads. Good! The locking had me wondering... https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:132: FRIEND_TEST_ALL_PREFIXES(AudioMixerTest, Create); On 2016/10/05 15:18:19, aleloi wrote: > On 2016/10/04 20:36:10, the sun wrote: > > Can we test without creating a circular relationship between the mixer impl > and > > the test cases? Please? > > MixerImpl::Create is public now. When we split the mixer build targets into API > + implementation, we can make the tests depend on the implementation. Then it > should be possible to get a gn check warning if some other target includes > audio_mixer_impl. I don't understand. Why can't we make the test depend on the impl already now? Declaring a test as a friend of the class being tested tells me something is wrong... https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_source_with_mix_status.h (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_source_with_mix_status.h:19: class AudioSourceWithMixStatus { On 2016/10/05 15:18:19, aleloi wrote: > That is a nice simplification! How about this plan: I change this class into a > struct and move it in to AudioMixerImpl in this CL. Then I add gain to this > struct, make the required changes mixing behavior (it will be lots of changes > again, I think) and maybe remove anonymous participants in the next CL? sgtm https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_source_with_mix_status.h:40: bool is_mixed_ = false; On 2016/10/05 15:18:19, aleloi wrote: > On 2016/10/04 20:36:10, the sun wrote: > > RTC_DISALLOW_IMPLICIT_CONSTRUCTORS > > Let's just simplify it to a struct with source, mixed flag and later gain? ok! https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:378: EXPECT_TRUE(mixer->GetAudioSourceMixabilityStatus(&participants[i])) On 2016/10/05 15:18:19, aleloi wrote: > On 2016/10/04 20:36:10, the sun wrote: > > This method is public on the mixer? Why then the need for the friend > > declarations? > > I wanted to make MixerImpl::Create private. Instead we can leave > GetAudioSourceMixabilityStatus and Create public and skip friends. Please do. Two public methods is a better choice than tightly coupling the test to the inner life of the class.
Was there a rebase mistake? It seems all of a sudden a lot of new stuff is going on. I thought this CL would come before making AudioReceiveStream an AudioMixer::Source. https://codereview.webrtc.org/2396483002/diff/130001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2396483002/diff/130001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:35: Source* audio_source_; Default-init these members https://codereview.webrtc.org/2396483002/diff/130001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:37: float gain_; hm? I thought you were going to do the gain thing in the next CL?
Patchset #4 (id:70001) has been deleted
Patchset #4 (id:130001) has been deleted
Patchset #4 (id:150001) has been deleted
Patchset #4 (id:170001) has been deleted
Hi! Yesterday, I put the new gain functionality and moved MixerAudioSource to AudioMixer::Source in this CL. That was a mistake. I have moved those to another CL. https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_defines.h (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_defines.h:49: virtual ~MixerAudioSource(); On 2016/10/05 19:32:10, the sun wrote: > On 2016/10/05 15:18:18, aleloi wrote: > > On 2016/10/04 20:36:09, the sun wrote: > > > - Remove ctor. > > > - Make the dtor inline and move to top of class declaration (among the > > methods). > > > > I don't understand what you mean by moving the dtor to the top. Do you want to > > make it public? I think a protected virtual destructor helps documenting that > > something is a pure virtual base class. > > A protected virtual destructor means that the object cannot be deleted using a > MixerAudioSource pointer. I don't think it is up to us to decide here how > someone implementing the interface manages objects, so yes, make the dtor public > and virtual. > > But thanks for pushing back - the AudioMixer dtor should indeed be protected, > but only once the interface inherits RefCountInterface. The reason of course > being that it should only be deleted from Release(). I see. Good point! https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:506: AudioSourceWithMixStatus* AudioMixerImpl::GetSourceWithStatus( On 2016/10/05 19:32:10, the sun wrote: > On 2016/10/05 15:18:19, aleloi wrote: > > On 2016/10/04 20:36:09, the sun wrote: > > > If this is only used for unit testing, can you mark it as such? > > > > > > Just a sanity check - the unit test is not running multiple threads, is it? > > I don't understand, how should I mark it? > > Well, you could either add a comment in the .h, or name it ...ForTest(), for > example. > > > The unit tests do not run multiple threads. > > Good! The locking had me wondering... I looked around in the webrtc source. ..ForTest/ForTests is used in lots of other places, so I added that. https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:132: FRIEND_TEST_ALL_PREFIXES(AudioMixerTest, Create); On 2016/10/05 19:32:11, the sun wrote: > On 2016/10/05 15:18:19, aleloi wrote: > > On 2016/10/04 20:36:10, the sun wrote: > > > Can we test without creating a circular relationship between the mixer impl > > and > > > the test cases? Please? > > > > MixerImpl::Create is public now. When we split the mixer build targets into > API > > + implementation, we can make the tests depend on the implementation. Then it > > should be possible to get a gn check warning if some other target includes > > audio_mixer_impl. > > I don't understand. Why can't we make the test depend on the impl already now? > > Declaring a test as a friend of the class being tested tells me something is > wrong... That was confusing, sorry. We can depend only on the implementation now (which we do in the patch that removes 'friend'). When we split the build target into api + impl, that dependency can be done more explicit and the build system will issue warnings when someone includes the impl.h file.
Thanks, that is much better! :) https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/BUILD.gn (right): https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/BUILD.gn:36: "../../base:gtest_prod", not needed? https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer.gypi (right): https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer.gypi:18: '<(webrtc_root)/base/base.gyp:gtest_prod', remove https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_defines.h (right): https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_defines.h:47: virtual ~MixerAudioSource() = default; nit: it is common to have the ctor/dtor as the first methods in the class (though I'm not sure if there's a style guide for that) https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:17: #include "webrtc/base/logging.h" nit: belongs in different CL? I don't see you doing anything with logging https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:61: AudioSourceWithMixStatus* audio_source_; Provide default values, since you haven't removed the default ctors/op= https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:427: bool AudioMixerImpl::IsAudioSourceInList( Note, this type of function, which does not touch any internal state, does not belong in the class. Move it to an anonymous name space at the beginning of the .cc. https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:455: std::find_if(audio_source_list->begin(), audio_source_list->end(), Looks repeated from IsAudioSourceInList(), although this time with a lambda. Can you make one function which returns an iterator and use that in both call sites? https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:18: #include "webrtc/base/gtest_prod_util.h" not needed https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_source_with_mix_status.h (right): https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_source_with_mix_status.h:40: bool is_mixed_ = false; RTC_DISALLOW_IMPLICIT_CONSTRUCTORS ?
https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:17: #include "webrtc/base/logging.h" On 2016/10/06 09:50:31, the sun wrote: > nit: belongs in different CL? I don't see you doing anything with logging I do in the new methods GetSourceWithStatusForTest and GetAudioSourceMixabilityStatusForTest. https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:61: AudioSourceWithMixStatus* audio_source_; On 2016/10/06 09:50:31, the sun wrote: > Provide default values, since you haven't removed the default ctors/op= Done. https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:427: bool AudioMixerImpl::IsAudioSourceInList( On 2016/10/06 09:50:31, the sun wrote: > Note, this type of function, which does not touch any internal state, does not > belong in the class. Move it to an anonymous name space at the beginning of the > .cc. Done. https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:455: std::find_if(audio_source_list->begin(), audio_source_list->end(), On 2016/10/06 09:50:31, the sun wrote: > Looks repeated from IsAudioSourceInList(), although this time with a lambda. Can > you make one function which returns an iterator and use that in both call sites? Turns out I still end up with two identical versions: one returning ::iterator and one returning vector<...>::const_iterator. The problem is in vector::erase, which in C++11 should work with both, but needs ::iterator before C++11. Our stl version seems not to be fully converted yet and has the old signature of erase. https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_source_with_mix_status.h (right): https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_source_with_mix_status.h:40: bool is_mixed_ = false; On 2016/10/06 09:50:31, the sun wrote: > RTC_DISALLOW_IMPLICIT_CONSTRUCTORS ? We put the AudioSourceWithMixStatus in a STL container. It needs a copy constructor and an assignment operator. The default ones just copy the is_mixed flag and the audio source pointer, which is what we want, I think.
https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/BUILD.gn (right): https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/BUILD.gn:36: "../../base:gtest_prod", On 2016/10/06 09:50:31, the sun wrote: > not needed? Removed. https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer.gypi (right): https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer.gypi:18: '<(webrtc_root)/base/base.gyp:gtest_prod', On 2016/10/06 09:50:31, the sun wrote: > remove Done. https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_defines.h (right): https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_defines.h:47: virtual ~MixerAudioSource() = default; On 2016/10/06 09:50:31, the sun wrote: > nit: it is common to have the ctor/dtor as the first methods in the class > (though I'm not sure if there's a style guide for that) Done. https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2396483002/diff/190001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:18: #include "webrtc/base/gtest_prod_util.h" On 2016/10/06 09:50:31, the sun wrote: > not needed Removed.
Patchset #5 (id:210001) has been deleted
lgtm % comments https://codereview.webrtc.org/2396483002/diff/250001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_frame_manipulator.cc (right): https://codereview.webrtc.org/2396483002/diff/250001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_frame_manipulator.cc:37: // TODO(andrew): this can easily overflow. Indeed. 4 samples in a row at 2**15 is all it takes. The worst thing is that overflow is sample rate dependent. For 48k, the average must not exceed sqrt(2**32 / 480) = 2991 (~ -21dBFS). Leave a comment that this needs to be fixed (probably using floats). https://codereview.webrtc.org/2396483002/diff/250001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2396483002/diff/250001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:142: MixerAudioSourceList::iterator FindSourceInList( If I patch this CL, it happily compiles and tests without this version of the function. Remove? https://codereview.webrtc.org/2396483002/diff/250001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_source_with_mix_status.h (right): https://codereview.webrtc.org/2396483002/diff/250001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_source_with_mix_status.h:39: MixerAudioSource* audio_source_; Since you do not disallow the default ctor, initialize this to nullptr
https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_source_with_mix_status.cc (right): https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_source_with_mix_status.cc:28: return IsMixed(); On 2016/10/05 15:18:19, aleloi wrote: > On 2016/10/04 20:39:29, ivoc wrote: > > I think this is pretty confusing, I think WasMixed should be removed if it > does > > the same thing as IsMixed. > > ivoc@, ossu@ and solenberg@ make that three against one. My current plan is to > change SourceWithStatus into a simple struct with no getters/setters inside > AudioMixerImpl. Are you still planning to do this? If so, will it be in this CL or in a follow-up?
Patchset #8 (id:290001) has been deleted
Patchset #7 (id:270001) has been deleted
On 2016/10/06 20:11:00, ivoc wrote: > https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... > File webrtc/modules/audio_mixer/audio_source_with_mix_status.cc (right): > > https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... > webrtc/modules/audio_mixer/audio_source_with_mix_status.cc:28: return IsMixed(); > On 2016/10/05 15:18:19, aleloi wrote: > > On 2016/10/04 20:39:29, ivoc wrote: > > > I think this is pretty confusing, I think WasMixed should be removed if it > > does > > > the same thing as IsMixed. > > > > ivoc@, ossu@ and solenberg@ make that three against one. My current plan is to > > change SourceWithStatus into a simple struct with no getters/setters inside > > AudioMixerImpl. > > Are you still planning to do this? If so, will it be in this CL or in a > follow-up? Next one, see https://codereview.webrtc.org/2396803004/
https://codereview.webrtc.org/2396483002/diff/250001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2396483002/diff/250001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:142: MixerAudioSourceList::iterator FindSourceInList( On 2016/10/06 18:23:17, the sun wrote: > If I patch this CL, it happily compiles and tests without this version of the > function. Remove? Maybe it only happens on linux? I get this error message when I remove the non-const version: ../../webrtc/modules/audio_mixer/audio_mixer_impl.cc:464:24: error: no matching member function for call to 'erase' audio_source_list->erase(iter); ~~~~~~~~~~~~~~~~~~~^~~~~ ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/debug/vector:479:7: note: candidate function not viable: no known conversion from 'const _Safe_iterator<__normal_iterator<const webrtc::AudioSourceWithMixStatus *, [...]>, [...]>' to '_Safe_iterator<__normal_iterator<pointer, [...]>, [...]>' for 1st argument erase(iterator __position) ^ ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/debug/vector:489:7: note: candidate function not viable: requires 2 arguments, but 1 was provided erase(iterator __first, iterator __last) https://codereview.webrtc.org/2396483002/diff/250001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_source_with_mix_status.h (right): https://codereview.webrtc.org/2396483002/diff/250001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_source_with_mix_status.h:39: MixerAudioSource* audio_source_; On 2016/10/06 18:23:17, the sun wrote: > Since you do not disallow the default ctor, initialize this to nullptr Done.
Patchset #7 (id:310001) has been deleted
On 2016/10/07 09:23:11, aleloi wrote: > On 2016/10/06 20:11:00, ivoc wrote: > > > https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... > > File webrtc/modules/audio_mixer/audio_source_with_mix_status.cc (right): > > > > > https://codereview.webrtc.org/2396483002/diff/40001/webrtc/modules/audio_mixe... > > webrtc/modules/audio_mixer/audio_source_with_mix_status.cc:28: return > IsMixed(); > > On 2016/10/05 15:18:19, aleloi wrote: > > > On 2016/10/04 20:39:29, ivoc wrote: > > > > I think this is pretty confusing, I think WasMixed should be removed if it > > > does > > > > the same thing as IsMixed. > > > > > > ivoc@, ossu@ and solenberg@ make that three against one. My current plan is > to > > > change SourceWithStatus into a simple struct with no getters/setters inside > > > AudioMixerImpl. > > > > Are you still planning to do this? If so, will it be in this CL or in a > > follow-up? > > Next one, see https://codereview.webrtc.org/2396803004/ Ok, in that case: lgtm, good job on the improvements.
Patchset #7 (id:330001) has been deleted
lgtm https://codereview.webrtc.org/2396483002/diff/250001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2396483002/diff/250001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:142: MixerAudioSourceList::iterator FindSourceInList( On 2016/10/07 09:23:28, aleloi wrote: > On 2016/10/06 18:23:17, the sun wrote: > > If I patch this CL, it happily compiles and tests without this version of the > > function. Remove? > > Maybe it only happens on linux? I get this error message when I remove the > non-const version: > > ../../webrtc/modules/audio_mixer/audio_mixer_impl.cc:464:24: error: no matching > member function for call to 'erase' > audio_source_list->erase(iter); > ~~~~~~~~~~~~~~~~~~~^~~~~ > ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/debug/vector:479:7: > note: candidate function not viable: no known conversion from 'const > _Safe_iterator<__normal_iterator<const webrtc::AudioSourceWithMixStatus *, > [...]>, [...]>' to '_Safe_iterator<__normal_iterator<pointer, [...]>, [...]>' > for 1st argument > erase(iterator __position) > ^ > ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/debug/vector:489:7: > note: candidate function not viable: requires 2 arguments, but 1 was provided > erase(iterator __first, iterator __last) Strange. Leave a comment to remove this once our linux libs are fully C++11 compliant.
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/2396483002/#ps350001 (title: "Removing dependency patch set.")
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 ========== Made MixerAudioSource a pure interface. This required quite a few small changes in the mixing algorithm structure, the mixer interface and the mixer unit tests. BUG=webrtc:6346 ========== to ========== Made MixerAudioSource a pure interface. This required quite a few small changes in the mixing algorithm structure, the mixer interface and the mixer unit tests. BUG=webrtc:6346 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:350001)
Message was sent while issue was closed.
Description was changed from ========== Made MixerAudioSource a pure interface. This required quite a few small changes in the mixing algorithm structure, the mixer interface and the mixer unit tests. BUG=webrtc:6346 ========== to ========== Made MixerAudioSource a pure interface. This required quite a few small changes in the mixing algorithm structure, the mixer interface and the mixer unit tests. BUG=webrtc:6346 Committed: https://crrev.com/2ae5fdff86b784545cbd724de54bb5ffedde1adf Cr-Commit-Position: refs/heads/master@{#14567} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/2ae5fdff86b784545cbd724de54bb5ffedde1adf Cr-Commit-Position: refs/heads/master@{#14567}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:350001) has been created in https://codereview.webrtc.org/2394253003/ by aleloi@webrtc.org. The reason for reverting is: breaks chromium FYI. |
