Hook WebMediaPlayerImpl up to Mojo's HTMLDocumentView.

Hook WebMediaPlayerImpl up to Mojo's HTMLDocumentView.

This patch provides the minimal changes necessary to hook
WebMediaPlayerImpl into HTMLDocumentView so that <audio>/<video> tags
work. HTMLMediaElement will now function, but audio won't be audible
right now. Proper audio output will be added in a followup patch.

Committed: https://crrev.com/50b1254ab61e1c74469253d65df4cce64fdc9127
Cr-Commit-Position: refs/heads/master@{#294692}

[acolwell GONE FROM CHROMIUM, 2014-09-11 21:45:57]

acolwell@chromium.org changed reviewers:
+ darin@chromium.org, scherkus@chromium.org

[acolwell GONE FROM CHROMIUM, 2014-09-11 21:45:58]

scherkus : Please review everything.
darin: mojo/services/html_viewer/html_viewer.cc
mojo/services/html_viewer/html_document_view.*

[scherkus (not reviewing), 2014-09-12 00:17:53]

https://codereview.chromium.org/567803002/diff/20001/media/blink/null_encrypt...
File media/blink/null_encrypted_media_player_support.cc (right):

https://codereview.chromium.org/567803002/diff/20001/media/blink/null_encrypt...
media/blink/null_encrypted_media_player_support.cc:12:
scoped_ptr<media::EncryptedMediaPlayerSupport>
s/media:://

https://codereview.chromium.org/567803002/diff/20001/media/blink/null_encrypt...
File media/blink/null_encrypted_media_player_support.h (right):

https://codereview.chromium.org/567803002/diff/20001/media/blink/null_encrypt...
media/blink/null_encrypted_media_player_support.h:19: static
scoped_ptr<media::EncryptedMediaPlayerSupport> Create(
strip off media:: here and everywhere else

https://codereview.chromium.org/567803002/diff/20001/mojo/services/html_viewe...
File mojo/services/html_viewer/webmediaplayer_factory.cc (right):

https://codereview.chromium.org/567803002/diff/20001/mojo/services/html_viewe...
mojo/services/html_viewer/webmediaplayer_factory.cc:23:
media::InitializeMediaLibraryForTesting();
sanity check: do you want to CHECK()?

https://codereview.chromium.org/567803002/diff/20001/mojo/services/html_viewe...
File mojo/services/html_viewer/webmediaplayer_factory.h (right):

https://codereview.chromium.org/567803002/diff/20001/mojo/services/html_viewe...
mojo/services/html_viewer/webmediaplayer_factory.h:36: class
WebMediaPlayerFactory {
instead of plumbing a WMPF* around ... maybe this could be a LazyInstance<T>?

https://codereview.chromium.org/567803002/diff/20001/mojo/services/html_viewe...
mojo/services/html_viewer/webmediaplayer_factory.h:51: scoped_ptr<base::Thread>
media_thread_;
nit: this can be not a pointer and instead you can check whether the thread has
started

https://codereview.chromium.org/567803002/diff/20001/mojo/services/html_viewe...
mojo/services/html_viewer/webmediaplayer_factory.h:54:
scoped_ptr<media::AudioHardwareConfig> audio_hardware_config_;
nit: I think all of these can be de-pointer-ized and stuffed all in the ctor

[scherkus (not reviewing), 2014-09-12 00:18:28]

+davemoore as CC since we were discussing this yesterday

[acolwell GONE FROM CHROMIUM, 2014-09-12 16:46:03]

https://codereview.chromium.org/567803002/diff/20001/media/blink/null_encrypt...
File media/blink/null_encrypted_media_player_support.cc (right):

https://codereview.chromium.org/567803002/diff/20001/media/blink/null_encrypt...
media/blink/null_encrypted_media_player_support.cc:12:
scoped_ptr<media::EncryptedMediaPlayerSupport>
On 2014/09/12 00:17:52, scherkus wrote:
> s/media:://

Done.

https://codereview.chromium.org/567803002/diff/20001/media/blink/null_encrypt...
File media/blink/null_encrypted_media_player_support.h (right):

https://codereview.chromium.org/567803002/diff/20001/media/blink/null_encrypt...
media/blink/null_encrypted_media_player_support.h:19: static
scoped_ptr<media::EncryptedMediaPlayerSupport> Create(
On 2014/09/12 00:17:52, scherkus wrote:
> strip off media:: here and everywhere else

Done.

https://codereview.chromium.org/567803002/diff/20001/mojo/services/html_viewe...
File mojo/services/html_viewer/webmediaplayer_factory.cc (right):

https://codereview.chromium.org/567803002/diff/20001/mojo/services/html_viewe...
mojo/services/html_viewer/webmediaplayer_factory.cc:23:
media::InitializeMediaLibraryForTesting();
On 2014/09/12 00:17:52, scherkus wrote:
> sanity check: do you want to CHECK()?

Done.

https://codereview.chromium.org/567803002/diff/20001/mojo/services/html_viewe...
File mojo/services/html_viewer/webmediaplayer_factory.h (right):

https://codereview.chromium.org/567803002/diff/20001/mojo/services/html_viewe...
mojo/services/html_viewer/webmediaplayer_factory.h:36: class
WebMediaPlayerFactory {
On 2014/09/12 00:17:53, scherkus wrote:
> instead of plumbing a WMPF* around ... maybe this could be a LazyInstance<T>?

I'm not sure what the runtime environment for the services is yet. It isn't
clear to me LazyInstance<T> is the right thing to do here since I believe that
would make this process global right?

https://codereview.chromium.org/567803002/diff/20001/mojo/services/html_viewe...
mojo/services/html_viewer/webmediaplayer_factory.h:51: scoped_ptr<base::Thread>
media_thread_;
On 2014/09/12 00:17:53, scherkus wrote:
> nit: this can be not a pointer and instead you can check whether the thread
has
> started

Done.

https://codereview.chromium.org/567803002/diff/20001/mojo/services/html_viewe...
mojo/services/html_viewer/webmediaplayer_factory.h:54:
scoped_ptr<media::AudioHardwareConfig> audio_hardware_config_;
On 2014/09/12 00:17:53, scherkus wrote:
> nit: I think all of these can be de-pointer-ized and stuffed all in the ctor

AudioManager can't. Its constructor is protected. I've fixed
audio_hardware_config_ though.

[darin (slow to review), 2014-09-12 16:57:53]

jamesr@ can review this in my place. I'm out today.
On Sep 11, 2014 2:45 PM, <acolwell@chromium.org> wrote:

> Reviewers: scherkus, darin,
>
> Message:
> scherkus : Please review everything.
> darin: mojo/services/html_viewer/html_viewer.cc
> mojo/services/html_viewer/html_document_view.*
>
> Description:
> Hook WebMediaPlayerImpl up to Mojo's HTMLDocumentView.
>
> This patch provides the minimal changes necessary to hook
> WebMediaPlayerImpl into HTMLDocumentView so that <audio>/<video> tags
> work. HTMLMediaElement will now function, but audio won't be audible
> right now. Proper audio output will be added in a followup patch.
>
> Please review this at https://codereview.chromium.org/567803002/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+356, -12 lines):
>   M media/blink/BUILD.gn
>   M media/blink/media_blink.gyp
>   A media/blink/null_encrypted_media_player_support.h
>   A media/blink/null_encrypted_media_player_support.cc
>   M media/blink/webmediaplayer_params.h
>   M mojo/mojo_services.gypi
>   M mojo/services/html_viewer/DEPS
>   M mojo/services/html_viewer/html_document_view.h
>   M mojo/services/html_viewer/html_document_view.cc
>   M mojo/services/html_viewer/html_viewer.cc
>   A mojo/services/html_viewer/webmediaplayer_factory.h
>   A mojo/services/html_viewer/webmediaplayer_factory.cc
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[scherkus (not reviewing), 2014-09-12 18:33:36]

scherkus@chromium.org changed reviewers:
+ jamesr@chromium.org
- darin@chromium.org

[scherkus (not reviewing), 2014-09-12 18:33:37]

lgtm

swapping darin -> jamesr to review mojo stuff

[jamesr, 2014-09-12 19:07:44]

This conflicts with the html_viewer compositor bindings, which just landed. 
Could you rebase?  I probably also need to add a bit of compositor support for
sending yuv planes as resources, should be pretty straightforward though.  If
you get dchecks after rebasing when playing video lemme know.

[acolwell GONE FROM CHROMIUM, 2014-09-12 20:34:03]

On 2014/09/12 19:07:44, jamesr wrote:
> This conflicts with the html_viewer compositor bindings, which just landed. 
> Could you rebase?  I probably also need to add a bit of compositor support for
> sending yuv planes as resources, should be pretty straightforward though.  If
> you get dchecks after rebasing when playing video lemme know.

Rebased. Getting "[FATAL:surfaces_type_converters.cc(280)] Check failed: false.
Unsupported material 11" now. 
I'm happy to work on a fix for this, but I'd prefer to fix it in a followup CL.
Audio-only playback doesn't crash so this patch is useful and moves things
forward on its own.

[jamesr, 2014-09-12 21:01:36]

On 2014/09/12 20:34:03, acolwell_leaving_chromium_9-23 wrote:
> On 2014/09/12 19:07:44, jamesr wrote:
> > This conflicts with the html_viewer compositor bindings, which just landed. 
> > Could you rebase?  I probably also need to add a bit of compositor support
for
> > sending yuv planes as resources, should be pretty straightforward though. 
If
> > you get dchecks after rebasing when playing video lemme know.
> 
> Rebased. Getting "[FATAL:surfaces_type_converters.cc(280)] Check failed:
false.
> Unsupported material 11" now. 
> I'm happy to work on a fix for this, but I'd prefer to fix it in a followup
CL.
> Audio-only playback doesn't crash so this patch is useful and moves things
> forward on its own.

OK, that means surface_type_converters needs to learn about YUV_VIDEO_CONTENT. 
Doing it in a separate CL sounds fine (I can do it, it's simple enough).  Will
take a look at the patch now..

[jamesr, 2014-09-12 21:03:59]

mojo/ lgtm

https://codereview.chromium.org/567803002/diff/80001/mojo/services/html_viewe...
File mojo/services/html_viewer/webmediaplayer_factory.h (right):

https://codereview.chromium.org/567803002/diff/80001/mojo/services/html_viewe...
mojo/services/html_viewer/webmediaplayer_factory.h:38: WebMediaPlayerFactory(
explicit

https://codereview.chromium.org/567803002/diff/80001/mojo/services/html_viewe...
mojo/services/html_viewer/webmediaplayer_factory.h:40: ;
woah this line wrapping is weird. is this what 'git cl format' did?

[acolwell GONE FROM CHROMIUM, 2014-09-12 21:43:41]

https://codereview.chromium.org/567803002/diff/80001/mojo/services/html_viewe...
File mojo/services/html_viewer/webmediaplayer_factory.h (right):

https://codereview.chromium.org/567803002/diff/80001/mojo/services/html_viewe...
mojo/services/html_viewer/webmediaplayer_factory.h:38: WebMediaPlayerFactory(
On 2014/09/12 21:03:58, jamesr wrote:
> explicit

Done.

https://codereview.chromium.org/567803002/diff/80001/mojo/services/html_viewe...
mojo/services/html_viewer/webmediaplayer_factory.h:40: ;
On 2014/09/12 21:03:58, jamesr wrote:
> woah this line wrapping is weird. is this what 'git cl format' did?

Nope. That was done manually. I just ran 'git cl format' on it and it looks
better now. I didn't realize I could linebreak on the <

[acolwell GONE FROM CHROMIUM, 2014-09-12 21:43:51]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-09-12 21:45:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/567803002/100001

[commit-bot: I haz the power, 2014-09-12 22:17:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-12 22:17:14]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[acolwell GONE FROM CHROMIUM, 2014-09-12 22:51:21]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-09-12 22:54:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/567803002/100001

[commit-bot: I haz the power, 2014-09-12 23:22:03]

Committed patchset #6 (id:100001) as 7e3a3f0fee32b2f280894a596fc61c06dc718744

[commit-bot: I haz the power, 2014-09-12 23:28:43]

Patchset 6 (id:??) landed as
https://crrev.com/50b1254ab61e1c74469253d65df4cce64fdc9127
Cr-Commit-Position: refs/heads/master@{#294692}