Vanquish the remnants of media::MessageLoopFactory.

Vanquish the remnants of media::MessageLoopFactory.

Now that media code runs on a single thread we no longer need to worry about creating additional threads at runtime.

To make changes like this easier in the future, webkit_media::WebMediaPlayerParams is introduced to avoid plumbing parameters through Chrome's various abstraction layers.

BUG=116873
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174808

[scherkus (not reviewing), 2012-12-07 23:54:11]

acolwell: see gpu_video_decoder.h

jam: see chrome/renderer/chrome_content_renderer_client.h

https://codereview.chromium.org/11468033/diff/2001/chrome/renderer/chrome_con...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/11468033/diff/2001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.h:85: const
scoped_refptr<media::GpuVideoDecoder::Factories>& gpu_factories,
we can't forward declare the inner class, resulting in an entry to
content/public/renderer/DEPS

jam: any ideas? de-inner-ify the class? switch to a params struct similar to
OverrideCreatePlugin()? (I hate the amount of param plumbing going on here)

https://codereview.chromium.org/11468033/diff/2001/media/filters/gpu_video_de...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/11468033/diff/2001/media/filters/gpu_video_de...
media/filters/gpu_video_decoder.h:59: virtual
scoped_refptr<base::MessageLoopProxy> GetMessageLoop() = 0;
this is because we now create GpuVideoDecoder in webkit/ as opposed to content/
and can't access the proper loop

alternatively, we can create the thread + GpuVideoDecoder in content/ and
plumbing them into WMPI() (it better reflects the end goal of passing a
MessageLoopProxy of the shared media thread into WMPI) -- it gets a bit hairy
though with OverrideWMPI()

acowell: any thoughts / preferences?

[jam, 2012-12-10 04:23:03]

https://codereview.chromium.org/11468033/diff/2001/chrome/renderer/chrome_con...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/11468033/diff/2001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.h:85: const
scoped_refptr<media::GpuVideoDecoder::Factories>& gpu_factories,
On 2012/12/07 23:54:11, scherkus wrote:
> we can't forward declare the inner class, resulting in an entry to
> content/public/renderer/DEPS
> 
> jam: any ideas? de-inner-ify the class? switch to a params struct similar to
> OverrideCreatePlugin()? (I hate the amount of param plumbing going on here)

this is why the style guide says to avoid nested classes, since they can't be
forward declared:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Nested_Classes

it seems good to minimize the includes of content_*_client.h files by forward
declaring parameters

https://codereview.chromium.org/11468033/diff/2001/content/renderer/media/ren...
File content/renderer/media/renderer_gpu_video_decoder_factories.h (right):

https://codereview.chromium.org/11468033/diff/2001/content/renderer/media/ren...
content/renderer/media/renderer_gpu_video_decoder_factories.h:58: virtual
scoped_refptr<base::MessageLoopProxy> GetMessageLoop() OVERRIDE;
nit: convention is no blank lines between overridden methods

[acolwell GONE FROM CHROMIUM, 2012-12-10 18:12:15]

https://codereview.chromium.org/11468033/diff/2001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/11468033/diff/2001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:2598: media::AudioRendererImpl*
audio_renderer =
Why is this change here? It appears unrelated to the MessageLoopFactory changes.

https://codereview.chromium.org/11468033/diff/2001/media/tools/player_x11/pla...
File media/tools/player_x11/player_x11.cc (left):

https://codereview.chromium.org/11468033/diff/2001/media/tools/player_x11/pla...
media/tools/player_x11/player_x11.cc:109: media::MessageLoopFactory*
message_loop_factory) {
What about this?

https://codereview.chromium.org/11468033/diff/2001/media/tools/player_x11/pla...
File media/tools/player_x11/player_x11.cc (right):

https://codereview.chromium.org/11468033/diff/2001/media/tools/player_x11/pla...
media/tools/player_x11/player_x11.cc:258: scoped_ptr<media::MessageLoopFactory>
message_loop_factory(
ditto

https://codereview.chromium.org/11468033/diff/2001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/11468033/diff/2001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:196: if (!audio_renderer_sink) {
Why are these audio changes included in this CL?

https://codereview.chromium.org/11468033/diff/2001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.h (left):

https://codereview.chromium.org/11468033/diff/2001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.h:118: media::FilterCollection* collection,
How about we create a MediaContext object that holds the FilterCollection,
audio_renderer_sink, and media_thread so that we only have to pass one parameter
down through all the layers? Then we wouldn't have to route the GPU specific
details everywhere. The WebMediaPlayerImpl could then own this object instead of
maintaining the seperate member variables.

[scherkus (not reviewing), 2012-12-21 22:41:39]

PTAL -- went the params-struct route

should have done something like this sooner!

https://codereview.chromium.org/11468033/diff/2001/content/renderer/media/ren...
File content/renderer/media/renderer_gpu_video_decoder_factories.h (right):

https://codereview.chromium.org/11468033/diff/2001/content/renderer/media/ren...
content/renderer/media/renderer_gpu_video_decoder_factories.h:58: virtual
scoped_refptr<base::MessageLoopProxy> GetMessageLoop() OVERRIDE;
On 2012/12/10 04:23:03, John Abd-El-Malek wrote:
> nit: convention is no blank lines between overridden methods

Done.

https://codereview.chromium.org/11468033/diff/2001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/11468033/diff/2001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:2598: media::AudioRendererImpl*
audio_renderer =
On 2012/12/10 18:12:16, acolwell wrote:
> Why is this change here? It appears unrelated to the MessageLoopFactory
changes.

This snuck in -- xhwang@ ended up landing this in http://crrev.com/173019

https://codereview.chromium.org/11468033/diff/12056/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.h (left):

https://codereview.chromium.org/11468033/diff/12056/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.h:93: // Construct a WebMediaPlayerImpl with
reference to the client, and media
I didn't find this comment very useful to keep around given the one at the top
of the file

https://codereview.chromium.org/11468033/diff/12056/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.h:108: // When calling this, the
|audio_source_provider| and
these comments move to WMPParams

[jam, 2012-12-26 16:12:03]

lgtm

[acolwell GONE FROM CHROMIUM, 2012-12-26 17:47:51]

LGTM

https://codereview.chromium.org/11468033/diff/12056/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/11468033/diff/12056/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:161: // Let V8 know we started new thread if
we did not did it yet.
nit: s/did not did/did not do

[scherkus (not reviewing), 2013-01-02 17:01:48]

https://codereview.chromium.org/11468033/diff/12056/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/11468033/diff/12056/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:161: // Let V8 know we started new thread if
we did not did it yet.
On 2012/12/26 17:47:51, acolwell wrote:
> nit: s/did not did/did not do

Done.