Move OnDecoderInitDone() from decoder to pipeline thread.

Move OnDecoderInitDone() from decoder to pipeline thread.

We need to ensure |sink_| access is thread safe so every sink implementation
doesn't have to worry about locking and out of order calls.

Failure to do so will lead to dead locks and shutdown crashes.

BUG=158848, 157793
TEST=media_unittests.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165854

[DaleCurtis, 2012-10-31 22:39:34]

Should resolve hangs in the field and on the bots while ensuring stability.  An
alternate solution would be to introduce a |sink_lock_| separate from |lock_|.

[DaleCurtis, 2012-11-01 00:34:16]

On 2012/10/31 22:39:34, DaleCurtis wrote:
> Should resolve hangs in the field and on the bots while ensuring stability. 
An
> alternate solution would be to introduce a |sink_lock_| separate from |lock_|.

Argh, hold off on this one, compile finally finished and playback fails with
this :/

[DaleCurtis, 2012-11-01 00:56:19]

On 2012/11/01 00:34:16, DaleCurtis wrote:
> On 2012/10/31 22:39:34, DaleCurtis wrote:
> > Should resolve hangs in the field and on the bots while ensuring stability. 
> An
> > alternate solution would be to introduce a |sink_lock_| separate from
|lock_|.
> 
> Argh, hold off on this one, compile finally finished and playback fails with
> this :/

All fixed, just missed an AutoUnlock. I've left
ScheduleRead_Locked/OnDecodedAudioReady on the decoder thread, but maybe that
one should be moved over. Let me know what you think.

[DaleCurtis, 2012-11-01 19:39:13]

scherkus to cc; a challenger appears! Thanks fischman!

[Ami GONE FROM CHROMIUM, 2012-11-01 19:57:01]

https://codereview.chromium.org/11275087/diff/3001/media/audio/null_audio_sin...
File media/audio/null_audio_sink.cc (right):

https://codereview.chromium.org/11275087/diff/3001/media/audio/null_audio_sin...
media/audio/null_audio_sink.cc:72: if (playing_)  {
This variable is set on the pipeline thread but is being read on the
NullAudioThread, with no locking.  What's up with that?

https://codereview.chromium.org/11275087/diff/3001/media/filters/audio_render...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/11275087/diff/3001/media/filters/audio_render...
media/filters/audio_renderer_impl.cc:424: if (stopped_ || GetPlaybackRate() ==
0.0f) {
Must not read stopped_ off pipeline thread.

https://codereview.chromium.org/11275087/diff/3001/media/filters/audio_render...
media/filters/audio_renderer_impl.cc:424: if (stopped_ || GetPlaybackRate() ==
0.0f) {
GetPlaybackRate() locks lock_ for its duration, but what if the playback rate is
changed immediately after this call returns?  I doubt this is correct code
(obvs., predates your CL).

https://codereview.chromium.org/11275087/diff/3001/media/filters/audio_render...
File media/filters/audio_renderer_impl.h (right):

https://codereview.chromium.org/11275087/diff/3001/media/filters/audio_render...
media/filters/audio_renderer_impl.h:201: // A flag that indicates this filter is
called to stop.
doco that this must be only read/written on pipeline_thread_

https://codereview.chromium.org/11275087/diff/3001/media/filters/audio_render...
media/filters/audio_renderer_impl.h:202: bool stopped_;
Might be worthwhile to drop stopped_ in favor of clearing sink_ after Stop()ing
it.

[DaleCurtis, 2012-11-01 22:02:59]

https://codereview.chromium.org/11275087/diff/3001/media/audio/null_audio_sin...
File media/audio/null_audio_sink.cc (right):

https://codereview.chromium.org/11275087/diff/3001/media/audio/null_audio_sin...
media/audio/null_audio_sink.cc:72: if (playing_)  {
On 2012/11/01 19:57:01, Ami Fischman wrote:
> This variable is set on the pipeline thread but is being read on the
> NullAudioThread, with no locking.  What's up with that?

Normally we don't want to lock on the audio thread for a simple play check (a
value which could change afterward anyways); for NullAudioSink it doesn't
matter, so I'll just revert this chunk since you're opposed.

https://codereview.chromium.org/11275087/diff/3001/media/filters/audio_render...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/11275087/diff/3001/media/filters/audio_render...
media/filters/audio_renderer_impl.cc:424: if (stopped_ || GetPlaybackRate() ==
0.0f) {
On 2012/11/01 19:57:01, Ami Fischman wrote:
> Must not read stopped_ off pipeline thread.

Done.

https://codereview.chromium.org/11275087/diff/3001/media/filters/audio_render...
media/filters/audio_renderer_impl.cc:424: if (stopped_ || GetPlaybackRate() ==
0.0f) {
On 2012/11/01 19:57:01, Ami Fischman wrote:
> GetPlaybackRate() locks lock_ for its duration, but what if the playback rate
is
> changed immediately after this call returns?  I doubt this is correct code
> (obvs., predates your CL).

Fixed. Read once and saved for the duration of Render().  I've also added fixed
some potential issues with removing the stopped_ flag and algorithm_ becoming
NULL in FillBuffer().

https://codereview.chromium.org/11275087/diff/3001/media/filters/audio_render...
File media/filters/audio_renderer_impl.h (right):

https://codereview.chromium.org/11275087/diff/3001/media/filters/audio_render...
media/filters/audio_renderer_impl.h:201: // A flag that indicates this filter is
called to stop.
On 2012/11/01 19:57:01, Ami Fischman wrote:
> doco that this must be only read/written on pipeline_thread_

Removed.

https://codereview.chromium.org/11275087/diff/3001/media/filters/audio_render...
media/filters/audio_renderer_impl.h:202: bool stopped_;
On 2012/11/01 19:57:01, Ami Fischman wrote:
> Might be worthwhile to drop stopped_ in favor of clearing sink_ after
Stop()ing
> it.

Done. The new patch set relies on nothing else being called after Stop() has
completed, which is true as far as I can tell since the stop_cb_ clears the
audio_renderer_ in Pipeline::OnStopCompleted() which is called from
AudioRendererImpl::Stop().

[Ami GONE FROM CHROMIUM, 2012-11-03 00:11:44]

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
media/filters/audio_renderer_impl.cc:45: callback.Run();
running the callback under lock looks strange.  Is that right?
In fact since hte callback is always run here, is it even necessary anymore? 
(possibly another CL to nuke it)

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
media/filters/audio_renderer_impl.cc:50: if (algorithm_->playback_rate() !=
0.0f) {
depending on impl detail of algorithm_ that this is safe?
could alternatively cache the last-set playbackrate in this class (under the
heading of only read/written on the pipeline thread) and avoid this smell.

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
media/filters/audio_renderer_impl.cc:230: algorithm_.reset(new
AudioRendererAlgorithm());
doesn't this need to be done under lock?

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
media/filters/audio_renderer_impl.cc:411: return algorithm_ ?
algorithm_->playback_rate() : 0;
How can this fail to be non-NULL?

Is it the case that we want to prevent |this| being used until after
OnDecoderInitDone completes?

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
media/filters/audio_renderer_impl.cc:423: if (playback_rate == 0.0f) {
Part of the point of my previous comment on this change is that it will use the
wrong rate if the rate is changed part-way through this function.  It's unclear
to me what the right answer to that is.

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
File media/filters/audio_renderer_impl.h (right):

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
media/filters/audio_renderer_impl.h:160: // audio, pipeline, and decoder threads
may dead lock.
deadlock is one word.

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
media/filters/audio_renderer_impl.h:178: // Callback provided to Pause(...).
ellipses here and below aren't useful

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
media/filters/audio_renderer_impl.h:189: // the pipeline thread.
this is a very codesmelly comment.
Why is it OK to access this concurrently under lock from one thread and not
under lock on the pipeline thread?

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
media/filters/audio_renderer_impl.h:203: // |state_| access must always be
guarded by |lock_|
redundant to l.184?

[DaleCurtis, 2012-11-03 01:32:47]

Are we there yet? :)

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
media/filters/audio_renderer_impl.cc:45: callback.Run();
On 2012/11/03 00:11:45, Ami Fischman wrote:
> running the callback under lock looks strange.  Is that right?
> In fact since hte callback is always run here, is it even necessary anymore? 
> (possibly another CL to nuke it)

Yeah, it should removed, but that's a base class change, so another day.

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
media/filters/audio_renderer_impl.cc:50: if (algorithm_->playback_rate() !=
0.0f) {
On 2012/11/03 00:11:45, Ami Fischman wrote:
> depending on impl detail of algorithm_ that this is safe?
> could alternatively cache the last-set playbackrate in this class (under the
> heading of only read/written on the pipeline thread) and avoid this smell.

Done.

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
media/filters/audio_renderer_impl.cc:230: algorithm_.reset(new
AudioRendererAlgorithm());
On 2012/11/03 00:11:45, Ami Fischman wrote:
> doesn't this need to be done under lock?

No since it's not used until Initialize() completes.

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
media/filters/audio_renderer_impl.cc:411: return algorithm_ ?
algorithm_->playback_rate() : 0;
On 2012/11/03 00:11:45, Ami Fischman wrote:
> How can this fail to be non-NULL?
> 
> Is it the case that we want to prevent |this| being used until after
> OnDecoderInitDone completes?

If Stop() happens on the pipeline thread and destroys |algorithm_| there may
still be an executing Render() callback on the audio thread which is trying to
use it.

Removed in any case.

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
media/filters/audio_renderer_impl.cc:423: if (playback_rate == 0.0f) {
On 2012/11/03 00:11:45, Ami Fischman wrote:
> Part of the point of my previous comment on this change is that it will use
the
> wrong rate if the rate is changed part-way through this function.  It's
unclear
> to me what the right answer to that is.

WHY U SO DIFFICULT! Fixed, don't blame me as the patch set grows :p

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
File media/filters/audio_renderer_impl.h (right):

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
media/filters/audio_renderer_impl.h:160: // audio, pipeline, and decoder threads
may dead lock.
On 2012/11/03 00:11:45, Ami Fischman wrote:
> deadlock is one word.

Done.

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
media/filters/audio_renderer_impl.h:178: // Callback provided to Pause(...).
On 2012/11/03 00:11:45, Ami Fischman wrote:
> ellipses here and below aren't useful

Done.

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
media/filters/audio_renderer_impl.h:189: // the pipeline thread.
On 2012/11/03 00:11:45, Ami Fischman wrote:
> this is a very codesmelly comment.
> Why is it OK to access this concurrently under lock from one thread and not
> under lock on the pipeline thread?

Removed.

https://codereview.chromium.org/11275087/diff/2003/media/filters/audio_render...
media/filters/audio_renderer_impl.h:203: // |state_| access must always be
guarded by |lock_|
On 2012/11/03 00:11:45, Ami Fischman wrote:
> redundant to l.184?

Done.

[Ami GONE FROM CHROMIUM, 2012-11-03 02:38:53]

LGTM
and by 'G' I mean "Garbage"...  I really hope you take a flamethrower to this
class some day soon.
(but I understand it won't be for a 24 merge)

[commit-bot: I haz the power, 2012-11-03 02:49:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11275087/6003

[commit-bot: I haz the power, 2012-11-03 07:07:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11275087/5003

[commit-bot: I haz the power, 2012-11-03 16:51:07]

Change committed as 165854