Issue 1721273002: MediaStream audio object graph untangling and clean-ups.

miu

Patchset #1 (id:1) has been deleted

4 years, 10 months ago (2016-02-23 05:35:49 UTC) #1

miu

miu@chromium.org changed reviewers: + mcasas@chromium.org, olka@chromium.org

4 years, 10 months ago (2016-02-23 05:38:06 UTC) #2

miu

mcasas/olka: PTAL. Please see the change description for this to make sense. ;-)

4 years, 10 months ago (2016-02-23 05:38:06 UTC) #3

mcasas

a few comments to get going. https://codereview.chromium.org/1721273002/diff/20001/content/public/renderer/media_stream_api.cc File content/public/renderer/media_stream_api.cc (right): https://codereview.chromium.org/1721273002/diff/20001/content/public/renderer/media_stream_api.cc#newcode47 content/public/renderer/media_stream_api.cc:47: const blink::WebString track_id ...

4 years, 10 months ago (2016-02-26 01:28:19 UTC) #4

miu

Thanks, mcasas. PTAL. o1ka, comments, or all lgty? https://codereview.chromium.org/1721273002/diff/20001/content/public/renderer/media_stream_api.cc File content/public/renderer/media_stream_api.cc (right): https://codereview.chromium.org/1721273002/diff/20001/content/public/renderer/media_stream_api.cc#newcode47 content/public/renderer/media_stream_api.cc:47: const ...

4 years, 10 months ago (2016-02-27 03:46:37 UTC) #5

Thanks, mcasas.  PTAL.  o1ka, comments, or all lgty?

https://codereview.chromium.org/1721273002/diff/20001/content/public/renderer...
File content/public/renderer/media_stream_api.cc (right):

https://codereview.chromium.org/1721273002/diff/20001/content/public/renderer...
content/public/renderer/media_stream_api.cc:47: const blink::WebString track_id
= MakeTrackId();
On 2016/02/26 01:28:19, mcasas wrote:
> Seems like a WebMediaStreamSource::id, which is not published
> to JS, is only used for comparison [1], so I'd recommend using
> base::GenerateGUID() [2]:
> 
> const blink::WebString track_id = 
>     blink::WebString::fromUTF8(base::GenerateGUID());
> 
> and remove MakeTrackId().
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Sounds good to me.  Done.

https://codereview.chromium.org/1721273002/diff/20001/content/renderer/media/...
File content/renderer/media/media_stream_audio_track.h (right):

https://codereview.chromium.org/1721273002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_audio_track.h:68: std::vector<base::Closure>
stop_callbacks_;
On 2016/02/26 01:28:19, mcasas wrote:
> Use base::ObserverList ? Meh could be worse
> since you'd need to make those implement an
> Observer interface ISO just a Closure...
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...

Took a look.  Yeah, I just have a dirt-simple case where observers are notified
exactly once (at Stop() time).  So, I left as-is.

https://codereview.chromium.org/1721273002/diff/20001/content/renderer/media/...
File content/renderer/media/webaudio_capturer_source.h (right):

https://codereview.chromium.org/1721273002/diff/20001/content/renderer/media/...
content/renderer/media/webaudio_capturer_source.h:61: WebRtcLocalAudioTrack*
track_;
On 2016/02/26 01:28:19, mcasas wrote:
> Shouldn't this guy be a WeakPtr?

No.  It must remain alive until Stop() is called (which sets it to nullptr). 
See comment for Start().

That said, this will go away in an upcoming change, where the relationship
between sources and tracks is made consistent for all sources and tracks.  ;-)

https://codereview.chromium.org/1721273002/diff/20001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc (right):

https://codereview.chromium.org/1721273002/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:50:
signaling_thread_checker_.DetachFromThread();
On 2016/02/26 01:28:19, mcasas wrote:
> DCHECK(signaling_thread_);
> (this should not happen but probably some
> or another unit tests will roll with it by 
> skipping some functionality, in that case
> probs the Create() method in l.34 should
> add an
> #if !defined(NDEBUG)
>   if(!signalling_thread.get())
>     signaling_thread = base::ThreadTaskRunnerHandle::Get();
> #endif
> WDYT?

I actually have some threading fixes to this class that I'll be introducing in
my next CL.  ;-)  Basically, the AddSink() and RemoveSink() methods are
currently called on the WebRTC signaling thread, but all other MediaStream code
adds and removes sinks/tracks/whatever on the main thread.  So, my next CL will
make things more consistent.

https://codereview.chromium.org/1721273002/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:75:
scoped_refptr<MediaStreamAudioLevelCalculator::Level> level) {
On 2016/02/26 01:28:19, mcasas wrote:
> Paranoia nit:
> 
> DCHECK(capture_thread_.CalledOnValidThread() ||
>        signaling_thread_checker_.CalledOnValidThread());
> 
> ?
> (Here and in l.134) ?

Actually, it's not called from either of those threads; it's called from the
main thread.  It really does not matter what thread calls this since it is just
set once before the audio flow begins and then left alone forever after.

https://codereview.chromium.org/1721273002/diff/20001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h (right):

https://codereview.chromium.org/1721273002/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h:92:
scoped_refptr<MediaStreamAudioProcessor> audio_processor_;
On 2016/02/26 01:28:19, mcasas wrote:
> This variable is only set in SetAudioProcessor() and
> then read in GetAudioProcessor() (providing a cast
> in between) and the latter is called in
> third_party/webrtc/api/statscollector_unittest.cc
> and 
> third_party/webrtc/api/statscollector.cc
> which is a code smell...

Acknowledged.

https://codereview.chromium.org/1721273002/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h:95:
std::vector<int> voe_channels_;
Removed this since it's not used at all.

https://codereview.chromium.org/1721273002/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h:101: //
Thread-safe accessor to current audio signal level.
On 2016/02/26 01:28:19, mcasas wrote:
> If this is thread-safe, we don't need to protect it with
> the |lock_| in l.109 right? Rename it if so to e.g.
> |thread_safe_level_calculator_|.

Done.  Actually, that's an interesting point.  If this refptr starts out as
null, and is only assigned once, it is completely safe to mutate it and read it
without a lock.

I also noticed, with a little more clean-up in WebRtcAudioCapturer, I was able
to do the same for |audio_processor_|.  So, now we don't need a lock for that.

...and now |lock_| isn't needed at all anymore.  So, I removed that.

https://codereview.chromium.org/1721273002/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h:105:
base::ThreadChecker signaling_thread_checker_;
On 2016/02/26 01:28:19, mcasas wrote:
> We don't need |signaling_thread_checker_| is we have
> |signaling_thread_| on which we can call 
> BelongsToCurrentThread() and  we assume is not nullptr 
> on ctor (needs at least a DCHECK btw).

Done.  Removed both since the latter is only used in one place (and is detached
there!).

https://codereview.chromium.org/1721273002/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h:108: //
Protects |voe_channels_|, |audio_processor_| and |signal_level_|.
On 2016/02/26 01:28:19, mcasas wrote:
> |signal_level_| is no more.

Done.  |lock_| is no more too (see above comment).  :)

https://codereview.chromium.org/1721273002/diff/20001/content/renderer/media/...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/1721273002/diff/20001/content/renderer/media/...
content/renderer/media/webrtc_audio_capturer.cc:30: bool HasDataEnergy(const
media::AudioBus& audio_source) {
On 2016/02/26 01:28:19, mcasas wrote:
> Isn't this method exactly a !audio_source.AreFramesZero()?
> Suggest inlining it with the said !...
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_b...

Done.

https://codereview.chromium.org/1721273002/diff/20001/content/renderer/media/...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/1721273002/diff/20001/content/renderer/media/...
content/renderer/media/webrtc_audio_capturer.h:91: media::AudioParameters
GetInputFormat() const;
On 2016/02/26 01:28:19, mcasas wrote:
> One thing that surprised me of audio processing
> vs video is that the parameters of the audio stream
> are passed partially in a preamble (as AudioParameters)
> and partially with the AudioBus (inside AudioBus), and
> there's still an overarching AudioBuffer that transports
> all necessary data with each blob of audio, but a copy
> is needed from AudioBus to AudioBuffer. Is AudioBuffer
> the future? 

I've never used AudioBuffer.  It's sole purpose seems to be to copy audio data
once, hold it for some time, and then copy it again.  That seems like an
anti-pattern to me, but perhaps there's a rationale I'm missing.

<rant>IMO, the purpose of copying data should be to share data, and only when it
would otherwise be impractical to guarantee the original data remains unchanged
while being shared.  Fast machines have allowed developers to become lazy w/ all
the copying and re-copying...</rant>

I'd rather see AudioBus become a little more like VideoFrame, where the audio
format is available as metadata alongside the audio samples.  Then, perhaps we
could all agree to use AudioBus everywhere and kill AudioBuffer and other
"solutions."

o1ka

I've got some questions. (Sorry for the delay, it took me a while to get ...

4 years, 9 months ago (2016-02-29 14:28:05 UTC) #6

miu

Thanks for taking a look. Responded to your comments and re-worked the Stop() mechanism a ...

4 years, 9 months ago (2016-03-01 09:43:55 UTC) #7

Thanks for taking a look.  Responded to your comments and re-worked the Stop()
mechanism a bit.  PTAL.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_audio_track.cc:22: Stop();
On 2016/02/29 14:28:04, o1ka wrote:
> If you add a method of a child class as a stop observer, wouldn't you call
that
> method on an already-deleted child object in Stop() here?
> 
> For example, these lines from WebRtcLocalAudioTrack bother me:
>   MediaStreamAudioTrack::AddStopObserver(base::Bind(
>        &WebRtcLocalAudioTrack::RemoveAllSinks, base::Unretained(this)));

Yes, you're right.  I had mitigated that by having the child class' dtor call
Stop().

However, I've looked at this again, and I see now that I over-used this
"pattern."  It should not be used for calling back into the MSAudioTrack
subclasses themselves.  So, I put back the Stop() method in the subclasses and
removed the base::Closures that would have them to refer to themselves.

The stop observers are meant for other classes (the capturers) to be notified
that the track is stopping.  Therefore, now I just require MSAudioTrack
subclasses to implement Stop(), and they are required to call the
WillStopTrack() method along with their other "stop" tasks.  SGTY?

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc/media_stream_remote_audio_track.cc (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_remote_audio_track.cc:126:
MediaStreamAudioTrack::AddStopObserver(base::Bind(
On 2016/02/29 14:28:04, o1ka wrote:
> See comments for WebRtcLocalAudioTrack

Done.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_remote_audio_track.cc:136: // members
of this class are destroyed.
On 2016/02/29 14:28:04, o1ka wrote:
> See comments for WebRtcLocalAudioTrack

Done.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:49:
signaling_task_runner_(signaling_task_runner) {}
On 2016/02/29 14:28:04, o1ka wrote:
> How thread safety of the class is guaranteed if signaling_task_runner is NULL,
> and what is the use case for that?

Many of the unit tests run single-threaded.  See lines 27-34 above.

tommi's TODO (line 24) would be the way to resolve this problem.  As much as I'd
like to fix everything, this CL is already too big.  ;)

For now, I've added a stricter check in Create() and also log a warning when we
think we're running in a single-threaded unit test.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:63: // protect
|audio_processor_| with a mutex.
On 2016/02/29 14:28:04, o1ka wrote:
> Is there any guarantee that there won't be a race between two
> SetAudioProcessor() calls?

No, but the header comments and the DCHECK on lines 64-65 are meant to ensure
the method is only called once.  So, if SetAudioProcessor() is called a second
time, that's a bug since it's explicitly documented that should not be done.

> And how calling it only once guarantees that it does not race with
> GetAudioProcessor()?

Two ways: 1) This method is called during initialization, before the signaling
thread from WebRTC would start calling GetAudioProcessor().  So, there is no
possibility for concurrent access.  2) Even if there were concurrent access,
assigning to a scoped_refptr<> only once guarantees the signaling thread will
only ever see either nullptr or a non-null pointer that never changes.  Either
way, the signaling thread will do the right thing.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:72: //
|level_| with a mutex.
On 2016/02/29 14:28:04, o1ka wrote:
> Same questions as for SetAudioProcessor()

Good questions.  Answered in other comment.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:85: if
(signaling_task_runner_.get() &&
On 2016/02/29 14:28:04, o1ka wrote:
> nit: if (signaling_task_runner_ && ...)

Done.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:94: return
webrtc::MediaStreamTrack<webrtc::AudioTrackInterface>::
On 2016/02/29 14:28:04, o1ka wrote:
> will be called from a random thread  if signaling_task_runner_ is NULL (see my
> question in the constructor)

Explained above.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:140: if
(!level_)
On 2016/02/29 14:28:04, o1ka wrote:
> How does it guarantee that there is no race?

Answered in comment above.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_capturer.cc:471:
DCHECK(audio_processor_->InputFormat().IsValid());
On 2016/02/29 14:28:04, o1ka wrote:
> I got lost in thread safety of |audio_processor_| here. Should we either
assure
> that we are on the valid thread, or use the lock to access InputFormat(), as
it
> is done in other places?

Agree it's confusing.  Here's how I understand it:

1. The call to audio_processor_->OnCaptureFormatChanged() is what would mutate
the value InputFormat() reads.  This OnCaptureFromatChanged() call only occurs
on the main thread AND only at a time when we know the audio thread will not be
in the middle of executing the code in Capture().  (We know the audio thread
will not be calling Capture() because the "source" is stopped.)

2. The locking is needed in GetInputFormat() because it's the WebRTC signaling
thread attempting to read from something that could be mutated at any time on
the main thread.

So, because of #1, locking shouldn't be needed here at these lines of code.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_capturer.cc:480:
audio_processor_->OutputFormat();
On 2016/02/29 14:28:04, o1ka wrote:
> Should we be on the valid thread, see Line 427?

No.  This is another case of #1 (in my last comment).

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_device_impl.cc (left):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_device_impl.cc:467: // Use the last
|capturer| which is from the latest getUserMedia call as
On 2016/02/29 14:28:04, o1ka wrote:
> The information on what is considered to be the default capturer got lost with
> the change. I would at least place it into the header comment for |capturers_|

Done.  BTW--It's never been clear to me what a "default capturer" actually is
and/or why we always choose the last capturer as the one and only one libjingle
"sees."  To speculate, I think this was just a hacky way of dealing with the
fact that WebRtcAudioDeviceImpl is a singleton instance whereas the
WebRtcAudioCapturers are not.  This should be fixed, but it'll probably involve
some major refactoring work in libjingle code.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_device_impl.h (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_device_impl.h:311: // Capturers must remain
valid until RemoveAudioCapturer() is called.
On 2016/02/29 14:28:04, o1ka wrote:
> How will we prevent bugs cause by violation of this contract? What is the
> benefit of this change?

The risk of that is non-existent here because the interaction is simple:
WebRtcAudioCapturer registeres itself with WebRtcAudioDeviceImpl when it is
constructed (in Initialize()), and de-registers itself when it is stopped
(before destruction).  Therefore, the pointer that WebRtcAudioDeviceImpl has is
never invalid.

I may not be understanding your question entirely, but it's worth noting that
ref-counting is not a mechanism we should use to guarantee contracts like this. 
The Chromium project does numerous things in the waterfall tests (vtable
clobbering at destruction, ASAN test runs, etc.) to confirm we're not
accidentally accessing an object after it has become invalid.

As for benefits:

1. The Chromium project is always trying to eliminate the use of ref-counting as
much as possible.  See
https://www.chromium.org/developers/smart-pointer-guidelines (search for
"Ref-counted objects" for technical discussion).

2. By removing the ref-counting here, I've made it clear which is the one object
that owns a WebRtcAudioCapturer (the MediaStreamAudioSource) and therefore it is
also clear how long the object will remain alive and when the object will be
torn-down.  In my next CL, I'll leverage this to cleanly factor-out the reusable
components from the WebRTC-specific ones.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_local_audio_track.cc:28:
&WebRtcLocalAudioTrack::RemoveAllSinks, base::Unretained(this)));
On 2016/02/29 14:28:05, o1ka wrote:
> See the comment to MediaStreamAudioTrack destructor regarding
> base::Unretained(this)

Done.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_local_audio_track.cc:40: Stop();
On 2016/02/29 14:28:05, o1ka wrote:
> That's why this pattern with stop observers and calling Stop() in
> MediaStreamAudioTrack destructor does not look like a safe and clean solution.

Done.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_local_audio_track.h (left):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_local_audio_track.h:40: // TODO(tommi): Rename to
MediaStreamLocalAudioTrack.
On 2016/02/29 14:28:05, o1ka wrote:
> Can we indeed make MediaStreamLocalAudioTrack and WebRtcLocalAudioTrack to
have
> some matching names?

I'll be addressing this is my next CL.  (BTW, for reference, see:
https://codereview.chromium.org/1647773002/ for the approximate "end state" I
want to achieve with this series of CLs.).

tl;dr: I'll be moving WebRtcLocalAudioTrack into the webrtc/ subdir, and moving
its non-WebRTC-specific functionality into MediaStreamAudioTrack (parent class).

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_local_audio_track.h (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_local_audio_track.h:102: mutable base::Lock lock_;
On 2016/02/29 14:28:05, o1ka wrote:
> I know it's not your change and probably I am paranoid but I would not use one
> lock to protect two unrelated entities, even if the scope of locking is small:
> you never know what you end up with in a year or two.

Totally agree.  But, this is yet another thing that will be fixed w/ the next
CL; so I'd prefer not to do any redundant work here.  :)

o1ka

Thank you for clarifications! Some more questions. https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/media_stream_audio_track.cc File content/renderer/media/media_stream_audio_track.cc (right): https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/media_stream_audio_track.cc#newcode22 content/renderer/media/media_stream_audio_track.cc:22: Stop(); On ...

4 years, 9 months ago (2016-03-01 14:18:59 UTC) #8

Thank you for clarifications!
Some more questions.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_audio_track.cc:22: Stop();
On 2016/03/01 09:43:54, miu wrote:
> On 2016/02/29 14:28:04, o1ka wrote:
> > If you add a method of a child class as a stop observer, wouldn't you call
> that
> > method on an already-deleted child object in Stop() here?
> > 
> > For example, these lines from WebRtcLocalAudioTrack bother me:
> >   MediaStreamAudioTrack::AddStopObserver(base::Bind(
> >        &WebRtcLocalAudioTrack::RemoveAllSinks, base::Unretained(this)));
> 
> Yes, you're right.  I had mitigated that by having the child class' dtor call
> Stop().
> 
> However, I've looked at this again, and I see now that I over-used this
> "pattern."  It should not be used for calling back into the MSAudioTrack
> subclasses themselves.  So, I put back the Stop() method in the subclasses and
> removed the base::Closures that would have them to refer to themselves.
> 
> The stop observers are meant for other classes (the capturers) to be notified
> that the track is stopping.  Therefore, now I just require MSAudioTrack
> subclasses to implement Stop(), and they are required to call the
> WillStopTrack() method along with their other "stop" tasks.  SGTY?
Though I had the same idea initially, the more I look at it, the more I feel
uncomfortable bout the fact that it's up to child classes to call
WillStopTrack() whenever they think reasonable (see my comment in
MediaStreamRemoteAudioTrack). It means the stop observers can't be sure when to
expect those calls. So, I would probably do an opposite thing: introduce a
protected OnStop() for children to implement, leave Stop() to be final in
MediaStreamTrack and make it call OnStop() after(before?) notifying stop
observers (but without calling Stop() in ~MediaStreamTrack()). And if you also
state that child classes must call Stop() in their destructors, it means clients
do not need to care of mandatory call to Stop(). What do you think?

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:49:
signaling_task_runner_(signaling_task_runner) {}
On 2016/03/01 09:43:54, miu wrote:
> On 2016/02/29 14:28:04, o1ka wrote:
> > How thread safety of the class is guaranteed if signaling_task_runner is
NULL,
> > and what is the use case for that?
> 
> Many of the unit tests run single-threaded.  See lines 27-34 above.
> 
> tommi's TODO (line 24) would be the way to resolve this problem.  As much as
I'd
> like to fix everything, this CL is already too big.  ;)
> 
> For now, I've added a stricter check in Create() and also log a warning when
we
> think we're running in a single-threaded unit test.

Acknowledged.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:63: // protect
|audio_processor_| with a mutex.
On 2016/03/01 09:43:55, miu wrote:
> On 2016/02/29 14:28:04, o1ka wrote:
> > Is there any guarantee that there won't be a race between two
> > SetAudioProcessor() calls?
> 
> No, but the header comments and the DCHECK on lines 64-65 are meant to ensure
> the method is only called once.  So, if SetAudioProcessor() is called a second
> time, that's a bug since it's explicitly documented that should not be done.
Agree that calling SetAudioProcessor() second time is a bug. There is still a
tiny chance that DCHECK won't catch it because of a race. Also, the code may
evolve in a way that this race can become less obvious and lead to
harder-to-catch bugs.
> 
> > And how calling it only once guarantees that it does not race with
> > GetAudioProcessor()?
> 
> Two ways: 1) This method is called during initialization, before the signaling
> thread from WebRTC would start calling GetAudioProcessor().  So, there is no
> possibility for concurrent access.  2) Even if there were concurrent access,
> assigning to a scoped_refptr<> only once guarantees the signaling thread will
> only ever see either nullptr or a non-null pointer that never changes.  Either
> way, the signaling thread will do the right thing.
I think (1) worth documenting in the header. As far as I understand,
scope_refptr operations are not atomic, so (2) is not guaranteed without (1). If
I'm not missing something and this is the case, the comments should be like
"|audio_processor_| must only be set once and _before_ any call to
GetAudioProcessor(). (And now the question is how to ensure the correct usage.
My paranoia tells me to introduce an atomic "initialized" flag. You never know
how the code itself and its usage are going to evolve. Probably it's an overkill
though.)

Also it would be good to have a header comment like "The variables below are not
protected by locks because... Take this into account when messing up with this
code" :).

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:72: //
|level_| with a mutex.
On 2016/03/01 09:43:55, miu wrote:
> On 2016/02/29 14:28:04, o1ka wrote:
> > Same questions as for SetAudioProcessor()
> 
> Good questions.  Answered in other comment.

Same as above: looks like just "setting it once" is not enough to guarantee
thread safety.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:85: if
(signaling_task_runner_.get() &&
On 2016/03/01 09:43:54, miu wrote:
> On 2016/02/29 14:28:04, o1ka wrote:
> > nit: if (signaling_task_runner_ && ...)
> 
> Done.

Acknowledged.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:94: return
webrtc::MediaStreamTrack<webrtc::AudioTrackInterface>::
On 2016/03/01 09:43:55, miu wrote:
> On 2016/02/29 14:28:04, o1ka wrote:
> > will be called from a random thread  if signaling_task_runner_ is NULL (see
my
> > question in the constructor)
> 
> Explained above.

Acknowledged.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:140: if
(!level_)
On 2016/03/01 09:43:54, miu wrote:
> On 2016/02/29 14:28:04, o1ka wrote:
> > How does it guarantee that there is no race?
> 
> Answered in comment above.

Same as above.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_capturer.cc:471:
DCHECK(audio_processor_->InputFormat().IsValid());
On 2016/03/01 09:43:55, miu wrote:
> On 2016/02/29 14:28:04, o1ka wrote:
> > I got lost in thread safety of |audio_processor_| here. Should we either
> assure
> > that we are on the valid thread, or use the lock to access InputFormat(), as
> it
> > is done in other places?
> 
> Agree it's confusing.  Here's how I understand it:
> 
> 1. The call to audio_processor_->OnCaptureFormatChanged() is what would mutate
> the value InputFormat() reads.  This OnCaptureFromatChanged() call only occurs
> on the main thread AND only at a time when we know the audio thread will not
be
> in the middle of executing the code in Capture().  (We know the audio thread
> will not be calling Capture() because the "source" is stopped.)
> 
> 2. The locking is needed in GetInputFormat() because it's the WebRTC signaling
> thread attempting to read from something that could be mutated at any time on
> the main thread.
> 
> So, because of #1, locking shouldn't be needed here at these lines of code.
> 

Great! Can we have this in the comment? It would be invaluable for unprepared
readers like me. (Though I've got a feeling it can be refactored to be more
clear and straightforward at some point, but I guess it's out of scope of this
change)

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_capturer.cc:480:
audio_processor_->OutputFormat();
On 2016/03/01 09:43:55, miu wrote:
> On 2016/02/29 14:28:04, o1ka wrote:
> > Should we be on the valid thread, see Line 427?
> 
> No.  This is another case of #1 (in my last comment).

Could you make a comment on this? Would be very helpful.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_device_impl.cc (left):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_device_impl.cc:467: // Use the last
|capturer| which is from the latest getUserMedia call as
On 2016/03/01 09:43:55, miu wrote:
> On 2016/02/29 14:28:04, o1ka wrote:
> > The information on what is considered to be the default capturer got lost
with
> > the change. I would at least place it into the header comment for
|capturers_|
> 
> Done.  BTW--It's never been clear to me what a "default capturer" actually is
> and/or why we always choose the last capturer as the one and only one
libjingle
> "sees."  To speculate, I think this was just a hacky way of dealing with the
> fact that WebRtcAudioDeviceImpl is a singleton instance whereas the
> WebRtcAudioCapturers are not.  This should be fixed, but it'll probably
involve
> some major refactoring work in libjingle code.
Agree. I have no idea on what "default capturer" is as well :)

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_device_impl.h (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_device_impl.h:311: // Capturers must remain
valid until RemoveAudioCapturer() is called.
On 2016/03/01 09:43:55, miu wrote:
> On 2016/02/29 14:28:04, o1ka wrote:
> > How will we prevent bugs cause by violation of this contract? What is the
> > benefit of this change?
> 
> The risk of that is non-existent here because the interaction is simple:
> WebRtcAudioCapturer registeres itself with WebRtcAudioDeviceImpl when it is
> constructed (in Initialize()), and de-registers itself when it is stopped
> (before destruction).  Therefore, the pointer that WebRtcAudioDeviceImpl has
is
> never invalid.
> 
> I may not be understanding your question entirely, but it's worth noting that
> ref-counting is not a mechanism we should use to guarantee contracts like
this. 
> The Chromium project does numerous things in the waterfall tests (vtable
> clobbering at destruction, ASAN test runs, etc.) to confirm we're not
> accidentally accessing an object after it has become invalid.
> 
> As for benefits:
> 
> 1. The Chromium project is always trying to eliminate the use of ref-counting
as
> much as possible.  See
> https://www.chromium.org/developers/smart-pointer-guidelines (search for
> "Ref-counted objects" for technical discussion).
> 
> 2. By removing the ref-counting here, I've made it clear which is the one
object
> that owns a WebRtcAudioCapturer (the MediaStreamAudioSource) and therefore it
is
> also clear how long the object will remain alive and when the object will be
> torn-down.  In my next CL, I'll leverage this to cleanly factor-out the
reusable
> components from the WebRTC-specific ones.
Thanks a lot for the detailed reply - it answers exactly the question I asked
and is very helpful (a very nice article you referenced, it clarified some
things for me - thank you!).

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_local_audio_track.cc:28:
&WebRtcLocalAudioTrack::RemoveAllSinks, base::Unretained(this)));
On 2016/02/29 14:28:05, o1ka wrote:
> See the comment to MediaStreamAudioTrack destructor regarding
> base::Unretained(this)

Acknowledged.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_local_audio_track.cc:40: Stop();
On 2016/02/29 14:28:05, o1ka wrote:
> That's why this pattern with stop observers and calling Stop() in
> MediaStreamAudioTrack destructor does not look like a safe and clean solution.

Acknowledged.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_local_audio_track.h (left):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_local_audio_track.h:40: // TODO(tommi): Rename to
MediaStreamLocalAudioTrack.
On 2016/03/01 09:43:55, miu wrote:
> On 2016/02/29 14:28:05, o1ka wrote:
> > Can we indeed make MediaStreamLocalAudioTrack and WebRtcLocalAudioTrack to
> have
> > some matching names?
> 
> I'll be addressing this is my next CL.  (BTW, for reference, see:
> https://codereview.chromium.org/1647773002/ for the approximate "end state" I
> want to achieve with this series of CLs.).
> 
> tl;dr: I'll be moving WebRtcLocalAudioTrack into the webrtc/ subdir, and
moving
> its non-WebRTC-specific functionality into MediaStreamAudioTrack (parent
class).

Acknowledged.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_local_audio_track.h (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_local_audio_track.h:102: mutable base::Lock lock_;
On 2016/03/01 09:43:55, miu wrote:
> On 2016/02/29 14:28:05, o1ka wrote:
> > I know it's not your change and probably I am paranoid but I would not use
one
> > lock to protect two unrelated entities, even if the scope of locking is
small:
> > you never know what you end up with in a year or two.
> 
> Totally agree.  But, this is yet another thing that will be fixed w/ the next
> CL; so I'd prefer not to do any redundant work here.  :)

Acknowledged.

https://codereview.chromium.org/1721273002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_track.h (right):

https://codereview.chromium.org/1721273002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_track.h:70: void WillStopTrack();
NotifyStopObservers() maybe?

https://codereview.chromium.org/1721273002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_track.h:73: // These are run after
this track has been stopped.  Even if Stop() is called
The comment needs some makeover now, probably something like "Run by
WillStopTrack() once, even if WillStopTrack() is called multiple times.."

https://codereview.chromium.org/1721273002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/media_stream_remote_audio_track.cc (right):

https://codereview.chromium.org/1721273002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_remote_audio_track.cc:146:
MediaStreamAudioTrack::WillStopTrack();
What if some observers rely on the fact that the callbacks are run before/after
the track has actually stopped (next lines)? Since observers subscribe in the
parent class, we probably should explicitly say there what is the expected
sequence of Stop()/WillStopTrack() actions (stop first, notify second, or the
opposite) and make the child classes to adhere to it, so that observers know
what to expect regardless of implementation the parent class is instantiated
with. What do you think?

miu

o1ka: Thanks. Addressed your 2nd round comments. PTAL. https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/media_stream_audio_track.cc File content/renderer/media/media_stream_audio_track.cc (right): https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/media_stream_audio_track.cc#newcode22 content/renderer/media/media_stream_audio_track.cc:22: Stop(); ...

4 years, 9 months ago (2016-03-02 01:12:51 UTC) #9

o1ka: Thanks.  Addressed your 2nd round comments.  PTAL.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_audio_track.cc:22: Stop();
On 2016/03/01 14:18:58, o1ka wrote:
> On 2016/03/01 09:43:54, miu wrote:
> > On 2016/02/29 14:28:04, o1ka wrote:
> > > If you add a method of a child class as a stop observer, wouldn't you call
> > that
> > > method on an already-deleted child object in Stop() here?
> > > 
> > > For example, these lines from WebRtcLocalAudioTrack bother me:
> > >   MediaStreamAudioTrack::AddStopObserver(base::Bind(
> > >        &WebRtcLocalAudioTrack::RemoveAllSinks, base::Unretained(this)));
> > 
> > Yes, you're right.  I had mitigated that by having the child class' dtor
call
> > Stop().
> > 
> > However, I've looked at this again, and I see now that I over-used this
> > "pattern."  It should not be used for calling back into the MSAudioTrack
> > subclasses themselves.  So, I put back the Stop() method in the subclasses
and
> > removed the base::Closures that would have them to refer to themselves.
> > 
> > The stop observers are meant for other classes (the capturers) to be
notified
> > that the track is stopping.  Therefore, now I just require MSAudioTrack
> > subclasses to implement Stop(), and they are required to call the
> > WillStopTrack() method along with their other "stop" tasks.  SGTY?
> Though I had the same idea initially, the more I look at it, the more I feel
> uncomfortable bout the fact that it's up to child classes to call
> WillStopTrack() whenever they think reasonable (see my comment in
> MediaStreamRemoteAudioTrack). It means the stop observers can't be sure when
to
> expect those calls. So, I would probably do an opposite thing: introduce a
> protected OnStop() for children to implement, leave Stop() to be final in
> MediaStreamTrack and make it call OnStop() after(before?) notifying stop
> observers (but without calling Stop() in ~MediaStreamTrack()). And if you also
> state that child classes must call Stop() in their destructors, it means
clients
> do not need to care of mandatory call to Stop(). What do you think?

Done.  I also re-evaluated why I needed to add multiple stop observers.  After
looking over all the code (including future changes), it turns out I really
don't need multiple stop observers.  So, there was more room for simplifying
things.

What I really needed was a way for a track to be able to halt the audio flow
from the capturer when its Stop() method is called.  So, I've explicitly added a
Start() method which takes a single callback.  The callback's sole purpose is to
halt the audio flow when the track requires it.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:63: // protect
|audio_processor_| with a mutex.
On 2016/03/01 14:18:59, o1ka wrote:
> Agree that calling SetAudioProcessor() second time is a bug. There is still a
> tiny chance that DCHECK won't catch it because of a race. Also, the code may
> evolve in a way that this race can become less obvious and lead to
> harder-to-catch bugs.

Where is the race condition?  I don't see what else should be done here.  If
someone in the future decides to change the code in a way that violates the
contract documented both in the header file and here, they had better make the
appropriate changes necessary to ensure thread safety.  I can't anticipate every
possible change that *might* be made.  ;-)

> I think (1) worth documenting in the header. As far as I understand,
> scope_refptr operations are not atomic, so (2) is not guaranteed without (1).
If
> I'm not missing something and this is the case, the comments should be like
> "|audio_processor_| must only be set once and _before_ any call to
> GetAudioProcessor(). (And now the question is how to ensure the correct usage.

Done.  Good idea.

> My paranoia tells me to introduce an atomic "initialized" flag. You never know
> how the code itself and its usage are going to evolve. Probably it's an
overkill
> though.)

I probably should never have de-railed the discussion with point #2.  ;-) 
Basically, it's irrelevant here because we don't need to worry about concurrent
reads and mutations.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:72: //
|level_| with a mutex.
On 2016/03/01 14:18:58, o1ka wrote:
> On 2016/03/01 09:43:55, miu wrote:
> > On 2016/02/29 14:28:04, o1ka wrote:
> > > Same questions as for SetAudioProcessor()
> > 
> > Good questions.  Answered in other comment.
> 
> Same as above: looks like just "setting it once" is not enough to guarantee
> thread safety.

I removed these comments since I think they were making it sound like
|audio_processor_| and |level_| were being accessed concurrently.  Part of
that's my fault, since I was in the frame-of-mind of having just removed the
unnecessary Lock around them.

IMO, since the header file comments explain when the setters and getters must be
called, that should be enough.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_capturer.cc:471:
DCHECK(audio_processor_->InputFormat().IsValid());
On 2016/03/01 14:18:59, o1ka wrote:
> On 2016/03/01 09:43:55, miu wrote:
> > On 2016/02/29 14:28:04, o1ka wrote:
> > > I got lost in thread safety of |audio_processor_| here. Should we either
> > assure
> > > that we are on the valid thread, or use the lock to access InputFormat(),
as
> > it
> > > is done in other places?
> > 
> > Agree it's confusing.  Here's how I understand it:
> > 
> > 1. The call to audio_processor_->OnCaptureFormatChanged() is what would
mutate
> > the value InputFormat() reads.  This OnCaptureFromatChanged() call only
occurs
> > on the main thread AND only at a time when we know the audio thread will not
> be
> > in the middle of executing the code in Capture().  (We know the audio thread
> > will not be calling Capture() because the "source" is stopped.)
> > 
> > 2. The locking is needed in GetInputFormat() because it's the WebRTC
signaling
> > thread attempting to read from something that could be mutated at any time
on
> > the main thread.
> > 
> > So, because of #1, locking shouldn't be needed here at these lines of code.
> > 
> 
> Great! Can we have this in the comment? It would be invaluable for unprepared
> readers like me. (Though I've got a feeling it can be refactored to be more
> clear and straightforward at some point, but I guess it's out of scope of this
> change)

Done.  And, yes my upcoming changes will clear this up a lot!  :)

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_capturer.cc:480:
audio_processor_->OutputFormat();
On 2016/03/01 14:18:59, o1ka wrote:
> On 2016/03/01 09:43:55, miu wrote:
> > On 2016/02/29 14:28:04, o1ka wrote:
> > > Should we be on the valid thread, see Line 427?
> > 
> > No.  This is another case of #1 (in my last comment).
> 
> Could you make a comment on this? Would be very helpful.

Done.  I also "grouped" this code with the DCHECKs above since they're all
related in this same way.

https://codereview.chromium.org/1721273002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_track.h (right):

https://codereview.chromium.org/1721273002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_track.h:70: void WillStopTrack();
On 2016/03/01 14:18:59, o1ka wrote:
> NotifyStopObservers() maybe?

Acknowledged.  This got re-worked, as explained in other comments.

https://codereview.chromium.org/1721273002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_track.h:73: // These are run after
this track has been stopped.  Even if Stop() is called
On 2016/03/01 14:18:59, o1ka wrote:
> The comment needs some makeover now, probably something like "Run by
> WillStopTrack() once, even if WillStopTrack() is called multiple times.."

Acknowledged.  This also got re-worked, as explained in other comments.

https://codereview.chromium.org/1721273002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/media_stream_remote_audio_track.cc (right):

https://codereview.chromium.org/1721273002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_remote_audio_track.cc:146:
MediaStreamAudioTrack::WillStopTrack();
On 2016/03/01 14:18:59, o1ka wrote:
> What if some observers rely on the fact that the callbacks are run
before/after
> the track has actually stopped (next lines)? Since observers subscribe in the
> parent class, we probably should explicitly say there what is the expected
> sequence of Stop()/WillStopTrack() actions (stop first, notify second, or the
> opposite) and make the child classes to adhere to it, so that observers know
> what to expect regardless of implementation the parent class is instantiated
> with. What do you think?

Good point, and I made the behavior more precisely defined in the latest patch
set.  No more observers: The point is for the audio flow to be immediately
halted, and then let the subclasses perform any additional steps as needed.

https://codereview.chromium.org/1721273002/diff/80001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.h (right):

https://codereview.chromium.org/1721273002/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:45:
base::WeakPtr<MediaStreamAudioSource> GetWeakPtr() {
Note: These really shouldn't be exposed publicly, but I'll be able to easily
make these GetWeakPtr() and StopAudioDeliveryTo() methods private in my next CL.

o1ka

Thank you, looks good, a couple of minor points. tommi@, could you take a look? ...

4 years, 9 months ago (2016-03-02 16:31:14 UTC) #11

Thank you, looks good, a couple of minor points.
tommi@, could you take a look?

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_audio_track.cc:22: Stop();
On 2016/03/02 01:12:50, miu wrote:
> On 2016/03/01 14:18:58, o1ka wrote:
> > On 2016/03/01 09:43:54, miu wrote:
> > > On 2016/02/29 14:28:04, o1ka wrote:
> > > > If you add a method of a child class as a stop observer, wouldn't you
call
> > > that
> > > > method on an already-deleted child object in Stop() here?
> > > > 
> > > > For example, these lines from WebRtcLocalAudioTrack bother me:
> > > >   MediaStreamAudioTrack::AddStopObserver(base::Bind(
> > > >        &WebRtcLocalAudioTrack::RemoveAllSinks, base::Unretained(this)));
> > > 
> > > Yes, you're right.  I had mitigated that by having the child class' dtor
> call
> > > Stop().
> > > 
> > > However, I've looked at this again, and I see now that I over-used this
> > > "pattern."  It should not be used for calling back into the MSAudioTrack
> > > subclasses themselves.  So, I put back the Stop() method in the subclasses
> and
> > > removed the base::Closures that would have them to refer to themselves.
> > > 
> > > The stop observers are meant for other classes (the capturers) to be
> notified
> > > that the track is stopping.  Therefore, now I just require MSAudioTrack
> > > subclasses to implement Stop(), and they are required to call the
> > > WillStopTrack() method along with their other "stop" tasks.  SGTY?
> > Though I had the same idea initially, the more I look at it, the more I feel
> > uncomfortable bout the fact that it's up to child classes to call
> > WillStopTrack() whenever they think reasonable (see my comment in
> > MediaStreamRemoteAudioTrack). It means the stop observers can't be sure when
> to
> > expect those calls. So, I would probably do an opposite thing: introduce a
> > protected OnStop() for children to implement, leave Stop() to be final in
> > MediaStreamTrack and make it call OnStop() after(before?) notifying stop
> > observers (but without calling Stop() in ~MediaStreamTrack()). And if you
also
> > state that child classes must call Stop() in their destructors, it means
> clients
> > do not need to care of mandatory call to Stop(). What do you think?
> 
> Done.  I also re-evaluated why I needed to add multiple stop observers.  After
> looking over all the code (including future changes), it turns out I really
> don't need multiple stop observers.  So, there was more room for simplifying
> things.
> 
> What I really needed was a way for a track to be able to halt the audio flow
> from the capturer when its Stop() method is called.  So, I've explicitly added
a
> Start() method which takes a single callback.  The callback's sole purpose is
to
> halt the audio flow when the track requires it.

Looks good!

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:63: // protect
|audio_processor_| with a mutex.
On 2016/03/02 01:12:50, miu wrote:
> On 2016/03/01 14:18:59, o1ka wrote:
> > Agree that calling SetAudioProcessor() second time is a bug. There is still
a
> > tiny chance that DCHECK won't catch it because of a race. Also, the code may
> > evolve in a way that this race can become less obvious and lead to
> > harder-to-catch bugs.
> 
> Where is the race condition?  I don't see what else should be done here.  If
> someone in the future decides to change the code in a way that violates the
> contract documented both in the header file and here, they had better make the
> appropriate changes necessary to ensure thread safety.  I can't anticipate
every
> possible change that *might* be made.  ;-)
> 
> > I think (1) worth documenting in the header. As far as I understand,
> > scope_refptr operations are not atomic, so (2) is not guaranteed without
(1).
> If
> > I'm not missing something and this is the case, the comments should be like
> > "|audio_processor_| must only be set once and _before_ any call to
> > GetAudioProcessor(). (And now the question is how to ensure the correct
usage.
> 
> Done.  Good idea.
> 
> > My paranoia tells me to introduce an atomic "initialized" flag. You never
know
> > how the code itself and its usage are going to evolve. Probably it's an
> overkill
> > though.)
> 
> I probably should never have de-railed the discussion with point #2.  ;-) 
> Basically, it's irrelevant here because we don't need to worry about
concurrent
> reads and mutations.

Acknowledged.

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/1721273002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_capturer.cc:471:
DCHECK(audio_processor_->InputFormat().IsValid());
On 2016/03/02 01:12:50, miu wrote:
> On 2016/03/01 14:18:59, o1ka wrote:
> > On 2016/03/01 09:43:55, miu wrote:
> > > On 2016/02/29 14:28:04, o1ka wrote:
> > > > I got lost in thread safety of |audio_processor_| here. Should we either
> > > assure
> > > > that we are on the valid thread, or use the lock to access
InputFormat(),
> as
> > > it
> > > > is done in other places?
> > > 
> > > Agree it's confusing.  Here's how I understand it:
> > > 
> > > 1. The call to audio_processor_->OnCaptureFormatChanged() is what would
> mutate
> > > the value InputFormat() reads.  This OnCaptureFromatChanged() call only
> occurs
> > > on the main thread AND only at a time when we know the audio thread will
not
> > be
> > > in the middle of executing the code in Capture().  (We know the audio
thread
> > > will not be calling Capture() because the "source" is stopped.)
> > > 
> > > 2. The locking is needed in GetInputFormat() because it's the WebRTC
> signaling
> > > thread attempting to read from something that could be mutated at any time
> on
> > > the main thread.
> > > 
> > > So, because of #1, locking shouldn't be needed here at these lines of
code.
> > > 
> > 
> > Great! Can we have this in the comment? It would be invaluable for
unprepared
> > readers like me. (Though I've got a feeling it can be refactored to be more
> > clear and straightforward at some point, but I guess it's out of scope of
this
> > change)
> 
> Done.  And, yes my upcoming changes will clear this up a lot!  :)

Great,thanks!

https://codereview.chromium.org/1721273002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/media_stream_remote_audio_track.cc (right):

https://codereview.chromium.org/1721273002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_remote_audio_track.cc:146:
MediaStreamAudioTrack::WillStopTrack();
On 2016/03/02 01:12:50, miu wrote:
> On 2016/03/01 14:18:59, o1ka wrote:
> > What if some observers rely on the fact that the callbacks are run
> before/after
> > the track has actually stopped (next lines)? Since observers subscribe in
the
> > parent class, we probably should explicitly say there what is the expected
> > sequence of Stop()/WillStopTrack() actions (stop first, notify second, or
the
> > opposite) and make the child classes to adhere to it, so that observers know
> > what to expect regardless of implementation the parent class is instantiated
> > with. What do you think?
> 
> Good point, and I made the behavior more precisely defined in the latest patch
> set.  No more observers: The point is for the audio flow to be immediately
> halted, and then let the subclasses perform any additional steps as needed.

I like it!

https://codereview.chromium.org/1721273002/diff/80001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.h (right):

https://codereview.chromium.org/1721273002/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:57: audio_capturer_ =
std::move(capturer);
It does not look quite in place here. Isn't it the caller's responsibility to
call std::move() (And you actually do it in
peer_connection_dependency_factory.cc:206)?

https://codereview.chromium.org/1721273002/diff/80001/content/renderer/media/...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/1721273002/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_audio_track.cc:40: stop_callback_ =
stop_callback;
Do you intentionally allow passing null as stop_callback?

https://codereview.chromium.org/1721273002/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_audio_track.cc:47: stop_callback_.Run();
Do you want to reset the callback probably?

https://codereview.chromium.org/1721273002/diff/80001/content/renderer/media/...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/1721273002/diff/80001/content/renderer/media/...
content/renderer/media/webrtc_local_audio_track.cc:142: sinks_.Clear();
Not your change, but probably RetrieveAndClearTags() as at line 51?

miu

https://codereview.chromium.org/1721273002/diff/80001/content/renderer/media/media_stream_audio_source.h File content/renderer/media/media_stream_audio_source.h (right): https://codereview.chromium.org/1721273002/diff/80001/content/renderer/media/media_stream_audio_source.h#newcode57 content/renderer/media/media_stream_audio_source.h:57: audio_capturer_ = std::move(capturer); On 2016/03/02 16:31:14, o1ka wrote: > ...

4 years, 9 months ago (2016-03-02 23:38:10 UTC) #12

tommi (sloooow) - chröme

looks great! looking forward to getting this in (and thanks for the thorough reviews olga) ...

4 years, 9 months ago (2016-03-03 11:07:31 UTC) #13

miu

Thanks, tommi. PTAL. https://codereview.chromium.org/1721273002/diff/140001/content/public/renderer/media_stream_audio_sink.cc File content/public/renderer/media_stream_audio_sink.cc (right): https://codereview.chromium.org/1721273002/diff/140001/content/public/renderer/media_stream_audio_sink.cc#newcode18 content/public/renderer/media_stream_audio_sink.cc:18: if (MediaStreamAudioTrack* native_track = MediaStreamAudioTrack::From(track)) On ...

4 years, 9 months ago (2016-03-05 02:55:31 UTC) #14

Thanks, tommi.  PTAL.

https://codereview.chromium.org/1721273002/diff/140001/content/public/rendere...
File content/public/renderer/media_stream_audio_sink.cc (right):

https://codereview.chromium.org/1721273002/diff/140001/content/public/rendere...
content/public/renderer/media_stream_audio_sink.cc:18: if
(MediaStreamAudioTrack* native_track = MediaStreamAudioTrack::From(track))
On 2016/03/03 11:07:30, tommi-chromium wrote:
> is the if() necessary?  It seems like we would previously crash in both
release
> and debug builds if we'd ever have a problem here but now we silently ignore
it
> in release builds (official anyway).

Ah, I did a little too much here perhaps.  Changed these to just add DCHECKs.

https://codereview.chromium.org/1721273002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_audio_level_calculator.h (right):

https://codereview.chromium.org/1721273002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_audio_level_calculator.h:49:
scoped_refptr<Level> level() const { return level_; }
On 2016/03/03 11:07:30, tommi-chromium wrote:
> return const & and avoid extra AddRef/Release?

Done.

https://codereview.chromium.org/1721273002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/1721273002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_audio_source.cc:70: void
MediaStreamAudioSource::StopAudioDeliveryTo(MediaStreamAudioTrack* track) {
On 2016/03/03 11:07:30, tommi-chromium wrote:
> DCHECK(track)?

Done.

https://codereview.chromium.org/1721273002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_audio_source.h (right):

https://codereview.chromium.org/1721273002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_audio_source.h:64: void
SetLocalAudioSource(scoped_refptr<webrtc::AudioSourceInterface> source) {
On 2016/03/03 11:07:30, tommi-chromium wrote:
> just checking - the intent here is to take ownership of a reference, as in,
the
> caller should use std::move()?  If not I think this should be const&

Done (using std::move() here and in other places throughout this change, to
avoid additional Add/RemoveRef() calls).

As for the argument being const&, I've been going by the recently updated
guidelines:
https://www.chromium.org/developers/coding-style#TOC-Object-ownership-and-cal...

The new guidelines are based on this group discussion:
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/Passin...

https://codereview.chromium.org/1721273002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_audio_source.h:65: local_audio_source_ =
source;
On 2016/03/03 11:07:30, tommi-chromium wrote:
> and if not passing by value is intentional, I think we should use std::move
> here.

Done.

https://codereview.chromium.org/1721273002/diff/140001/content/renderer/media...
File content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc (right):

https://codereview.chromium.org/1721273002/diff/140001/content/renderer/media...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:72: level_ =
level;
On 2016/03/03 11:07:31, tommi-chromium wrote:
> assuming |level| took ref ownership, you probably want to use std::move here

Done.

https://codereview.chromium.org/1721273002/diff/140001/content/renderer/media...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:144: 0.5f /*
rounding to nearest int */);
On 2016/03/03 11:07:31, tommi-chromium wrote:
> Double checking on the behavior...
> 
> I see you're maintaining the 'assume non-zero audio' behavior where audio
energy
> is calculated post processing.  That's great and it solved a situation where
we
> could frequently get false positives.
> 
> Here, if |signal_level| is 1.0f/max() (smallest possible non-zero value), will
> it be rounded to 0 or 1?
> 
> I'm assuming that before it would have been 1, but I might be mistaken.

Yes, it will round to 1.  Here's a test program I wrote to double-check this:

#include <stdlib.h>
#include <iostream>
#include <limits>

int main(int argc, char* argv[]) {
  double signal_level = 1.0 / std::numeric_limits<int16_t>::max();
  int level =
      static_cast<int>(signal_level * std::numeric_limits<int16_t>::max() +
                       0.5f /* rounding to nearest int */);

  std::cout << "Result: " << level << std::endl;

  return 0;
}

Output is:

$ g++ test.cc
$ ./a.out
Result: 1

https://codereview.chromium.org/1721273002/diff/140001/content/renderer/media...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/1721273002/diff/140001/content/renderer/media...
content/renderer/media/webrtc_audio_device_impl.cc:294:
capturers_.back()->SetVolume(volume);
On 2016/03/03 11:07:31, tommi-chromium wrote:
> I'm not quite following what's going to happen if there are ever more than one
> capturers.  Why do we need to support many?

Great question!  Both o1ka and I both have no idea on the history behind this. 
I speculated about it in an earlier e-mail on this code review.  tl;dr: It's
probably a hack, since WebRtcAudioDeviceImpl is a singleton instance and the
interface it implements (from libjingle) doesn't account for the possibility of
multiple captures from different sources.  I've seen some TODO comments in other
files that hint at this explanation.

Anyway, since I don't know exactly why it does this, I'm trying not to change
any behavior here.

https://codereview.chromium.org/1721273002/diff/140001/content/renderer/media...
content/renderer/media/webrtc_audio_device_impl.cc:478:
!device_info.device.matched_output.frames_per_buffer)
On 2016/03/03 11:07:31, tommi-chromium wrote:
> {}

Done.

https://codereview.chromium.org/1721273002/diff/140001/content/renderer/media...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/1721273002/diff/140001/content/renderer/media...
content/renderer/media/webrtc_local_audio_track.cc:21: :
MediaStreamAudioTrack(true), adapter_(adapter) {
On 2016/03/03 11:07:31, tommi-chromium wrote:
> use std::move()? (assuming |adapter| is passed by value on purpose.

Done.

miu

miu@chromium.org changed reviewers: + finnur@chromium.org, jochen@chromium.org, peter@chromium.org

4 years, 9 months ago (2016-03-05 02:58:44 UTC) #15

miu

finnur: PTAL at chrome/renderer/extensions/cast_streaming_native_handler.cc (related to content/public/renderer/media_stream_api.h changes). jochen: PTAL at content/public/renderer/* changes. peter: Need ...

4 years, 9 months ago (2016-03-05 02:58:45 UTC) #16

Finnur

Rubberstamp LGTM with one nit. https://codereview.chromium.org/1721273002/diff/160001/chrome/renderer/extensions/cast_streaming_native_handler.cc File chrome/renderer/extensions/cast_streaming_native_handler.cc (right): https://codereview.chromium.org/1721273002/diff/160001/chrome/renderer/extensions/cast_streaming_native_handler.cc#newcode873 chrome/renderer/extensions/cast_streaming_native_handler.cc:873: params.frames_per_buffer(), true /* is_remote ...

4 years, 9 months ago (2016-03-07 12:14:24 UTC) #17

o1ka

Great stuff! lgtm after clarifying my last question. https://codereview.chromium.org/1721273002/diff/80001/content/renderer/media/media_stream_audio_source.h File content/renderer/media/media_stream_audio_source.h (right): https://codereview.chromium.org/1721273002/diff/80001/content/renderer/media/media_stream_audio_source.h#newcode57 content/renderer/media/media_stream_audio_source.h:57: audio_capturer_ ...

4 years, 9 months ago (2016-03-07 16:00:28 UTC) #18

miu

https://codereview.chromium.org/1721273002/diff/160001/chrome/renderer/extensions/cast_streaming_native_handler.cc File chrome/renderer/extensions/cast_streaming_native_handler.cc (right): https://codereview.chromium.org/1721273002/diff/160001/chrome/renderer/extensions/cast_streaming_native_handler.cc#newcode873 chrome/renderer/extensions/cast_streaming_native_handler.cc:873: params.frames_per_buffer(), true /* is_remote */, On 2016/03/07 12:14:24, Finnur ...

4 years, 9 months ago (2016-03-07 20:08:52 UTC) #19

miu

Description was changed from ========== MediaStream audio object graph untangling and clean-ups. This change consists ...

4 years, 9 months ago (2016-03-09 02:30:40 UTC) #22

Description was changed from

==========
MediaStream audio object graph untangling and clean-ups.

This change consists of a number of "clean-up" changes that are being
done to make the soon-upcoming refactoring of these classes go much more
smoothly.  These are:

1. Public content MediaStreamApi functions cleaned up.  Removed
"duplicated" functions that don't really do the same thing.  Removed
hard-coded audio parameters from AddAudioTrackToMediaStream().

2. Eliminated ref-counting of WebRtcAudioCapturer and
WebAudioCaptureSource.  Removed unnecessary references to these from
WebRtcLocalAudioTrack.  Not only did this improve encapsulation of some
code, but it also allowed for the removal of several dozen lines of
"dead weight" testing set-upcode throughout the directory.

3. Renamed MediaStreamAudioTrack::GetTrack() method to From(), to be
consistent with how this pattern is used in other parts of libcontent,
and added a MediaStreamAudioSource::From().

4. Moved audio level calculations out of WebRtcLocalAudioTrack and into
WebRtcAudioCapturer.  This way, when multiple tracks are present, the
calculation is only being done once on the same audio.

5. Eliminated call to WebRtcCapturer::Stop() from
WebRtcAudioDeviceImpl::Terminate(), which are each supposed to run on
different threads.  From testing, DCHECKs should have been firing, but
weren't, so the Terminate() method seems to be dead code.

6. Miscellaneous other "compaction" and comment updates.

BUG=577881, 577874
==========

to

==========
MediaStream audio object graph untangling and clean-ups.

This change consists of a number of "clean-up" changes that are being
done to make the soon-upcoming refactoring of these classes go much more
smoothly.  These are:

1. Public content MediaStreamApi functions cleaned up.  Removed
"duplicated" functions that don't really do the same thing.  Removed
hard-coded audio parameters from AddAudioTrackToMediaStream().

2. Eliminated ref-counting of WebRtcAudioCapturer and
WebAudioCaptureSource.  Removed unnecessary references to these from
WebRtcLocalAudioTrack.  Not only did this improve encapsulation of some
code, but it also allowed for the removal of several dozen lines of
"dead weight" testing set-upcode throughout the directory.

3. Renamed MediaStreamAudioTrack::GetTrack() method to From(), to be
consistent with how this pattern is used in other parts of libcontent,
and added a MediaStreamAudioSource::From().

4. Moved audio level calculations out of WebRtcLocalAudioTrack and into
WebRtcAudioCapturer.  This way, when multiple tracks are present, the
calculation is only being done once on the same audio.

5. Eliminated call to WebRtcCapturer::Stop() from
WebRtcAudioDeviceImpl::Terminate(), which are each supposed to run on
different threads.  From testing, DCHECKs should have been firing, but
weren't, so the Terminate() method seems to be dead code.

6. Miscellaneous other "compaction" and comment updates.

BUG=577881, 577874
TBR=peter@chromium.org
==========

Henrik Grunell

grunell@chromium.org changed reviewers: + grunell@chromium.org

4 years, 9 months ago (2016-03-09 02:31:23 UTC) #23

Henrik Grunell

Awesome that you do this! Lgtm, though I haven't looked in detail in all files, ...

4 years, 9 months ago (2016-03-09 02:31:24 UTC) #24

miu

Thanks all. Bot issues in latest patchset were transient, unrelated to this change. (Note: TBR'ing ...

4 years, 9 months ago (2016-03-09 02:32:19 UTC) #26

miu

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org, olka@chromium.org, tommi@chromium.org, jochen@chromium.org ...

4 years, 9 months ago (2016-03-09 03:06:30 UTC) #28

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721273002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721273002/200001

4 years, 9 months ago (2016-03-09 03:06:43 UTC) #29

commit-bot: I haz the power

Description was changed from ========== MediaStream audio object graph untangling and clean-ups. This change consists ...

4 years, 9 months ago (2016-03-09 04:32:52 UTC) #30

Message was sent while issue was closed.

Description was changed from

==========
MediaStream audio object graph untangling and clean-ups.

This change consists of a number of "clean-up" changes that are being
done to make the soon-upcoming refactoring of these classes go much more
smoothly.  These are:

1. Public content MediaStreamApi functions cleaned up.  Removed
"duplicated" functions that don't really do the same thing.  Removed
hard-coded audio parameters from AddAudioTrackToMediaStream().

2. Eliminated ref-counting of WebRtcAudioCapturer and
WebAudioCaptureSource.  Removed unnecessary references to these from
WebRtcLocalAudioTrack.  Not only did this improve encapsulation of some
code, but it also allowed for the removal of several dozen lines of
"dead weight" testing set-upcode throughout the directory.

3. Renamed MediaStreamAudioTrack::GetTrack() method to From(), to be
consistent with how this pattern is used in other parts of libcontent,
and added a MediaStreamAudioSource::From().

4. Moved audio level calculations out of WebRtcLocalAudioTrack and into
WebRtcAudioCapturer.  This way, when multiple tracks are present, the
calculation is only being done once on the same audio.

5. Eliminated call to WebRtcCapturer::Stop() from
WebRtcAudioDeviceImpl::Terminate(), which are each supposed to run on
different threads.  From testing, DCHECKs should have been firing, but
weren't, so the Terminate() method seems to be dead code.

6. Miscellaneous other "compaction" and comment updates.

BUG=577881, 577874
TBR=peter@chromium.org
==========

to

==========
MediaStream audio object graph untangling and clean-ups.

This change consists of a number of "clean-up" changes that are being
done to make the soon-upcoming refactoring of these classes go much more
smoothly.  These are:

1. Public content MediaStreamApi functions cleaned up.  Removed
"duplicated" functions that don't really do the same thing.  Removed
hard-coded audio parameters from AddAudioTrackToMediaStream().

2. Eliminated ref-counting of WebRtcAudioCapturer and
WebAudioCaptureSource.  Removed unnecessary references to these from
WebRtcLocalAudioTrack.  Not only did this improve encapsulation of some
code, but it also allowed for the removal of several dozen lines of
"dead weight" testing set-upcode throughout the directory.

3. Renamed MediaStreamAudioTrack::GetTrack() method to From(), to be
consistent with how this pattern is used in other parts of libcontent,
and added a MediaStreamAudioSource::From().

4. Moved audio level calculations out of WebRtcLocalAudioTrack and into
WebRtcAudioCapturer.  This way, when multiple tracks are present, the
calculation is only being done once on the same audio.

5. Eliminated call to WebRtcCapturer::Stop() from
WebRtcAudioDeviceImpl::Terminate(), which are each supposed to run on
different threads.  From testing, DCHECKs should have been firing, but
weren't, so the Terminate() method seems to be dead code.

6. Miscellaneous other "compaction" and comment updates.

BUG=577881, 577874
TBR=peter@chromium.org
==========

commit-bot: I haz the power

Description was changed from ========== MediaStream audio object graph untangling and clean-ups. This change consists ...

4 years, 9 months ago (2016-03-09 04:41:35 UTC) #32

Message was sent while issue was closed.

Description was changed from

==========
MediaStream audio object graph untangling and clean-ups.

This change consists of a number of "clean-up" changes that are being
done to make the soon-upcoming refactoring of these classes go much more
smoothly.  These are:

1. Public content MediaStreamApi functions cleaned up.  Removed
"duplicated" functions that don't really do the same thing.  Removed
hard-coded audio parameters from AddAudioTrackToMediaStream().

2. Eliminated ref-counting of WebRtcAudioCapturer and
WebAudioCaptureSource.  Removed unnecessary references to these from
WebRtcLocalAudioTrack.  Not only did this improve encapsulation of some
code, but it also allowed for the removal of several dozen lines of
"dead weight" testing set-upcode throughout the directory.

3. Renamed MediaStreamAudioTrack::GetTrack() method to From(), to be
consistent with how this pattern is used in other parts of libcontent,
and added a MediaStreamAudioSource::From().

4. Moved audio level calculations out of WebRtcLocalAudioTrack and into
WebRtcAudioCapturer.  This way, when multiple tracks are present, the
calculation is only being done once on the same audio.

5. Eliminated call to WebRtcCapturer::Stop() from
WebRtcAudioDeviceImpl::Terminate(), which are each supposed to run on
different threads.  From testing, DCHECKs should have been firing, but
weren't, so the Terminate() method seems to be dead code.

6. Miscellaneous other "compaction" and comment updates.

BUG=577881, 577874
TBR=peter@chromium.org
==========

to

==========
MediaStream audio object graph untangling and clean-ups.

This change consists of a number of "clean-up" changes that are being
done to make the soon-upcoming refactoring of these classes go much more
smoothly.  These are:

1. Public content MediaStreamApi functions cleaned up.  Removed
"duplicated" functions that don't really do the same thing.  Removed
hard-coded audio parameters from AddAudioTrackToMediaStream().

2. Eliminated ref-counting of WebRtcAudioCapturer and
WebAudioCaptureSource.  Removed unnecessary references to these from
WebRtcLocalAudioTrack.  Not only did this improve encapsulation of some
code, but it also allowed for the removal of several dozen lines of
"dead weight" testing set-upcode throughout the directory.

3. Renamed MediaStreamAudioTrack::GetTrack() method to From(), to be
consistent with how this pattern is used in other parts of libcontent,
and added a MediaStreamAudioSource::From().

4. Moved audio level calculations out of WebRtcLocalAudioTrack and into
WebRtcAudioCapturer.  This way, when multiple tracks are present, the
calculation is only being done once on the same audio.

5. Eliminated call to WebRtcCapturer::Stop() from
WebRtcAudioDeviceImpl::Terminate(), which are each supposed to run on
different threads.  From testing, DCHECKs should have been firing, but
weren't, so the Terminate() method seems to be dead code.

6. Miscellaneous other "compaction" and comment updates.

BUG=577881, 577874
TBR=peter@chromium.org

Committed: https://crrev.com/26bfd80549511a7e05f23c9941c41ced104ddf28
Cr-Commit-Position: refs/heads/master@{#380065}
==========

commit-bot: I haz the power

Patchset 10 (id:??) landed as https://crrev.com/26bfd80549511a7e05f23c9941c41ced104ddf28 Cr-Commit-Position: refs/heads/master@{#380065}

4 years, 9 months ago (2016-03-09 04:41:36 UTC) #33

sof

sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com

4 years, 9 months ago (2016-03-09 08:45:23 UTC) #34

sof

A revert of this CL (patchset #10 id:200001) has been created in https://codereview.chromium.org/1780653002/ by sigbjornf@opera.com. ...

4 years, 9 months ago (2016-03-09 08:49:16 UTC) #36

Henrik Grunell

On 2016/03/09 08:45:24, sof wrote: > Were any layout tests run for this CL? Are ...

4 years, 9 months ago (2016-03-09 13:17:00 UTC) #37

sof

On 2016/03/09 13:17:00, Henrik Grunell wrote: > On 2016/03/09 08:45:24, sof wrote: > > Were ...

4 years, 9 months ago (2016-03-09 13:54:08 UTC) #38

o1ka

On 2016/03/09 13:17:00, Henrik Grunell wrote: > On 2016/03/09 08:45:24, sof wrote: > > Were ...

4 years, 9 months ago (2016-03-09 13:59:19 UTC) #39

sof

On 2016/03/09 13:59:19, o1ka wrote: > On 2016/03/09 13:17:00, Henrik Grunell wrote: > > On ...

4 years, 9 months ago (2016-03-09 14:04:10 UTC) #40

miu

4 years, 9 months ago (2016-03-09 19:33:47 UTC) #41

Message was sent while issue was closed.

Ok, looks like all the tests that broke were crashes that occurred due to a
CHECK() I added in WebRtcLocalAudioTrackAdapter::Create().  It seems there can
be a RenderThreadImpl but no PCFactory signaling thread.  I'll whip-up a retry
change.

Issue 1721273002: MediaStream audio object graph untangling and clean-ups. (Closed)

Description

Patch Set 1 : #

Patch Set 2 : Addressed mcasas's 1st round comments, plus REBASE. #

Patch Set 3 : addressed o1ka's comments #

Patch Set 4 : Addressed o1ka's 2nd round of comments. #

Patch Set 5 : REBASE again to fix compile #

Patch Set 6 : addressed o1ka's 3rd round of comments #

Patch Set 7 : Fix unit tests broken by recent MSAudioTrack::Stop() reworking. #

Patch Set 8 : Changes addressing tommi's suggestions. #

Patch Set 9 : Resolved minor nits around in-line argument comments, use of const&. #

Patch Set 10 : REBASE #

Messages