media: Introduce Renderer interface and RendererImpl.

media: Introduce Renderer interface and RendererImpl.

Add a Renderer interface to manage all audio/video (and in the future text) rendering. With Renderer, Pipeline only needs to manage a Demuxer and a Renderer, which helps move a lot of complicated logic out of Pipeline.

On Desktop Chrome, we use RendererImpl, which manages AudioRendererImpl and VideoRendererImpl.

On other platforms, we could add different Renderer implementation. For example, we could support Browser side decoding/rendering.

BUG=392259
Committed: https://crrev.com/be9da705e341863169faeff532c24c568fad2852
Cr-Commit-Position: refs/heads/master@{#291592}

[xhwang, 2014-07-25 00:39:29]

This is still working in progress. It compiles and test videos play pretty well.
But I need to fix some unit tests. Before I spend more time on this, I'd like
you to take a look to make sure I am on the right direction.

PS1 copies pipeline.* to renderer_impl.* and there's no code change.

PS2 is the CL to review. You can compare PS2 against PS1 to see how code are
moved from Pipeline to RendererImpl.

PTAL!

https://codereview.chromium.org/418143005/diff/40001/media/base/renderer.h
File media/base/renderer.h (right):

https://codereview.chromium.org/418143005/diff/40001/media/base/renderer.h#ne...
media/base/renderer.h:45: // TODO(xhwang): Switch to SetMediaTime() +
StartPlaying() model?
See TODO :)

https://codereview.chromium.org/418143005/diff/40001/media/base/renderer.h#ne...
media/base/renderer.h:66: TimeDeltaInterpolator* interpolator);
It's ugly to put these two here. But I don't have a good way to do it yet. May
clean it up in later CLs.

https://codereview.chromium.org/418143005/diff/40001/media/filters/audio_rend...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/418143005/diff/40001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:593: task_runner_->PostTask(FROM_HERE,
ended_cb_);
Post ended_cb_ so that we don't need to force posting in RendererImpl, as we did
in Pipeline.

[scherkus (not reviewing), 2014-07-31 20:22:09]

overall seems fine -- we should chat some more about the best way to land this
stuff

https://codereview.chromium.org/418143005/diff/40001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/418143005/diff/40001/media/base/pipeline.cc#n...
media/base/pipeline.cc:644: renderer_.reset(new RendererImpl(
it looks like we should inject a Renderer into Pipeline and make
FilterCollection a detail of RendererImpl

that'll help make testing easier

do you think it'd be easier to do that in this CL, or in a future CL?

[xhwang, 2014-08-01 00:10:15]

https://codereview.chromium.org/418143005/diff/40001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/418143005/diff/40001/media/base/pipeline.cc#n...
media/base/pipeline.cc:644: renderer_.reset(new RendererImpl(
On 2014/07/31 20:22:09, scherkus wrote:
> it looks like we should inject a Renderer into Pipeline and make
> FilterCollection a detail of RendererImpl
> 
> that'll help make testing easier
> 
> do you think it'd be easier to do that in this CL, or in a future CL?

Do you mean to pass the Demuxer and Renderer to Pipeline, and make
FilterCollection an impl detail of RendererImpl? That makes sense. I'll finish
updating the tests first, then see how bit that change will be. If it's too big
I'll probably do it in a separate CL.

But how about the demuxer_? The Pipeline owns the Demuxer and the Renderer now.

[xhwang, 2014-08-02 00:01:23]

On 2014/08/01 00:10:15, xhwang wrote:
> https://codereview.chromium.org/418143005/diff/40001/media/base/pipeline.cc
> File media/base/pipeline.cc (right):
> 
>
https://codereview.chromium.org/418143005/diff/40001/media/base/pipeline.cc#n...
> media/base/pipeline.cc:644: renderer_.reset(new RendererImpl(
> On 2014/07/31 20:22:09, scherkus wrote:
> > it looks like we should inject a Renderer into Pipeline and make
> > FilterCollection a detail of RendererImpl
> > 
> > that'll help make testing easier
> > 
> > do you think it'd be easier to do that in this CL, or in a future CL?
> 
> Do you mean to pass the Demuxer and Renderer to Pipeline, and make
> FilterCollection an impl detail of RendererImpl? That makes sense. I'll finish
> updating the tests first, then see how bit that change will be. If it's too
big
> I'll probably do it in a separate CL.
> 
> But how about the demuxer_? The Pipeline owns the Demuxer and the Renderer
now.

I updated FilterCollection so that only Pipeline uses FilterCollection. Renderer
takes ARI and VRI directly.

I'll need to rebase this CL. Otherwise, PTAL.

[xhwang, 2014-08-02 00:39:55]

rebase only

[damienv1, 2014-08-06 16:10:03]

https://codereview.chromium.org/418143005/diff/140001/media/filters/video_ren...
File media/filters/video_renderer_impl.cc (right):

https://codereview.chromium.org/418143005/diff/140001/media/filters/video_ren...
media/filters/video_renderer_impl.cc:130: // failed.
Could you please give me a callstack / sequence of events where this is an issue
?
This seems to me sth that should be handled by the owner of VideoRendererImpl
not by VideoRendererImpl itself.

[damienv1, 2014-08-06 21:43:48]

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

https://codereview.chromium.org/418143005/diff/140001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:1155: return renderer.Pass();
nit: |renderer| not really needed here:

return scoped_ptr<media::Renderer>(new ...);

https://codereview.chromium.org/418143005/diff/140001/media/base/renderer.h
File media/base/renderer.h (right):

https://codereview.chromium.org/418143005/diff/140001/media/base/renderer.h#n...
media/base/renderer.h:19: class MEDIA_EXPORT Renderer {
Renderer seems to be too generic: Renderer is already used quite a lot in chrome
codebase.

Is it possible to be a bit more specific: e.g. MediaRenderer ?

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
File media/filters/renderer_impl.cc (right):

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
media/filters/renderer_impl.cc:80: init_cb_ = init_cb;
nit: |init_cb_| is not really needed: it could be propagated through function
parameters or bind state.

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
media/filters/renderer_impl.cc:165: // WebMediaPlayerImpl to this class.
What is the plan on this side ?

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
media/filters/renderer_impl.cc:198:
base::Bind(&RendererImpl::OnAudioRendererInitializeDone, weak_this_);
To avoid the modifications in AudioRenderer, we could have:

done_cb = BindToCurrentLoop(base::Bind(...));

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
media/filters/renderer_impl.cc:202: task_runner_->PostTask(FROM_HERE,
base::Bind(done_cb, PIPELINE_OK));
No need for PostTask if using BindToCurrentLoop for |done_cb|. And it would be
easier to follow.

done_cb.Run(PIPELINE_OK);

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
media/filters/renderer_impl.cc:294: done_cb.Run();
nit: sounds overkill to create a bind state in this case.

Why not:

if (!audio_renderer_) {
  OnAudioRendererFlushDone();
  return;
}

audio_renderer_->Flush(base::Bind(...));

[xhwang, 2014-08-07 05:46:50]

Comments only. I'll make do will-do-s after I get comments from scherkus.

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

https://codereview.chromium.org/418143005/diff/140001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:1155: return renderer.Pass();
On 2014/08/06 21:43:47, damienv1 wrote:
> nit: |renderer| not really needed here:
> 
> return scoped_ptr<media::Renderer>(new ...);

Will do.

https://codereview.chromium.org/418143005/diff/140001/media/base/renderer.h
File media/base/renderer.h (right):

https://codereview.chromium.org/418143005/diff/140001/media/base/renderer.h#n...
media/base/renderer.h:19: class MEDIA_EXPORT Renderer {
On 2014/08/06 21:43:47, damienv1 wrote:
> Renderer seems to be too generic: Renderer is already used quite a lot in
chrome
> codebase.
> 
> Is it possible to be a bit more specific: e.g. MediaRenderer ?

Well, it's already in the media namespace... Personally I like simpler names.
+scherkus for opinions...

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
File media/filters/renderer_impl.cc (right):

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
media/filters/renderer_impl.cc:80: init_cb_ = init_cb;
On 2014/08/06 21:43:48, damienv1 wrote:
> nit: |init_cb_| is not really needed: it could be propagated through function
> parameters or bind state.

In media code, we don't like outstanding callbacks. Even during teardown, we'll
file all pending callbacks. See FireAllPendingCallbacks().

If we propagate the callback through, when |this| is destructed, we'll
invalidate all weak pointers and the callback will be lost.

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
media/filters/renderer_impl.cc:165: // WebMediaPlayerImpl to this class.
On 2014/08/06 21:43:48, damienv1 wrote:
> What is the plan on this side ?

See http://crbug.com/401264

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
media/filters/renderer_impl.cc:198:
base::Bind(&RendererImpl::OnAudioRendererInitializeDone, weak_this_);
On 2014/08/06 21:43:47, damienv1 wrote:
> To avoid the modifications in AudioRenderer, we could have:
> 
> done_cb = BindToCurrentLoop(base::Bind(...));

In media code, the contract is that the callee (not the caller) will post
callbacks to avoid reentrance. 

One exception is when the callee runs on a different thread, and the caller has
to use BindCurrentLoop() to make sure the callback is posted back to the correct
thread. But this doesn't apply here.

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
media/filters/renderer_impl.cc:202: task_runner_->PostTask(FROM_HERE,
base::Bind(done_cb, PIPELINE_OK));
On 2014/08/06 21:43:47, damienv1 wrote:
> No need for PostTask if using BindToCurrentLoop for |done_cb|. And it would be
> easier to follow.
> 
> done_cb.Run(PIPELINE_OK);

scherkus may have different opinion :) Basically we don't want to abuse
BindToCurrentLoop, which is a bit less obvious for people who are not familiar
with it.

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
media/filters/renderer_impl.cc:294: done_cb.Run();
On 2014/08/06 21:43:47, damienv1 wrote:
> nit: sounds overkill to create a bind state in this case.
> 
> Why not:
> 
> if (!audio_renderer_) {
>   OnAudioRendererFlushDone();
>   return;
> }
> 
> audio_renderer_->Flush(base::Bind(...));

Will do.

https://codereview.chromium.org/418143005/diff/140001/media/filters/video_ren...
File media/filters/video_renderer_impl.cc (right):

https://codereview.chromium.org/418143005/diff/140001/media/filters/video_ren...
media/filters/video_renderer_impl.cc:130: // failed.
On 2014/08/06 16:10:03, damienv1 wrote:
> Could you please give me a callstack / sequence of events where this is an
issue
> ?
> This seems to me sth that should be handled by the owner of VideoRendererImpl
> not by VideoRendererImpl itself.

See:
https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...

Basically in media code the callee should post instead of relying on the caller
to post. Maybe I should just make it an explicitly PostTask.

[scherkus (not reviewing), 2014-08-21 21:17:00]

looking good

damienv: do you feel this new Renderer interface gives you the flexibility you
need?

https://codereview.chromium.org/418143005/diff/140001/media/base/filter_colle...
File media/base/filter_collection.cc (right):

https://codereview.chromium.org/418143005/diff/140001/media/base/filter_colle...
media/base/filter_collection.cc:25: void FilterCollection::SetRenderer(
nit: reformat

https://codereview.chromium.org/418143005/diff/140001/media/base/filter_colle...
File media/base/filter_collection.h (right):

https://codereview.chromium.org/418143005/diff/140001/media/base/filter_colle...
media/base/filter_collection.h:18: // renderers. Used to start a Pipeline object
for media playback.
s/renderers/renderer/

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline.cc#...
media/base/pipeline.cc:191: DCHECK(!renderer_);
nit: are these really needed for a testing function?

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline.cc#...
media/base/pipeline.cc:198: DCHECK(interpolator);
ditto

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline.cc#...
media/base/pipeline.cc:404: // DoXXX() functions is considered safe as they are
owned by
nit: reflow comment

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline.cc#...
media/base/pipeline.cc:492: void Pipeline::AddBufferedTimeRange(TimeDelta start,
nit: reflow

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline.cc#...
media/base/pipeline.cc:568: if (renderer_)
can we make a stronger assumption that renderer_ must exist if we get this task
AND state == kPlaying?

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline.cc#...
media/base/pipeline.cc:579: if (renderer_)
ditto

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline.h#n...
media/base/pipeline.h:177: void
SetTimeDeltaInterpolatorForTesting(TimeDeltaInterpolator* interpolator);
can this get removed and for existing tests they can call it directly on
RendererImpl before passing it to a Pipeline?

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline_uni...
File media/base/pipeline_unittest.cc (right):

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline_uni...
media/base/pipeline_unittest.cc:68: class CallbackHelper {
OOC why move here? so it doesn't pollute namespace?

https://codereview.chromium.org/418143005/diff/140001/media/base/renderer.h
File media/base/renderer.h (right):

https://codereview.chromium.org/418143005/diff/140001/media/base/renderer.h#n...
media/base/renderer.h:19: class MEDIA_EXPORT Renderer {
On 2014/08/07 05:46:49, xhwang wrote:
> On 2014/08/06 21:43:47, damienv1 wrote:
> > Renderer seems to be too generic: Renderer is already used quite a lot in
> chrome
> > codebase.
> > 
> > Is it possible to be a bit more specific: e.g. MediaRenderer ?
> 
> Well, it's already in the media namespace... Personally I like simpler names.
> +scherkus for opinions...

agree with xhwang -- everything is namespaced

https://codereview.chromium.org/418143005/diff/140001/media/base/renderer.h#n...
media/base/renderer.h:46: // Discards any buffered data, executing |done_cb|
when completed.
done_cb -> flush_cb?

or flush_cb -> done_cb?

https://codereview.chromium.org/418143005/diff/140001/media/base/renderer.h#n...
media/base/renderer.h:59: virtual base::TimeDelta GetMediaTime() = 0;
needs docs everywhere

https://codereview.chromium.org/418143005/diff/140001/media/base/renderer.h#n...
media/base/renderer.h:69: virtual void DisableUnderflowForTesting();
can these be moved to RendererImpl for tests?

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
File media/filters/renderer_impl.h (right):

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
media/filters/renderer_impl.h:45: // Stops rendering and fires all pending
callbacks.
nit: no need to duplicate documentation from Renderer

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
media/filters/renderer_impl.h:68: enum State {
for new code can we use MACRO_STYLE as per Chromium style guide?

[xhwang, 2014-08-22 00:09:50]

rebase only

[damienv1, 2014-08-22 16:39:48]

Agree with Andrew, this is going into the right direction (Xiaohan, do you have
a list of follow-up CLs you want to have, e.g. the Renderer factory side).
Is this new design friendly with Android as well (don't much on this side) ?
(You mentioned that ultimately, it would be nice if Android use this model as
well).

Thanks for working on this :)

https://codereview.chromium.org/418143005/diff/160001/media/base/renderer.h
File media/base/renderer.h (right):

https://codereview.chromium.org/418143005/diff/160001/media/base/renderer.h#n...
media/base/renderer.h:30: // TODO(xhwang): Replace |init_cb|, |flush_cb| and
|stop_cb| with Closures.
1) I don't see a Stop. Is it missing ? (or Stop can only be done by destroying
the Renderer ?).

2) Regarding |init_cb|, |flush_cb|, I still don't know whether they should be
closure or not. But since this is not the scope of this CL, that can be
evaluated later on.

https://codereview.chromium.org/418143005/diff/160001/media/base/renderer.h#n...
media/base/renderer.h:46: // Discards any buffered data, executing |done_cb|
when completed.
nit: done_cb VS flush_cb

https://codereview.chromium.org/418143005/diff/160001/media/base/renderer.h#n...
media/base/renderer.h:71: TimeDeltaInterpolator* interpolator);
I don't think this should belong to the base class (Is there any way to put that
only in implementation class ?)
When you do some tests, you actually test an implementation.

[xhwang, 2014-08-22 17:03:21]

comments addressed

[xhwang, 2014-08-22 19:11:07]

On 2014/08/22 16:39:48, damienv1 wrote:
> Agree with Andrew, this is going into the right direction (Xiaohan, do you
have
> a list of follow-up CLs you want to have, e.g. the Renderer factory side).
> Is this new design friendly with Android as well (don't much on this side) ?
> (You mentioned that ultimately, it would be nice if Android use this model as
> well).
> 
Currently we are evaluating using Mojo to implement the out-of-process Renderer
implementation. So in short the original plan will change and currently I don't
have a concrete new plan yet. I'll update with you when we have a concrete plan
again.

[xhwang, 2014-08-22 19:11:32]

I think I addressed all comments. PTAL again.

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

https://codereview.chromium.org/418143005/diff/140001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:1155: return renderer.Pass();
On 2014/08/07 05:46:49, xhwang wrote:
> On 2014/08/06 21:43:47, damienv1 wrote:
> > nit: |renderer| not really needed here:
> > 
> > return scoped_ptr<media::Renderer>(new ...);
> 
> Will do.

Done.

https://codereview.chromium.org/418143005/diff/140001/media/base/filter_colle...
File media/base/filter_collection.cc (right):

https://codereview.chromium.org/418143005/diff/140001/media/base/filter_colle...
media/base/filter_collection.cc:25: void FilterCollection::SetRenderer(
On 2014/08/21 21:16:59, scherkus wrote:
> nit: reformat

Done.

https://codereview.chromium.org/418143005/diff/140001/media/base/filter_colle...
File media/base/filter_collection.h (right):

https://codereview.chromium.org/418143005/diff/140001/media/base/filter_colle...
media/base/filter_collection.h:18: // renderers. Used to start a Pipeline object
for media playback.
On 2014/08/21 21:16:59, scherkus wrote:
> s/renderers/renderer/

Done.

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline.cc#...
media/base/pipeline.cc:191: DCHECK(!renderer_);
On 2014/08/21 21:16:59, scherkus wrote:
> nit: are these really needed for a testing function?

This function is removed.

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline.cc#...
media/base/pipeline.cc:198: DCHECK(interpolator);
On 2014/08/21 21:17:00, scherkus wrote:
> ditto

This function is removed.

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline.cc#...
media/base/pipeline.cc:404: // DoXXX() functions is considered safe as they are
owned by
On 2014/08/21 21:16:59, scherkus wrote:
> nit: reflow comment

Done. Actually now this comment only applies to DoSeek().

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline.cc#...
media/base/pipeline.cc:492: void Pipeline::AddBufferedTimeRange(TimeDelta start,
On 2014/08/21 21:16:59, scherkus wrote:
> nit: reflow

Done.

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline.cc#...
media/base/pipeline.cc:568: if (renderer_)
On 2014/08/21 21:16:59, scherkus wrote:
> can we make a stronger assumption that renderer_ must exist if we get this
task
> AND state == kPlaying?

Done here and in a few other places.

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline.cc#...
media/base/pipeline.cc:579: if (renderer_)
On 2014/08/21 21:16:59, scherkus wrote:
> ditto

Done.

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline.h#n...
media/base/pipeline.h:177: void
SetTimeDeltaInterpolatorForTesting(TimeDeltaInterpolator* interpolator);
On 2014/08/21 21:17:00, scherkus wrote:
> can this get removed and for existing tests they can call it directly on
> RendererImpl before passing it to a Pipeline?

Good point. Done!

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline_uni...
File media/base/pipeline_unittest.cc (right):

https://codereview.chromium.org/418143005/diff/140001/media/base/pipeline_uni...
media/base/pipeline_unittest.cc:68: class CallbackHelper {
On 2014/08/21 21:17:00, scherkus wrote:
> OOC why move here? so it doesn't pollute namespace?

Yep. RendererImplTest is also using CallbackHelp, if I put both under media:: I
have duplicate definitions.

https://codereview.chromium.org/418143005/diff/140001/media/base/renderer.h
File media/base/renderer.h (right):

https://codereview.chromium.org/418143005/diff/140001/media/base/renderer.h#n...
media/base/renderer.h:19: class MEDIA_EXPORT Renderer {
On 2014/08/21 21:17:00, scherkus wrote:
> On 2014/08/07 05:46:49, xhwang wrote:
> > On 2014/08/06 21:43:47, damienv1 wrote:
> > > Renderer seems to be too generic: Renderer is already used quite a lot in
> > chrome
> > > codebase.
> > > 
> > > Is it possible to be a bit more specific: e.g. MediaRenderer ?
> > 
> > Well, it's already in the media namespace... Personally I like simpler
names.
> > +scherkus for opinions...
> 
> agree with xhwang -- everything is namespaced

Acknowledged.

https://codereview.chromium.org/418143005/diff/140001/media/base/renderer.h#n...
media/base/renderer.h:46: // Discards any buffered data, executing |done_cb|
when completed.
On 2014/08/21 21:17:00, scherkus wrote:
> done_cb -> flush_cb?
> 
> or flush_cb -> done_cb?

Done.

https://codereview.chromium.org/418143005/diff/140001/media/base/renderer.h#n...
media/base/renderer.h:59: virtual base::TimeDelta GetMediaTime() = 0;
On 2014/08/21 21:17:00, scherkus wrote:
> needs docs everywhere

Done.

https://codereview.chromium.org/418143005/diff/140001/media/base/renderer.h#n...
media/base/renderer.h:69: virtual void DisableUnderflowForTesting();
On 2014/08/21 21:17:00, scherkus wrote:
> can these be moved to RendererImpl for tests?

Good point. Done.

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
File media/filters/renderer_impl.cc (right):

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
media/filters/renderer_impl.cc:294: done_cb.Run();
On 2014/08/07 05:46:49, xhwang wrote:
> On 2014/08/06 21:43:47, damienv1 wrote:
> > nit: sounds overkill to create a bind state in this case.
> > 
> > Why not:
> > 
> > if (!audio_renderer_) {
> >   OnAudioRendererFlushDone();
> >   return;
> > }
> > 
> > audio_renderer_->Flush(base::Bind(...));
> 
> Will do.

Done.

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
File media/filters/renderer_impl.h (right):

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
media/filters/renderer_impl.h:45: // Stops rendering and fires all pending
callbacks.
On 2014/08/21 21:17:00, scherkus wrote:
> nit: no need to duplicate documentation from Renderer

Done.

https://codereview.chromium.org/418143005/diff/140001/media/filters/renderer_...
media/filters/renderer_impl.h:68: enum State {
On 2014/08/21 21:17:00, scherkus wrote:
> for new code can we use MACRO_STYLE as per Chromium style guide?

Done.

https://codereview.chromium.org/418143005/diff/160001/media/base/renderer.h
File media/base/renderer.h (right):

https://codereview.chromium.org/418143005/diff/160001/media/base/renderer.h#n...
media/base/renderer.h:30: // TODO(xhwang): Replace |init_cb|, |flush_cb| and
|stop_cb| with Closures.
On 2014/08/22 16:39:48, damienv1 wrote:
> 1) I don't see a Stop. Is it missing ? (or Stop can only be done by destroying
> the Renderer ?).

Stop() has been folded into the dtor. Fixed the comment. Thanks for catching
this.
 
> 2) Regarding |init_cb|, |flush_cb|, I still don't know whether they should be
> closure or not. But since this is not the scope of this CL, that can be
> evaluated later on.

Yep. Let's iterate on this later.

https://codereview.chromium.org/418143005/diff/160001/media/base/renderer.h#n...
media/base/renderer.h:46: // Discards any buffered data, executing |done_cb|
when completed.
On 2014/08/22 16:39:48, damienv1 wrote:
> nit: done_cb VS flush_cb

Done.

https://codereview.chromium.org/418143005/diff/160001/media/base/renderer.h#n...
media/base/renderer.h:71: TimeDeltaInterpolator* interpolator);
On 2014/08/22 16:39:48, damienv1 wrote:
> I don't think this should belong to the base class (Is there any way to put
that
> only in implementation class ?)
> When you do some tests, you actually test an implementation.

scherkus@ had the same comment. Removed and only have it in the impl.

[scherkus (not reviewing), 2014-08-22 20:50:01]

https://codereview.chromium.org/418143005/diff/200001/content/renderer/media/...
File content/renderer/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/418143005/diff/200001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.h:205: // Creates a media::Renderer
that will be used by the Pipeline.
nit: s/Pipeline/|pipeline_|/?

https://codereview.chromium.org/418143005/diff/200001/media/base/filter_colle...
File media/base/filter_collection.h (right):

https://codereview.chromium.org/418143005/diff/200001/media/base/filter_colle...
media/base/filter_collection.h:17: // Represents a set of uninitialized demuxer
and audio/video decoders and
I guess there aren't even decoders ... this whole comment is kinda crummy anyway
haha

https://codereview.chromium.org/418143005/diff/200001/media/base/filter_colle...
media/base/filter_collection.h:34: scoped_ptr<TextRenderer> GetTextRenderer();
considering TextRenderer is concrete, we should consider:
  - Moving switches::kEnableInbandTextTracks to media/
  - Have Pipeline create a TextRenderer if switch is present
  - Removing this from FilterCollection
  - Changing Pipeline ctor to accept a Demuxer and Renderer
  - Deleting FilterCollection and fix crbug.com/110800 once and for all :)

(not in this CL of course ... but it's great post-CL cleanup!)

https://codereview.chromium.org/418143005/diff/200001/media/base/renderer.h
File media/base/renderer.h (right):

https://codereview.chromium.org/418143005/diff/200001/media/base/renderer.h#n...
media/base/renderer.h:50: // TODO(xhwang): Switch to SetMediaTime() +
StartPlaying() model?
does this still need to be done? should we chat about this?

https://codereview.chromium.org/418143005/diff/200001/media/base/renderer.h#n...
media/base/renderer.h:60: virtual base::TimeDelta GetMediaTime() = 0;
note that this is the only method that gets called from multiple threads (I
believe)

https://codereview.chromium.org/418143005/diff/200001/media/base/renderer.h#n...
media/base/renderer.h:69: virtual void SetCdm(MediaKeys* cdm) = 0;
does this need to be on the interface, or can it be on the impl?

https://codereview.chromium.org/418143005/diff/200001/media/filters/renderer_...
File media/filters/renderer_impl.cc (right):

https://codereview.chromium.org/418143005/diff/200001/media/filters/renderer_...
media/filters/renderer_impl.cc:165: // WebMediaPlayerImpl to this class.
link to http://crbug.com/401264 in TODO

https://codereview.chromium.org/418143005/diff/200001/media/filters/video_ren...
File media/filters/video_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/418143005/diff/200001/media/filters/video_ren...
media/filters/video_renderer_impl_unittest.cc:545: AdvanceTimeInMs(3000); //
Must match kTimeToDeclareHaveNothing.
nit: can you add an extra space before // ?

[tim (not reviewing), 2014-08-22 22:26:42]

https://codereview.chromium.org/418143005/diff/200001/media/base/renderer.h
File media/base/renderer.h (right):

https://codereview.chromium.org/418143005/diff/200001/media/base/renderer.h#n...
media/base/renderer.h:30: // TODO(xhwang): Replace |init_cb| and |flush_cb| with
Closures.
nit: Looks like you already did this for |flush_cb|

[xhwang, 2014-08-22 22:48:23]

https://codereview.chromium.org/418143005/diff/200001/content/renderer/media/...
File content/renderer/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/418143005/diff/200001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.h:205: // Creates a media::Renderer
that will be used by the Pipeline.
On 2014/08/22 20:50:01, scherkus wrote:
> nit: s/Pipeline/|pipeline_|/?

Done.

https://codereview.chromium.org/418143005/diff/200001/media/base/filter_colle...
File media/base/filter_collection.h (right):

https://codereview.chromium.org/418143005/diff/200001/media/base/filter_colle...
media/base/filter_collection.h:17: // Represents a set of uninitialized demuxer
and audio/video decoders and
On 2014/08/22 20:50:01, scherkus wrote:
> I guess there aren't even decoders ... this whole comment is kinda crummy
anyway
> haha

Done.

https://codereview.chromium.org/418143005/diff/200001/media/base/filter_colle...
media/base/filter_collection.h:34: scoped_ptr<TextRenderer> GetTextRenderer();
On 2014/08/22 20:50:01, scherkus wrote:
> considering TextRenderer is concrete, we should consider:
>   - Moving switches::kEnableInbandTextTracks to media/
>   - Have Pipeline create a TextRenderer if switch is present
>   - Removing this from FilterCollection
>   - Changing Pipeline ctor to accept a Demuxer and Renderer
>   - Deleting FilterCollection and fix crbug.com/110800 once and for all :)
> 
> (not in this CL of course ... but it's great post-CL cleanup!)

Modified TODO to reflect this. Will do after this lands.

https://codereview.chromium.org/418143005/diff/200001/media/base/renderer.h
File media/base/renderer.h (right):

https://codereview.chromium.org/418143005/diff/200001/media/base/renderer.h#n...
media/base/renderer.h:30: // TODO(xhwang): Replace |init_cb| and |flush_cb| with
Closures.
On 2014/08/22 22:26:42, timsteele wrote:
> nit: Looks like you already did this for |flush_cb|

Done.

https://codereview.chromium.org/418143005/diff/200001/media/base/renderer.h#n...
media/base/renderer.h:50: // TODO(xhwang): Switch to SetMediaTime() +
StartPlaying() model?
On 2014/08/22 20:50:01, scherkus wrote:
> does this still need to be done? should we chat about this?

Dropped the TODO. SetMediaTime() doesn't make too much sense here since
TimeSource is hidden.

https://codereview.chromium.org/418143005/diff/200001/media/base/renderer.h#n...
media/base/renderer.h:60: virtual base::TimeDelta GetMediaTime() = 0;
On 2014/08/22 20:50:01, scherkus wrote:
> note that this is the only method that gets called from multiple threads (I
> believe)

I have that comment in renderer_impl.h. I am hesitating to add it here since
this is an interface. In the original plan we could also have a browser side
Renderer impl, where this comment does apply.

https://codereview.chromium.org/418143005/diff/200001/media/base/renderer.h#n...
media/base/renderer.h:69: virtual void SetCdm(MediaKeys* cdm) = 0;
On 2014/08/22 20:50:01, scherkus wrote:
> does this need to be on the interface, or can it be on the impl?

It needs to be on the interface because all Renderers could be associated with a
CDM. In the future, WebMediaPlayer::SetCDM() will be plumbed to this call.

https://codereview.chromium.org/418143005/diff/200001/media/filters/renderer_...
File media/filters/renderer_impl.cc (right):

https://codereview.chromium.org/418143005/diff/200001/media/filters/renderer_...
media/filters/renderer_impl.cc:165: // WebMediaPlayerImpl to this class.
On 2014/08/22 20:50:01, scherkus wrote:
> link to http://crbug.com/401264 in TODO

Done.

https://codereview.chromium.org/418143005/diff/200001/media/filters/video_ren...
File media/filters/video_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/418143005/diff/200001/media/filters/video_ren...
media/filters/video_renderer_impl_unittest.cc:545: AdvanceTimeInMs(3000); //
Must match kTimeToDeclareHaveNothing.
On 2014/08/22 20:50:01, scherkus wrote:
> nit: can you add an extra space before // ?

Done.

[gunsch, 2014-08-22 22:55:27]

https://codereview.chromium.org/418143005/diff/220001/content/renderer/media/...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/418143005/diff/220001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:1172: // TODO(xhwang): Move this
to a factory class so that we can create different
qq: what's the plan for this? Is this expected to be swappable at runtime by the
content embedder implementation? Or are you looking to get further towards the
IPC-based pipeline path before providing that sort of possibility?

[gunsch, 2014-08-22 22:56:35]

https://codereview.chromium.org/418143005/diff/220001/content/renderer/media/...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/418143005/diff/220001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:1172: // TODO(xhwang): Move this
to a factory class so that we can create different
On 2014/08/22 22:55:26, gunsch wrote:
> qq: what's the plan for this? Is this expected to be swappable at runtime by
the
> content embedder implementation? Or are you looking to get further towards the
> IPC-based pipeline path before providing that sort of possibility?

Ah, I just saw your reply to Damien earlier this CL about plans going forward
from here. Feel free to ignore this :)

[xhwang, 2014-08-22 23:00:55]

https://codereview.chromium.org/418143005/diff/220001/content/renderer/media/...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/418143005/diff/220001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:1172: // TODO(xhwang): Move this
to a factory class so that we can create different
On 2014/08/22 22:56:35, gunsch wrote:
> On 2014/08/22 22:55:26, gunsch wrote:
> > qq: what's the plan for this? Is this expected to be swappable at runtime by
> the
> > content embedder implementation? Or are you looking to get further towards
the
> > IPC-based pipeline path before providing that sort of possibility?
> 
> Ah, I just saw your reply to Damien earlier this CL about plans going forward
> from here. Feel free to ignore this :)

The current plan is that we'll create different Renderer implementation for
different platform and pass it to the WebMediaPlayerImpl. I don't think we can
swap Renderers at run-time after the initialization though.

[scherkus (not reviewing), 2014-08-22 23:29:44]

lgtm

[xhwang, 2014-08-22 23:41:20]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-08-22 23:41:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/418143005/220001

[damienv1, 2014-08-22 23:45:04]

lgtm

[damienv1, 2014-08-22 23:45:06]

lgtm

[commit-bot: I haz the power, 2014-08-23 06:55:29]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-23 21:45:03]

Committed patchset #11 (220001) as 2e9a1db9628266155c228cc6003b51157fda60c2

[Jeffrey Yasskin, 2014-08-26 00:42:54]

A revert of this CL (patchset #11) has been created in
https://codereview.chromium.org/503053007/ by jyasskin@chromium.org.

The reason for reverting is:
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28T...

==================
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=13728)
Cycle in lock order graph: M2073 (0x7d4400009808) => M2054 (0x7d480000bb98) =>
M2073

Mutex M2054 acquired here while holding mutex M2073 in main thread:
#0 pthread_mutex_lock <null>:0 (media_unittests+0x00000016d3b0)
#1 base::internal::LockImpl::Lock()
base/synchronization/lock_impl_posix.cc:45:12 (media_unittests+0x0000008a7029)
#2 Acquire base/synchronization/lock.h:23:20 (media_unittests+0x00000073537e)
#3 AutoLock base/synchronization/lock.h:99 (media_unittests+0x00000073537e)
#4 media::Pipeline::GetMediaDuration() const media/base/pipeline.cc:167
(media_unittests+0x00000073537e)
#5 Run base/bind_internal.h:153:12 (media_unittests+0x0000007384c2)
#6 MakeItSo base/bind_internal.h:863 (media_unittests+0x0000007384c2)
#7 base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<base::TimeDelta
(media::Pipeline::*)() const>, base::TimeDelta (media::Pipeline const*), void
(base::internal::UnretainedWrapper<media::Pipeline>)>, base::TimeDelta
(media::Pipeline const*)>::Run(base::internal::BindStateBase*)
base/bind_internal.h:1166 (media_unittests+0x0000007384c2)
#8 Run base/callback.h:401:12 (media_unittests+0x0000007a3bc5)
#9 media::RendererImpl::StartPlayback() media/filters/renderer_impl.cc:461
(media_unittests+0x0000007a3bc5)
#10 media::RendererImpl::OnBufferingStateChanged(media::BufferingState*,
media::BufferingState) media/filters/renderer_impl.cc:418:5
(media_unittests+0x0000007a28d9)
#11 Run base/bind_internal.h:248:12 (media_unittests+0x0000007a423e)
#12 MakeItSo base/bind_internal.h:938 (media_unittests+0x0000007a423e)
#13 base::internal::Invoker<2,
base::internal::BindState<base::internal::RunnableAdapter<void
(media::RendererImpl::*)(media::BufferingState*, media::BufferingState)>, void
(media::RendererImpl*, media::BufferingState*, media::BufferingState), void
(base::WeakPtr<media::RendererImpl>, media::BufferingState*)>, void
(media::RendererImpl*, media::BufferingState*,
media::BufferingState)>::Run(base::internal::BindStateBase*,
media::BufferingState const&) base/bind_internal.h:1343
(media_unittests+0x0000007a423e)
#14 Run base/callback.h:441:12 (media_unittests+0x0000007bdc96)
#15 media::VideoRendererImpl::TransitionToHaveEnough_Locked()
media/filters/video_renderer_impl.cc:385 (media_unittests+0x0000007bdc96)
#16
media::VideoRendererImpl::FrameReady(media::DecoderStream<(media::DemuxerStream::Type)2>::Status,
scoped_refptr<media::VideoFrame> const&)
media/filters/video_renderer_impl.cc:343:5 (media_unittests+0x0000007bda19)
#17 Run base/bind_internal.h:248:12 (media_unittests+0x0000007bdfe8)
#18 MakeItSo base/bind_internal.h:938 (media_unittests+0x0000007bdfe8)
#19 base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<void
(media::VideoRendererImpl::*)(media::DecoderStream<(media::DemuxerStream::Type)2>::Status,
scoped_refptr<media::VideoFrame> const&)>, void (media::VideoRendererImpl*,
media::DecoderStream<(media::DemuxerStream::Type)2>::Status,
scoped_refptr<media::VideoFrame> const&), void
(base::WeakPtr<media::VideoRendererImpl>)>, void (media::VideoRendererImpl*,
media::DecoderStream<(media::DemuxerStream::Type)2>::Status,
scoped_refptr<media::VideoFrame> const&)>::Run(base::internal::BindStateBase*,
media::DecoderStream<(media::DemuxerStream::Type)2>::Status const&,
scoped_refptr<media::VideoFrame> const&) base/bind_internal.h:1310
(media_unittests+0x0000007bdfe8)
#20 Run base/callback.h:483:12 (media_unittests+0x000000774657)
#21 SatisfyRead media/filters/decoder_stream.cc:248
(media_unittests+0x000000774657)
#22
media::DecoderStream<(media::DemuxerStream::Type)2>::OnDecodeOutputReady(scoped_refptr<media::VideoFrame>
const&) media/filters/decoder_stream.cc:369 (media_unittests+0x000000774657)
#23 Run base/bind_internal.h:190:12 (media_unittests+0x00000077afa1)
#24 MakeItSo base/bind_internal.h:909 (media_unittests+0x00000077afa1)
#25 base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<void
(media::DecoderStream<(media::DemuxerStream::Type)2>::*)(scoped_refptr<media::VideoFrame>
const&)>, void (media::DecoderStream<(media::DemuxerStream::Type)2>*,
scoped_refptr<media::VideoFrame> const&), void
(base::WeakPtr<media::DecoderStream<(media::DemuxerStream::Type)2> >)>, void
(media::DecoderStream<(media::DemuxerStream::Type)2>*,
scoped_refptr<media::VideoFrame> const&)>::Run(base::internal::BindStateBase*,
scoped_refptr<media::VideoFrame> const&) base/bind_internal.h:1219
(media_unittests+0x00000077afa1)
#26 Run base/callback.h:441:12 (media_unittests+0x00000066f1b6)
#27 MakeItSo base/bind_internal.h:871 (media_unittests+0x00000066f1b6)
#28 base::internal::Invoker<1, base::internal::BindState<base::Callback<void
(scoped_refptr<media::VideoFrame> const&)>, void
(scoped_refptr<media::VideoFrame> const&), void
(scoped_refptr<media::VideoFrame>)>, void (scoped_refptr<media::VideoFrame>
const&)>::Run(base::internal::BindStateBase*) base/bind_internal.h:1166
(media_unittests+0x00000066f1b6)
#29 Run base/callback.h:401:12 (media_unittests+0x0000008c190b)
#30 base::debug::TaskAnnotator::RunTask(char const*, char const*,
base::PendingTask const&) base/debug/task_annotator.cc:62
(media_unittests+0x0000008c190b)
#31 base::MessageLoop::RunTask(base::PendingTask const&)
base/message_loop/message_loop.cc:435:3 (media_unittests+0x00000088eed3)
#32 DeferOrRunPendingTask base/message_loop/message_loop.cc:445:5
(media_unittests+0x00000088f824)
#33 base::MessageLoop::DoWork() base/message_loop/message_loop.cc:552
(media_unittests+0x00000088f824)
#34 base::MessagePumpDefault::Run(base::MessagePump::Delegate*)
base/message_loop/message_pump_default.cc:32:21 (media_unittests+0x000000892e30)
#35 base::MessageLoop::RunHandler() base/message_loop/message_loop.cc:408:3
(media_unittests+0x00000088e87b)
#36 base::RunLoop::Run() base/run_loop.cc:49:3 (media_unittests+0x00000089d89f)
#37 base::MessageLoop::Run() base/message_loop/message_loop.cc:301:3
(media_unittests+0x00000088e5b5)
#38
media::PipelineIntegrationTest::StartPipelineWithMediaSource(media::MockMediaSource*)
media/filters/pipeline_integration_test.cc:554:5
(media_unittests+0x0000005d4cd1)
#39
media::PipelineIntegrationTest::TestSeekDuringRead(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&,
std::__1::basic_string<char, std::__1::char_traits<char>,
std::__1::allocator<char> > const&, int, base::TimeDelta, base::TimeDelta, int,
int) media/filters/pipeline_integration_test.cc:599:5
(media_unittests+0x0000005e0b42)
#40
media::PipelineIntegrationTest_ChunkDemuxerAbortRead_VideoOnly_Test::TestBody()
media/filters/pipeline_integration_test.cc:1430:3
(media_unittests+0x0000005e0d2d)
#41 HandleExceptionsInMethodIfSupported<testing::Test, void>
testing/gtest/src/gtest.cc:2418:12 (media_unittests+0x00000090b120)
#42 testing::Test::Run() testing/gtest/src/gtest.cc:2434
(media_unittests+0x00000090b120)
#43 testing::TestInfo::Run() testing/gtest/src/gtest.cc:2610:5
(media_unittests+0x00000090c56e)
#44 testing::TestCase::Run() testing/gtest/src/gtest.cc:2728:5
(media_unittests+0x00000090cfcf)
#45 testing::internal::UnitTestImpl::RunAllTests()
testing/gtest/src/gtest.cc:4591:11 (media_unittests+0x000000918ad9)
#46 HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>
testing/gtest/src/gtest.cc:2418:12 (media_unittests+0x000000918516)
#47 testing::UnitTest::Run() testing/gtest/src/gtest.cc:4209
(media_unittests+0x000000918516)
#48 RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2304:10
(media_unittests+0x0000008df65f)
#49 base::TestSuite::Run() base/test/test_suite.cc:227
(media_unittests+0x0000008df65f)
#50 Run base/bind_internal.h:134:12 (media_unittests+0x0000003c5a62)
#51 MakeItSo base/bind_internal.h:863 (media_unittests+0x0000003c5a62)
#52 base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<int
(base::TestSuite::*)()>, int (base::TestSuite*), void
(base::internal::UnretainedWrapper<TestSuiteNoAtExit>)>, int
(base::TestSuite*)>::Run(base::internal::BindStateBase*)
base/bind_internal.h:1166 (media_unittests+0x0000003c5a62)
#53 Run base/callback.h:401:12 (media_unittests+0x0000008da7ec)
#54 base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback<int ()>
const&, int, bool, base::Callback<void ()> const&)
base/test/launcher/unit_test_launcher.cc:498 (media_unittests+0x0000008da7ec)
#55 base::LaunchUnitTests(int, char**, base::Callback<int ()> const&)
base/test/launcher/unit_test_launcher.cc:553:10 (media_unittests+0x0000008da3be)
#56 main media/base/run_all_unittests.cc:45:10 (media_unittests+0x0000003c59a5)

Mutex M2073 previously acquired by the same thread here:
#0 pthread_mutex_lock <null>:0 (media_unittests+0x00000016d3b0)
#1 base::internal::LockImpl::Lock()
base/synchronization/lock_impl_posix.cc:45:12 (media_unittests+0x0000008a7029)
#2 Acquire base/synchronization/lock.h:23:20 (media_unittests+0x0000007a3b3e)
#3 AutoLock base/synchronization/lock.h:99 (media_unittests+0x0000007a3b3e)
#4 media::RendererImpl::StartPlayback() media/filters/renderer_impl.cc:459
(media_unittests+0x0000007a3b3e)
#5 media::RendererImpl::OnBufferingStateChanged(media::BufferingState*,
media::BufferingState) media/filters/renderer_impl.cc:418:5
(media_unittests+0x0000007a28d9)
#6 Run base/bind_internal.h:248:12 (media_unittests+0x0000007a423e)
#7 MakeItSo base/bind_internal.h:938 (media_unittests+0x0000007a423e)
#8 base::internal::Invoker<2,
base::internal::BindState<base::internal::RunnableAdapter<void
(media::RendererImpl::*)(media::BufferingState*, media::BufferingState)>, void
(media::RendererImpl*, media::BufferingState*, media::BufferingState), void
(base::WeakPtr<media::RendererImpl>, media::BufferingState*)>, void
(media::RendererImpl*, media::BufferingState*,
media::BufferingState)>::Run(base::internal::BindStateBase*,
media::BufferingState const&) base/bind_internal.h:1343
(media_unittests+0x0000007a423e)
#9 Run base/callback.h:441:12 (media_unittests+0x0000007bdc96)
#10 media::VideoRendererImpl::TransitionToHaveEnough_Locked()
media/filters/video_renderer_impl.cc:385 (media_unittests+0x0000007bdc96)
#11
media::VideoRendererImpl::FrameReady(media::DecoderStream<(media::DemuxerStream::Type)2>::Status,
scoped_refptr<media::VideoFrame> const&)
media/filters/video_renderer_impl.cc:343:5 (media_unittests+0x0000007bda19)
#12 Run base/bind_internal.h:248:12 (media_unittests+0x0000007bdfe8)
#13 MakeItSo base/bind_internal.h:938 (media_unittests+0x0000007bdfe8)
#14 base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<void
(media::VideoRendererImpl::*)(media::DecoderStream<(media::DemuxerStream::Type)2>::Status,
scoped_refptr<media::VideoFrame> const&)>, void (media::VideoRendererImpl*,
media::DecoderStream<(media::DemuxerStream::Type)2>::Status,
scoped_refptr<media::VideoFrame> const&), void
(base::WeakPtr<media::VideoRendererImpl>)>, void (media::VideoRendererImpl*,
media::DecoderStream<(media::DemuxerStream::Type)2>::Status,
scoped_refptr<media::VideoFrame> const&)>::Run(base::internal::BindStateBase*,
media::DecoderStream<(media::DemuxerStream::Type)2>::Status const&,
scoped_refptr<media::VideoFrame> const&) base/bind_internal.h:1310
(media_unittests+0x0000007bdfe8)
#15 Run base/callback.h:483:12 (media_unittests+0x000000774657)
#16 SatisfyRead media/filters/decoder_stream.cc:248
(media_unittests+0x000000774657)
#17
media::DecoderStream<(media::DemuxerStream::Type)2>::OnDecodeOutputReady(scoped_refptr<media::VideoFrame>
const&) media/filters/decoder_stream.cc:369 (media_unittests+0x000000774657)
#18 Run base/bind_internal.h:190:12 (media_unittests+0x00000077afa1)
#19 MakeItSo base/bind_internal.h:909 (media_unittests+0x00000077afa1)
#20 base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<void
(media::DecoderStream<(media::DemuxerStream::Type)2>::*)(scoped_refptr<media::VideoFrame>
const&)>, void (media::DecoderStream<(media::DemuxerStream::Type)2>*,
scoped_refptr<media::VideoFrame> const&), void
(base::WeakPtr<media::DecoderStream<(media::DemuxerStream::Type)2> >)>, void
(media::DecoderStream<(media::DemuxerStream::Type)2>*,
scoped_refptr<media::VideoFrame> const&)>::Run(base::internal::BindStateBase*,
scoped_refptr<media::VideoFrame> const&) base/bind_internal.h:1219
(media_unittests+0x00000077afa1)
#21 Run base/callback.h:441:12 (media_unittests+0x00000066f1b6)
#22 MakeItSo base/bind_internal.h:871 (media_unittests+0x00000066f1b6)
#23 base::internal::Invoker<1, base::internal::BindState<base::Callback<void
(scoped_refptr<media::VideoFrame> const&)>, void
(scoped_refptr<media::VideoFrame> const&), void
(scoped_refptr<media::VideoFrame>)>, void (scoped_refptr<media::VideoFrame>
const&)>::Run(base::internal::BindStateBase*) base/bind_internal.h:1166
(media_unittests+0x00000066f1b6)
#24 Run base/callback.h:401:12 (media_unittests+0x0000008c190b)
#25 base::debug::TaskAnnotator::RunTask(char const*, char const*,
base::PendingTask const&) base/debug/task_annotator.cc:62
(media_unittests+0x0000008c190b)
#26 base::MessageLoop::RunTask(base::PendingTask const&)
base/message_loop/message_loop.cc:435:3 (media_unittests+0x00000088eed3)
#27 DeferOrRunPendingTask base/message_loop/message_loop.cc:445:5
(media_unittests+0x00000088f824)
#28 base::MessageLoop::DoWork() base/message_loop/message_loop.cc:552
(media_unittests+0x00000088f824)
#29 base::MessagePumpDefault::Run(base::MessagePump::Delegate*)
base/message_loop/message_pump_default.cc:32:21 (media_unittests+0x000000892e30)
#30 base::MessageLoop::RunHandler() base/message_loop/message_loop.cc:408:3
(media_unittests+0x00000088e87b)
#31 base::RunLoop::Run() base/run_loop.cc:49:3 (media_unittests+0x00000089d89f)
#32 base::MessageLoop::Run() base/message_loop/message_loop.cc:301:3
(media_unittests+0x00000088e5b5)
#33
media::PipelineIntegrationTest::StartPipelineWithMediaSource(media::MockMediaSource*)
media/filters/pipeline_integration_test.cc:554:5
(media_unittests+0x0000005d4cd1)
#34
media::PipelineIntegrationTest::TestSeekDuringRead(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&,
std::__1::basic_string<char, std::__1::char_traits<char>,
std::__1::allocator<char> > const&, int, base::TimeDelta, base::TimeDelta, int,
int) media/filters/pipeline_integration_test.cc:599:5
(media_unittests+0x0000005e0b42)
#35
media::PipelineIntegrationTest_ChunkDemuxerAbortRead_VideoOnly_Test::TestBody()
media/filters/pipeline_integration_test.cc:1430:3
(media_unittests+0x0000005e0d2d)
#36 HandleExceptionsInMethodIfSupported<testing::Test, void>
testing/gtest/src/gtest.cc:2418:12 (media_unittests+0x00000090b120)
#37 testing::Test::Run() testing/gtest/src/gtest.cc:2434
(media_unittests+0x00000090b120)
#38 testing::TestInfo::Run() testing/gtest/src/gtest.cc:2610:5
(media_unittests+0x00000090c56e)
#39 testing::TestCase::Run() testing/gtest/src/gtest.cc:2728:5
(media_unittests+0x00000090cfcf)
#40 testing::internal::UnitTestImpl::RunAllTests()
testing/gtest/src/gtest.cc:4591:11 (media_unittests+0x000000918ad9)
#41 HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>
testing/gtest/src/gtest.cc:2418:12 (media_unittests+0x000000918516)
#42 testing::UnitTest::Run() testing/gtest/src/gtest.cc:4209
(media_unittests+0x000000918516)
#43 RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2304:10
(media_unittests+0x0000008df65f)
#44 base::TestSuite::Run() base/test/test_suite.cc:227
(media_unittests+0x0000008df65f)
#45 Run base/bind_internal.h:134:12 (media_unittests+0x0000003c5a62)
#46 MakeItSo base/bind_internal.h:863 (media_unittests+0x0000003c5a62)
#47 base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<int
(base::TestSuite::*)()>, int (base::TestSuite*), void
(base::internal::UnretainedWrapper<TestSuiteNoAtExit>)>, int
(base::TestSuite*)>::Run(base::internal::BindStateBase*)
base/bind_internal.h:1166 (media_unittests+0x0000003c5a62)
#48 Run base/callback.h:401:12 (media_unittests+0x0000008da7ec)
#49 base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback<int ()>
const&, int, bool, base::Callback<void ()> const&)
base/test/launcher/unit_test_launcher.cc:498 (media_unittests+0x0000008da7ec)
#50 base::LaunchUnitTests(int, char**, base::Callback<int ()> const&)
base/test/launcher/unit_test_launcher.cc:553:10 (media_unittests+0x0000008da3be)
#51 main media/base/run_all_unittests.cc:45:10 (media_unittests+0x0000003c59a5)

Mutex M2073 acquired here while holding mutex M2054 in main thread:
#0 pthread_mutex_lock <null>:0 (media_unittests+0x00000016d3b0)
#1 base::internal::LockImpl::Lock()
base/synchronization/lock_impl_posix.cc:45:12 (media_unittests+0x0000008a7029)
#2 Acquire base/synchronization/lock.h:23:20 (media_unittests+0x0000007a22f1)
#3 AutoLock base/synchronization/lock.h:99 (media_unittests+0x0000007a22f1)
#4 media::RendererImpl::GetMediaTime() media/filters/renderer_impl.cc:147
(media_unittests+0x0000007a22f1)
#5 media::Pipeline::GetMediaTime() const media/base/pipeline.cc:157:31
(media_unittests+0x0000007350d1)
#6 media::PipelineIntegrationTestBase::QuitAfterCurrentTimeTask(base::TimeDelta
const&) media/filters/pipeline_integration_test_base.cc:186:7
(media_unittests+0x0000005f01e9)
#7 Run base/bind_internal.h:190:12 (media_unittests+0x0000005f1d2a)
#8 MakeItSo base/bind_internal.h:898 (media_unittests+0x0000005f1d2a)
#9 base::internal::Invoker<2,
base::internal::BindState<base::internal::RunnableAdapter<void
(media::PipelineIntegrationTestBase::*)(base::TimeDelta const&)>, void
(media::PipelineIntegrationTestBase*, base::TimeDelta const&), void
(base::internal::UnretainedWrapper<media::PipelineIntegrationTestBase>,
base::TimeDelta)>, void (media::PipelineIntegrationTestBase*, base::TimeDelta
const&)>::Run(base::internal::BindStateBase*) base/bind_internal.h:1248
(media_unittests+0x0000005f1d2a)
#10 Run base/callback.h:401:12 (media_unittests+0x0000008c190b)
#11 base::debug::TaskAnnotator::RunTask(char const*, char const*,
base::PendingTask const&) base/debug/task_annotator.cc:62
(media_unittests+0x0000008c190b)
#12 base::MessageLoop::RunTask(base::PendingTask const&)
base/message_loop/message_loop.cc:435:3 (media_unittests+0x00000088eed3)
#13 DeferOrRunPendingTask base/message_loop/message_loop.cc:445:5
(media_unittests+0x00000088fd96)
#14 base::MessageLoop::DoDelayedWork(base::TimeTicks*)
base/message_loop/message_loop.cc:590 (media_unittests+0x00000088fd96)
#15 base::MessagePumpDefault::Run(base::MessagePump::Delegate*)
base/message_loop/message_pump_default.cc:36:17 (media_unittests+0x000000892d63)
#16 base::MessageLoop::RunHandler() base/message_loop/message_loop.cc:408:3
(media_unittests+0x00000088e87b)
#17 base::RunLoop::Run() base/run_loop.cc:49:3 (media_unittests+0x00000089d89f)
#18 base::MessageLoop::Run() base/message_loop/message_loop.cc:301:3
(media_unittests+0x00000088e5b5)
#19
media::PipelineIntegrationTestBase::WaitUntilCurrentTimeIsAfter(base::TimeDelta
const&) media/filters/pipeline_integration_test_base.cc:211:3
(media_unittests+0x0000005f0424)
#20
media::PipelineIntegrationTest::TestSeekDuringRead(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&,
std::__1::basic_string<char, std::__1::char_traits<char>,
std::__1::allocator<char> > const&, int, base::TimeDelta, base::TimeDelta, int,
int) media/filters/pipeline_integration_test.cc:605:10
(media_unittests+0x0000005e0b7e)
#21
media::PipelineIntegrationTest_ChunkDemuxerAbortRead_VideoOnly_Test::TestBody()
media/filters/pipeline_integration_test.cc:1430:3
(media_unittests+0x0000005e0d2d)
#22 HandleExceptionsInMethodIfSupported<testing::Test, void>
testing/gtest/src/gtest.cc:2418:12 (media_unittests+0x00000090b120)
#23 testing::Test::Run() testing/gtest/src/gtest.cc:2434
(media_unittests+0x00000090b120)
#24 testing::TestInfo::Run() testing/gtest/src/gtest.cc:2610:5
(media_unittests+0x00000090c56e)
#25 testing::TestCase::Run() testing/gtest/src/gtest.cc:2728:5
(media_unittests+0x00000090cfcf)
#26 testing::internal::UnitTestImpl::RunAllTests()
testing/gtest/src/gtest.cc:4591:11 (media_unittests+0x000000918ad9)
#27 HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>
testing/gtest/src/gtest.cc:2418:12 (media_unittests+0x000000918516)
#28 testing::UnitTest::Run() testing/gtest/src/gtest.cc:4209
(media_unittests+0x000000918516)
#29 RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2304:10
(media_unittests+0x0000008df65f)
#30 base::TestSuite::Run() base/test/test_suite.cc:227
(media_unittests+0x0000008df65f)
#31 Run base/bind_internal.h:134:12 (media_unittests+0x0000003c5a62)
#32 MakeItSo base/bind_internal.h:863 (media_unittests+0x0000003c5a62)
#33 base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<int
(base::TestSuite::*)()>, int (base::TestSuite*), void
(base::internal::UnretainedWrapper<TestSuiteNoAtExit>)>, int
(base::TestSuite*)>::Run(base::internal::BindStateBase*)
base/bind_internal.h:1166 (media_unittests+0x0000003c5a62)
#34 Run base/callback.h:401:12 (media_unittests+0x0000008da7ec)
#35 base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback<int ()>
const&, int, bool, base::Callback<void ()> const&)
base/test/launcher/unit_test_launcher.cc:498 (media_unittests+0x0000008da7ec)
#36 base::LaunchUnitTests(int, char**, base::Callback<int ()> const&)
base/test/launcher/unit_test_launcher.cc:553:10 (media_unittests+0x0000008da3be)
#37 main media/base/run_all_unittests.cc:45:10 (media_unittests+0x0000003c59a5)

Mutex M2054 previously acquired by the same thread here:
#0 pthread_mutex_lock <null>:0 (media_unittests+0x00000016d3b0)
#1 base::internal::LockImpl::Lock()
base/synchronization/lock_impl_posix.cc:45:12 (media_unittests+0x0000008a7029)
#2 Acquire base/synchronization/lock.h:23:20 (media_unittests+0x0000007350a5)
#3 AutoLock base/synchronization/lock.h:99 (media_unittests+0x0000007350a5)
#4 media::Pipeline::GetMediaTime() const media/base/pipeline.cc:156
(media_unittests+0x0000007350a5)
#5 media::PipelineIntegrationTestBase::QuitAfterCurrentTimeTask(base::TimeDelta
const&) media/filters/pipeline_integration_test_base.cc:186:7
(media_unittests+0x0000005f01e9)
#6 Run base/bind_internal.h:190:12 (media_unittests+0x0000005f1d2a)
#7 MakeItSo base/bind_internal.h:898 (media_unittests+0x0000005f1d2a)
#8 base::internal::Invoker<2,
base::internal::BindState<base::internal::RunnableAdapter<void
(media::PipelineIntegrationTestBase::*)(base::TimeDelta const&)>, void
(media::PipelineIntegrationTestBase*, base::TimeDelta const&), void
(base::internal::UnretainedWrapper<media::PipelineIntegrationTestBase>,
base::TimeDelta)>, void (media::PipelineIntegrationTestBase*, base::TimeDelta
const&)>::Run(base::internal::BindStateBase*) base/bind_internal.h:1248
(media_unittests+0x0000005f1d2a)
#9 Run base/callback.h:401:12 (media_unittests+0x0000008c190b)
#10 base::debug::TaskAnnotator::RunTask(char const*, char const*,
base::PendingTask const&) base/debug/task_annotator.cc:62
(media_unittests+0x0000008c190b)
#11 base::MessageLoop::RunTask(base::PendingTask const&)
base/message_loop/message_loop.cc:435:3 (media_unittests+0x00000088eed3)
#12 DeferOrRunPendingTask base/message_loop/message_loop.cc:445:5
(media_unittests+0x00000088fd96)
#13 base::MessageLoop::DoDelayedWork(base::TimeTicks*)
base/message_loop/message_loop.cc:590 (media_unittests+0x00000088fd96)
#14 base::MessagePumpDefault::Run(base::MessagePump::Delegate*)
base/message_loop/message_pump_default.cc:36:17 (media_unittests+0x000000892d63)
#15 base::MessageLoop::RunHandler() base/message_loop/message_loop.cc:408:3
(media_unittests+0x00000088e87b)
#16 base::RunLoop::Run() base/run_loop.cc:49:3 (media_unittests+0x00000089d89f)
#17 base::MessageLoop::Run() base/message_loop/message_loop.cc:301:3
(media_unittests+0x00000088e5b5)
#18
media::PipelineIntegrationTestBase::WaitUntilCurrentTimeIsAfter(base::TimeDelta
const&) media/filters/pipeline_integration_test_base.cc:211:3
(media_unittests+0x0000005f0424)
#19
media::PipelineIntegrationTest::TestSeekDuringRead(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&,
std::__1::basic_string<char, std::__1::char_traits<char>,
std::__1::allocator<char> > const&, int, base::TimeDelta, base::TimeDelta, int,
int) media/filters/pipeline_integration_test.cc:605:10
(media_unittests+0x0000005e0b7e)
#20
media::PipelineIntegrationTest_ChunkDemuxerAbortRead_VideoOnly_Test::TestBody()
media/filters/pipeline_integration_test.cc:1430:3
(media_unittests+0x0000005e0d2d)
#21 HandleExceptionsInMethodIfSupported<testing::Test, void>
testing/gtest/src/gtest.cc:2418:12 (media_unittests+0x00000090b120)
#22 testing::Test::Run() testing/gtest/src/gtest.cc:2434
(media_unittests+0x00000090b120)
#23 testing::TestInfo::Run() testing/gtest/src/gtest.cc:2610:5
(media_unittests+0x00000090c56e)
#24 testing::TestCase::Run() testing/gtest/src/gtest.cc:2728:5
(media_unittests+0x00000090cfcf)
#25 testing::internal::UnitTestImpl::RunAllTests()
testing/gtest/src/gtest.cc:4591:11 (media_unittests+0x000000918ad9)
#26 HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>
testing/gtest/src/gtest.cc:2418:12 (media_unittests+0x000000918516)
#27 testing::UnitTest::Run() testing/gtest/src/gtest.cc:4209
(media_unittests+0x000000918516)
#28 RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2304:10
(media_unittests+0x0000008df65f)
#29 base::TestSuite::Run() base/test/test_suite.cc:227
(media_unittests+0x0000008df65f)
#30 Run base/bind_internal.h:134:12 (media_unittests+0x0000003c5a62)
#31 MakeItSo base/bind_internal.h:863 (media_unittests+0x0000003c5a62)
#32 base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<int
(base::TestSuite::*)()>, int (base::TestSuite*), void
(base::internal::UnretainedWrapper<TestSuiteNoAtExit>)>, int
(base::TestSuite*)>::Run(base::internal::BindStateBase*)
base/bind_internal.h:1166 (media_unittests+0x0000003c5a62)
#33 Run base/callback.h:401:12 (media_unittests+0x0000008da7ec)
#34 base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback<int ()>
const&, int, bool, base::Callback<void ()> const&)
base/test/launcher/unit_test_launcher.cc:498 (media_unittests+0x0000008da7ec)
#35 base::LaunchUnitTests(int, char**, base::Callback<int ()> const&)
base/test/launcher/unit_test_launcher.cc:553:10 (media_unittests+0x0000008da3be)
#36 main media/base/run_all_unittests.cc:45:10 (media_unittests+0x0000003c59a5)

SUMMARY: ThreadSanitizer: lock-order-inversion (potential deadlock) ??:0
pthread_mutex_lock.

[commit-bot: I haz the power, 2014-09-10 02:31:26]

Patchset 11 (id:??) landed as
https://crrev.com/be9da705e341863169faeff532c24c568fad2852
Cr-Commit-Position: refs/heads/master@{#291592}