Use FFmpegVideoDecoder from a UtilityProcess

Use FFmpegVideoDecoder from a UtilityProcess

Add UtilityMojoMediaClient.

The result is already quite beneficial compared to do decoding
in the renderer process. For example killing the utility process
almost does not affect the user. Whereas killing the renderer process
make the tab unavailable, i.e. the user will see the classic
"Aw, Snap! Something went wrong while displaying this web page".

At the moment it just does not restart from the position where
it has crashed, but it sounds doable to implement the feature.
Instead, it pauses on the last frame seen. And clicking play
will for now restart playback from the beginning.

It also seems beneficial compared to do decoding in the gpu
process. Indeed when killing the gpu process, the browser has
to restart the whole GL bits (and sometimes just decides to
fallback to software rendering so no gpu process). Also it
can affects others tabs.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

[Julien Isorce Samsung, 2016-11-28 17:21:14]

Description was changed from

==========
NOT_FOR_REVIEW: use FFmpegVideoDecoder from a UtilityProcess

Contains lot of hacks just for quick demo.

BUG=
==========

to

==========
NOT_FOR_REVIEW: use FFmpegVideoDecoder from a UtilityProcess

Contains lot of hacks just for quick demo.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Julien Isorce Samsung, 2016-11-29 17:21:33]

Description was changed from

==========
NOT_FOR_REVIEW: use FFmpegVideoDecoder from a UtilityProcess

Contains lot of hacks just for quick demo.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
NOT_FOR_REVIEW YET: use FFmpegVideoDecoder from a UtilityProcess

TODO: define a UtilityMojoMediaClient instead of using TestMojoMediaClient
(by taking example from GpuMojoMediaClient 
https://codereview.chromium.org/2382103002/)

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Julien Isorce Samsung, 2016-11-29 17:34:52]

Description was changed from

==========
NOT_FOR_REVIEW YET: use FFmpegVideoDecoder from a UtilityProcess

TODO: define a UtilityMojoMediaClient instead of using TestMojoMediaClient
(by taking example from GpuMojoMediaClient 
https://codereview.chromium.org/2382103002/)

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
NOT_FOR_REVIEW YET: use FFmpegVideoDecoder from a UtilityProcess

The result is already quite beneficial compared to do decoding
in the renderer process. For example killing the utility process
almost does not affect the user. Whereas killing the renderer process
make the tab unavailable, i.e. the user will see the classic
"Aw, Snap! Something went wrong while displaying this web page".

At the moment it just does not restart from the position where
it has crashed, but it sounds doable to implement the feature.
Instead, it pauses on the last frame seen. And clicking play
will for now restart playback from the beginning.

It also seems beneficial compared to do decoding in the gpu
process. Indeed when killing the gpu process, the browser has
to restart the whole GL bits (and sometimes just decides to
fallback to software rendering so no gpu process). Also it
can affects others tabs.

TODO: define a UtilityMojoMediaClient instead of using TestMojoMediaClient
(by taking example from GpuMojoMediaClient 
https://codereview.chromium.org/2382103002/)

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Julien Isorce Samsung, 2016-11-29 17:34:52]

j.isorce@samsung.com changed reviewers:
+ kbr@chromium.org, sandersd@chromium.org

[Julien Isorce Samsung, 2016-11-30 15:07:56]

Description was changed from

==========
NOT_FOR_REVIEW YET: use FFmpegVideoDecoder from a UtilityProcess

The result is already quite beneficial compared to do decoding
in the renderer process. For example killing the utility process
almost does not affect the user. Whereas killing the renderer process
make the tab unavailable, i.e. the user will see the classic
"Aw, Snap! Something went wrong while displaying this web page".

At the moment it just does not restart from the position where
it has crashed, but it sounds doable to implement the feature.
Instead, it pauses on the last frame seen. And clicking play
will for now restart playback from the beginning.

It also seems beneficial compared to do decoding in the gpu
process. Indeed when killing the gpu process, the browser has
to restart the whole GL bits (and sometimes just decides to
fallback to software rendering so no gpu process). Also it
can affects others tabs.

TODO: define a UtilityMojoMediaClient instead of using TestMojoMediaClient
(by taking example from GpuMojoMediaClient 
https://codereview.chromium.org/2382103002/)

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Use FFmpegVideoDecoder from a UtilityProcess

Add UtilityMojoMediaClient.

The result is already quite beneficial compared to do decoding
in the renderer process. For example killing the utility process
almost does not affect the user. Whereas killing the renderer process
make the tab unavailable, i.e. the user will see the classic
"Aw, Snap! Something went wrong while displaying this web page".

At the moment it just does not restart from the position where
it has crashed, but it sounds doable to implement the feature.
Instead, it pauses on the last frame seen. And clicking play
will for now restart playback from the beginning.

It also seems beneficial compared to do decoding in the gpu
process. Indeed when killing the gpu process, the browser has
to restart the whole GL bits (and sometimes just decides to
fallback to software rendering so no gpu process). Also it
can affects others tabs.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Julien Isorce Samsung, 2016-11-30 15:07:57]

j.isorce@samsung.com changed reviewers:
+ xhwang@chromium.org

[Julien Isorce Samsung, 2016-11-30 15:08:13]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 15:08:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 15:23:11]

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

[commit-bot: I haz the power, 2016-11-30 15:23:12]

Dry run: Try jobs failed on following builders:
  linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)

[Julien Isorce Samsung, 2016-11-30 17:14:42]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 17:15:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 17:22:26]

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

[commit-bot: I haz the power, 2016-11-30 17:22:27]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Julien Isorce Samsung, 2016-12-01 10:23:50]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 10:24:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 11:29:23]

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

[commit-bot: I haz the power, 2016-12-01 11:29:24]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Julien Isorce Samsung, 2016-12-01 15:11:27]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 15:11:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 16:41:39]

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

[commit-bot: I haz the power, 2016-12-01 16:41:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Julien Isorce Samsung, 2016-12-12 22:57:02]

j.isorce@samsung.com changed reviewers:
+ dalecurtis@chromium.org, tguilbert@chromium.org
- kbr@chromium.org

[Julien Isorce Samsung, 2016-12-12 23:03:10]

Hi,
This CL is not like in a final state, I am submitting it for review because I
need some input whether this is something you are interested in. If yes then
whether the VideoFramePoolDelegate is a good choice or not. It is used in
UtilityMojoMediaClient::CreateVideoDecoder to make the VideoFramePool create
MojoSharedBufferVideoFrame instead of simply VideoFrame. 
Thx!
Julien

[sandersd (OOO until July 31), 2016-12-15 00:38:02]

I will leave questions about the right direction for this to xhwang@. Overall
code comments only.

https://codereview.chromium.org/2115603002/diff/160001/media/base/video_frame...
File media/base/video_frame_pool.cc (right):

https://codereview.chromium.org/2115603002/diff/160001/media/base/video_frame...
media/base/video_frame_pool.cc:88: } else {
These conditions can be removed by creating a default video frame provider that
is backed by VideoFrame.

https://codereview.chromium.org/2115603002/diff/160001/media/base/video_frame...
File media/base/video_frame_pool.h (right):

https://codereview.chromium.org/2115603002/diff/160001/media/base/video_frame...
media/base/video_frame_pool.h:16: class VideoFramePoolDelegate {
I am opposed to the name Delegate on the grounds that media/ has a few of them
and it never means the same thing. This particular usage might be better named
VideoFrameProvider? (Or go all the way and call it
VideoFrame::Provider/VideoFrame::Factory or something like that.)

https://codereview.chromium.org/2115603002/diff/160001/media/base/video_frame...
media/base/video_frame_pool.h:54: void
SetDelegate(std::unique_ptr<VideoFramePoolDelegate> delegate);
It seems like the pool doesn't work without calling this. I recommend making it
a constructor argument with a default value.

https://codereview.chromium.org/2115603002/diff/160001/media/filters/ffmpeg_v...
File media/filters/ffmpeg_video_decoder.h (right):

https://codereview.chromium.org/2115603002/diff/160001/media/filters/ffmpeg_v...
media/filters/ffmpeg_video_decoder.h:55: void SetVideoFramePoolDelegate(
This should also be a constructor parameter.

https://codereview.chromium.org/2115603002/diff/160001/media/media_options.gni
File media/media_options.gni (right):

https://codereview.chromium.org/2115603002/diff/160001/media/media_options.gn...
media/media_options.gni:122: } else if (is_desktop_linux) {
|is_desktop_linux| seems to be defined as |current_os == "linux"|. Does that
exclude chromecast?

If not, there should be a comment or condition here to make it clear that the
order of the conditions matters.

https://codereview.chromium.org/2115603002/diff/160001/media/media_options.gn...
media/media_options.gni:126: mojo_media_services = [ "video_decoder" ]
Do you plan to expand past desktop linux eventually? If so we have a problem
here, since this is the path that VDAs (DXVA, VTVDA, and the CrOS VDAs) will be
using soon.

https://codereview.chromium.org/2115603002/diff/160001/media/mojo/common/mojo...
File media/mojo/common/mojo_shared_buffer_video_frame.cc (right):

https://codereview.chromium.org/2115603002/diff/160001/media/mojo/common/mojo...
media/mojo/common/mojo_shared_buffer_video_frame.cc:75: LOG(DFATAL) <<
__FUNCTION__ << " Invalid config. "
Nit: Prefer __func__ to __FUNCTION__.

[Julien Isorce Samsung, 2016-12-15 16:59:39]

On 2016/12/15 00:38:02, sandersd wrote:
> I will leave questions about the right direction for this to xhwang@. Overall
> code comments only.
> 

Thx a lot for your comments Dan, I will apply what you suggested with
factory/provider
and default case, if it is worth to continue further, so depending on xhwang@ 's
reply. Also only linux desktop because I think it is the less risky platform
to incubate this CL. Also providing that hw decoders are not enabled by default.

[xhwang, 2016-12-15 18:58:10]

I'll be OOO next week so I am afraid I won't have time to do a thorough review.
Here are some super early thoughts.

1. A bug and design doc would be really nice for high level discussion than in
code reviews.

2. I agree with the benefits you described. But:
- I assume the crash rate is low :)
- By moving FFVD to the utility process, it'll be harder to repro/debug crashes.
In the utility process, we don't have all the info we have in the render
process.
- Running a remote video decoder does come with costs. Performance-wise, IPC is
not free. Also, transmitting compressed buffers involves some memory copy. This
will also adds latency to the whole media pipeline.

3. With (2), I don't feel we should enable this by default on any platform. If
you think this is something you want in your product, one option is to add a new
gn arg which can be enabled by your product, but default to false.

[Julien Isorce Samsung, 2017-01-17 17:06:11]

I addressed remarks here https://codereview.chromium.org/2633383003/

On 2016/12/15 18:58:10, xhwang wrote:
> I'll be OOO next week so I am afraid I won't have time to do a thorough
review.
> Here are some super early thoughts.
> 
> 1. A bug and design doc would be really nice for high level discussion than in
> code reviews.

Done bug and doc: http://crbug/681852

> 
> 2. I agree with the benefits you described. But:
> - I assume the crash rate is low :)
> - By moving FFVD to the utility process, it'll be harder to repro/debug
crashes.
> In the utility process, we don't have all the info we have in the render
> process.
> - Running a remote video decoder does come with costs. Performance-wise, IPC
is
> not free. Also, transmitting compressed buffers involves some memory copy.
This
> will also adds latency to the whole media pipeline.
> 
> 3. With (2), I don't feel we should enable this by default on any platform. If
> you think this is something you want in your product, one option is to add a
new
> gn arg which can be enabled by your product, but default to false.

Done, it will only be enable through gn arg, see bug above.

Thx!