Support HEVC through EME

Support HEVC through EME

Currently we support HEVC demuxing through regular MSE and ffmpeg code
paths. Now we need to add also support for HEVC playback (and
appropriate media mime type and codec support checks) through EME code
path as well.

BUG=606564
Committed: https://crrev.com/779123fd5a7c241cfffa34e93c786b0ad96956b0
Cr-Commit-Position: refs/heads/master@{#399964}

[servolk, 2016-04-25 23:31:03]

Description was changed from

==========
HEVC support through EME
==========

to

==========
Support HEVC through EME

Currently we support HEVC demuxing through regular MSE and ffmpeg code
paths. Now we need to add also support for HEVC playback (and
appropriate media mime type and codec support checks) through EME code
path as well.

BUG=606564
==========

[servolk, 2016-04-25 23:31:15]

servolk@chromium.org changed reviewers:
+ ddorwin@chromium.org

[servolk, 2016-05-12 18:25:15]

servolk@chromium.org changed reviewers:
+ halliwell@chromium.org

[halliwell, 2016-05-13 01:53:20]

https://codereview.chromium.org/1909313002/diff/60001/media/test/pipeline_int...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1909313002/diff/60001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1925: }
Where will these tests get run in practice?  Is it worth setting the buildflag
for public cast_shell bot and adding media tests to it?

[servolk, 2016-05-13 18:33:02]

https://codereview.chromium.org/1909313002/diff/60001/media/test/pipeline_int...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1909313002/diff/60001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1925: }
On 2016/05/13 01:53:20, halliwell wrote:
> Where will these tests get run in practice?  Is it worth setting the buildflag
> for public cast_shell bot and adding media tests to it?

Yes, these tests will run on cast_shell* trybots, HEVC demuxing is already
enabled there (and only there for now).
You can check out the logs for media_unittests on cast_shell_linux trybot in
this CL to confirm that these test cases got run there (the log link is
https://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_lin...):
[2905/2936]
PipelineIntegrationTest.BasicPlayback_MediaSource_VideoOnly_MP4_HEVC1 (1 ms)
[2906/2936]
PipelineIntegrationTest.BasicPlayback_MediaSource_VideoOnly_MP4_HEVC2 (0 ms)

[servolk, 2016-05-13 18:42:51]

https://codereview.chromium.org/1909313002/diff/60001/media/test/pipeline_int...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1909313002/diff/60001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1925: }
On 2016/05/13 18:33:02, servolk wrote:
> On 2016/05/13 01:53:20, halliwell wrote:
> > Where will these tests get run in practice?  Is it worth setting the
buildflag
> > for public cast_shell bot and adding media tests to it?
> 
> Yes, these tests will run on cast_shell* trybots, HEVC demuxing is already
> enabled there (and only there for now).
> You can check out the logs for media_unittests on cast_shell_linux trybot in
> this CL to confirm that these test cases got run there (the log link is
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_lin...):
> [2905/2936]
> PipelineIntegrationTest.BasicPlayback_MediaSource_VideoOnly_MP4_HEVC1 (1 ms)
> [2906/2936]
> PipelineIntegrationTest.BasicPlayback_MediaSource_VideoOnly_MP4_HEVC2 (0 ms)

Btw, note that these tests actually use a regular playback code path. Perhaps we
should also add some new tests to
content/browser/media/encrypted_media_browsertest.cc for end-to-end testing of
this. But I have noticed that so far there's no proprietary codecs tests in
content/browser/media/encrypted_media_browsertest.cc at all.

[halliwell, 2016-05-13 22:09:58]

On 2016/05/13 18:42:51, servolk wrote:
>
https://codereview.chromium.org/1909313002/diff/60001/media/test/pipeline_int...
> File media/test/pipeline_integration_test.cc (right):
> 
>
https://codereview.chromium.org/1909313002/diff/60001/media/test/pipeline_int...
> media/test/pipeline_integration_test.cc:1925: }
> On 2016/05/13 18:33:02, servolk wrote:
> > On 2016/05/13 01:53:20, halliwell wrote:
> > > Where will these tests get run in practice?  Is it worth setting the
> buildflag
> > > for public cast_shell bot and adding media tests to it?
> > 
> > Yes, these tests will run on cast_shell* trybots, HEVC demuxing is already
> > enabled there (and only there for now).
> > You can check out the logs for media_unittests on cast_shell_linux trybot in
> > this CL to confirm that these test cases got run there (the log link is
> >
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_lin...):
> > [2905/2936]
> > PipelineIntegrationTest.BasicPlayback_MediaSource_VideoOnly_MP4_HEVC1 (1 ms)
> > [2906/2936]
> > PipelineIntegrationTest.BasicPlayback_MediaSource_VideoOnly_MP4_HEVC2 (0 ms)
> 
> Btw, note that these tests actually use a regular playback code path. Perhaps
we
> should also add some new tests to
> content/browser/media/encrypted_media_browsertest.cc for end-to-end testing of
> this. But I have noticed that so far there's no proprietary codecs tests in
> content/browser/media/encrypted_media_browsertest.cc at all.

Ok thanks, good to know :)

[servolk, 2016-05-17 18:58:22]

On 2016/05/13 22:09:58, halliwell wrote:
> On 2016/05/13 18:42:51, servolk wrote:
> >
>
https://codereview.chromium.org/1909313002/diff/60001/media/test/pipeline_int...
> > File media/test/pipeline_integration_test.cc (right):
> > 
> >
>
https://codereview.chromium.org/1909313002/diff/60001/media/test/pipeline_int...
> > media/test/pipeline_integration_test.cc:1925: }
> > On 2016/05/13 18:33:02, servolk wrote:
> > > On 2016/05/13 01:53:20, halliwell wrote:
> > > > Where will these tests get run in practice?  Is it worth setting the
> > buildflag
> > > > for public cast_shell bot and adding media tests to it?
> > > 
> > > Yes, these tests will run on cast_shell* trybots, HEVC demuxing is already
> > > enabled there (and only there for now).
> > > You can check out the logs for media_unittests on cast_shell_linux trybot
in
> > > this CL to confirm that these test cases got run there (the log link is
> > >
> >
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_lin...):
> > > [2905/2936]
> > > PipelineIntegrationTest.BasicPlayback_MediaSource_VideoOnly_MP4_HEVC1 (1
ms)
> > > [2906/2936]
> > > PipelineIntegrationTest.BasicPlayback_MediaSource_VideoOnly_MP4_HEVC2 (0
ms)
> > 
> > Btw, note that these tests actually use a regular playback code path.
Perhaps
> we
> > should also add some new tests to
> > content/browser/media/encrypted_media_browsertest.cc for end-to-end testing
of
> > this. But I have noticed that so far there's no proprietary codecs tests in
> > content/browser/media/encrypted_media_browsertest.cc at all.
> 
> Ok thanks, good to know :)

David, could you take a look please?
Also, do you you know why there's no EME tests for proprietary codecs in
content/browser/media/encrypted_media_browsertest.cc? Are the properietary
codecs with combination with EME tested somewhere else?

[ddorwin, 2016-05-18 19:51:14]

This seems to be two separate CLs combined into one. Can we break them up?

https://codereview.chromium.org/1909313002/diff/60001/media/base/eme_constants.h
File media/base/eme_constants.h (right):

https://codereview.chromium.org/1909313002/diff/60001/media/base/eme_constant...
media/base/eme_constants.h:43: #if !BUILDFLAG(ENABLE_HEVC_DEMUXING)
You might need to update cdm_message_filter_android.cc if you want to support
Android.

https://codereview.chromium.org/1909313002/diff/60001/media/base/eme_constant...
media/base/eme_constants.h:46: EME_CODEC_MP4_HEVC = 1 << 7,
This CL alone does not enable this anywhere.

https://codereview.chromium.org/1909313002/diff/60001/media/test/data/README
File media/test/data/README (right):

https://codereview.chromium.org/1909313002/diff/60001/media/test/data/README#...
media/test/data/README:222: media/test/data/bear-320x240-v_frag-hevc.mp4
Is this file encrypted?

https://codereview.chromium.org/1909313002/diff/60001/media/test/pipeline_int...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1909313002/diff/60001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:732: EXPECT_EQ(pipeline_status_,
expected_pipeline_status_);
Expected comes first in GTest.

https://codereview.chromium.org/1909313002/diff/60001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:767: EXPECT_EQ(pipeline_status_,
expected_pipeline_status_);
ditto

https://codereview.chromium.org/1909313002/diff/60001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1921: expected_pipeline_status_ =
DECODER_ERROR_NOT_SUPPORTED;
Adding a member variable to the base class is overly intrusive for these two
tests, and it is not consistently used base the base or its children.

It would be much simpler to add a new
StartPipelineWithMediaSourceAndExpectStatus() or something similar. The main
StartPipelineWithMediaSource will need to be updated to handle an additional
parameter.

[servolk, 2016-05-19 00:43:43]

https://codereview.chromium.org/1909313002/diff/60001/media/base/eme_constants.h
File media/base/eme_constants.h (right):

https://codereview.chromium.org/1909313002/diff/60001/media/base/eme_constant...
media/base/eme_constants.h:43: #if !BUILDFLAG(ENABLE_HEVC_DEMUXING)
On 2016/05/18 19:51:13, ddorwin wrote:
> You might need to update cdm_message_filter_android.cc if you want to support
> Android.

Done.

https://codereview.chromium.org/1909313002/diff/60001/media/base/eme_constant...
media/base/eme_constants.h:46: EME_CODEC_MP4_HEVC = 1 << 7,
On 2016/05/18 19:51:13, ddorwin wrote:
> This CL alone does not enable this anywhere.

Done.

https://codereview.chromium.org/1909313002/diff/60001/media/test/data/README
File media/test/data/README (right):

https://codereview.chromium.org/1909313002/diff/60001/media/test/data/README#...
media/test/data/README:222: media/test/data/bear-320x240-v_frag-hevc.mp4
On 2016/05/18 19:51:13, ddorwin wrote:
> Is this file encrypted?

No, it's not.

https://codereview.chromium.org/1909313002/diff/60001/media/test/pipeline_int...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1909313002/diff/60001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:732: EXPECT_EQ(pipeline_status_,
expected_pipeline_status_);
On 2016/05/18 19:51:13, ddorwin wrote:
> Expected comes first in GTest.

Done.

https://codereview.chromium.org/1909313002/diff/60001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:767: EXPECT_EQ(pipeline_status_,
expected_pipeline_status_);
On 2016/05/18 19:51:14, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/1909313002/diff/60001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1921: expected_pipeline_status_ =
DECODER_ERROR_NOT_SUPPORTED;
On 2016/05/18 19:51:14, ddorwin wrote:
> Adding a member variable to the base class is overly intrusive for these two
> tests, and it is not consistently used base the base or its children.
> 
> It would be much simpler to add a new
> StartPipelineWithMediaSourceAndExpectStatus() or something similar. The main
> StartPipelineWithMediaSource will need to be updated to handle an additional
> parameter.

I've decided to go a little further and introduce a new overload for
StartPipelineWithMediaSource function that consolidates the code that is
currently duplicated between StartPipelineWithMediaSource and
StartPipelineWithEncryptedMedia. The new function will simply return the
PipelineStatus without checking what it is, and then the old
StartPipelineWithMediaSource and StartPipelineWithEncryptedMedia can call it and
do the same checks as before.
All of that was done in a separate CL

[ddorwin, 2016-05-19 21:32:13]

Should we add to encrypted_media_supported_types_browsertest.cc? There are three
Video_MP4 tests.

https://codereview.chromium.org/1909313002/diff/100001/chromecast/renderer/ke...
File chromecast/renderer/key_systems_cast.cc (right):

https://codereview.chromium.org/1909313002/diff/100001/chromecast/renderer/ke...
chromecast/renderer/key_systems_cast.cc:44: ::media::EME_CODEC_MP4_HEVC;
I know this is the default state for Cast in Chromium, but I assume it can be
overridden for specific devices. Should it therefore be protected by the build
flag?

[servolk, 2016-05-20 23:00:19]

https://codereview.chromium.org/1909313002/diff/100001/chromecast/renderer/ke...
File chromecast/renderer/key_systems_cast.cc (right):

https://codereview.chromium.org/1909313002/diff/100001/chromecast/renderer/ke...
chromecast/renderer/key_systems_cast.cc:44: ::media::EME_CODEC_MP4_HEVC;
On 2016/05/19 21:32:13, ddorwin wrote:
> I know this is the default state for Cast in Chromium, but I assume it can be
> overridden for specific devices. Should it therefore be protected by the build
> flag?

Done.

[servolk, 2016-06-14 18:46:59]

On 2016/05/20 23:00:19, servolk wrote:
>
https://codereview.chromium.org/1909313002/diff/100001/chromecast/renderer/ke...
> File chromecast/renderer/key_systems_cast.cc (right):
> 
>
https://codereview.chromium.org/1909313002/diff/100001/chromecast/renderer/ke...
> chromecast/renderer/key_systems_cast.cc:44: ::media::EME_CODEC_MP4_HEVC;
> On 2016/05/19 21:32:13, ddorwin wrote:
> > I know this is the default state for Cast in Chromium, but I assume it can
be
> > overridden for specific devices. Should it therefore be protected by the
build
> > flag?
> 
> Done.

ping

[ddorwin, 2016-06-14 21:18:40]

From before:

Should we add to encrypted_media_supported_types_browsertest.cc? There are three
Video_MP4 tests.

https://codereview.chromium.org/1909313002/diff/140001/chrome/browser/media/e...
File chrome/browser/media/encrypted_media_supported_types_browsertest.cc
(right):

https://codereview.chromium.org/1909313002/diff/140001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_supported_types_browsertest.cc:144: #if
BUILDFLAG(ENABLE_MP4_VP9_DEMUXING)
These are always invalid. :)

[servolk, 2016-06-14 21:30:11]

I probably should have called that out explicitly: I've made the necessary
changes to encrypted_media_supported_types_browsertest.cc in PS #7.

https://codereview.chromium.org/1909313002/diff/140001/chrome/browser/media/e...
File chrome/browser/media/encrypted_media_supported_types_browsertest.cc
(right):

https://codereview.chromium.org/1909313002/diff/140001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_supported_types_browsertest.cc:144: #if
BUILDFLAG(ENABLE_MP4_VP9_DEMUXING)
On 2016/06/14 21:18:40, ddorwin wrote:
> These are always invalid. :)

Done. Also noticed that I forgot to update BUILDFLAG name after copy/paste
above. Fixed that too.

[ddorwin, 2016-06-14 21:57:52]

LGTM with one comment to cover the negative case too.

On 2016/06/14 21:30:11, servolk wrote:
> I probably should have called that out explicitly: I've made the necessary
> changes to encrypted_media_supported_types_browsertest.cc in PS #7.

Ah, yes. I was thinking of something else.

https://codereview.chromium.org/1909313002/diff/160001/chrome/browser/media/e...
File chrome/browser/media/encrypted_media_supported_types_browsertest.cc
(right):

https://codereview.chromium.org/1909313002/diff/160001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_supported_types_browsertest.cc:129: #endif
#else
  Add to invalid_codecs_

That way we have negative tests for these valid strings.

[servolk, 2016-06-14 22:03:22]

https://codereview.chromium.org/1909313002/diff/160001/chrome/browser/media/e...
File chrome/browser/media/encrypted_media_supported_types_browsertest.cc
(right):

https://codereview.chromium.org/1909313002/diff/160001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_supported_types_browsertest.cc:129: #endif
On 2016/06/14 21:57:51, ddorwin wrote:
> #else
>   Add to invalid_codecs_
> 
> That way we have negative tests for these valid strings.

Done.

[servolk, 2016-06-14 22:06:06]

The CQ bit was checked by servolk@chromium.org

[servolk, 2016-06-14 22:06:06]

The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/1909313002/#ps180001
(title: "CR feedback")

[commit-bot: I haz the power, 2016-06-14 22:06:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1909313002/180001

[commit-bot: I haz the power, 2016-06-14 22:16:30]

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

[commit-bot: I haz the power, 2016-06-14 22:16:32]

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

[halliwell, 2016-06-15 17:39:52]

On 2016/06/14 22:16:32, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

chromecast/ lgtm % gn question

[halliwell, 2016-06-15 17:40:04]

https://codereview.chromium.org/1909313002/diff/180001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/1909313002/diff/180001/chromecast/chromecast....
chromecast/chromecast.gyp:327: 'media/media.gyp:media_features',
Actually no need to update gyp now.  Do we need an equivalent change in gn?

[servolk, 2016-06-15 17:42:30]

On 2016/06/15 17:40:04, halliwell wrote:
>
https://codereview.chromium.org/1909313002/diff/180001/chromecast/chromecast.gyp
> File chromecast/chromecast.gyp (right):
> 
>
https://codereview.chromium.org/1909313002/diff/180001/chromecast/chromecast....
> chromecast/chromecast.gyp:327: 'media/media.gyp:media_features',
> Actually no need to update gyp now.  Do we need an equivalent change in gn?

Let's keep gyp up-to-date just in case for upstream trybots for now. Re GN: we
don't need this change to be explicit for GN, since GN is better at tracking
transitive deps and key_systems_cast.cc is part of the 'renderer' GN source set
that already has dependency on //media (which in turn has dependency on
//media:media_features). So I believe the GN should already be good as-is.

[servolk, 2016-06-15 17:43:23]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2016-06-15 17:45:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1909313002/180001

[commit-bot: I haz the power, 2016-06-15 18:27:23]

Description was changed from

==========
Support HEVC through EME

Currently we support HEVC demuxing through regular MSE and ffmpeg code
paths. Now we need to add also support for HEVC playback (and
appropriate media mime type and codec support checks) through EME code
path as well.

BUG=606564
==========

to

==========
Support HEVC through EME

Currently we support HEVC demuxing through regular MSE and ffmpeg code
paths. Now we need to add also support for HEVC playback (and
appropriate media mime type and codec support checks) through EME code
path as well.

BUG=606564
==========

[commit-bot: I haz the power, 2016-06-15 18:27:24]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-06-15 18:27:32]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-15 18:28:50]

Description was changed from

==========
Support HEVC through EME

Currently we support HEVC demuxing through regular MSE and ffmpeg code
paths. Now we need to add also support for HEVC playback (and
appropriate media mime type and codec support checks) through EME code
path as well.

BUG=606564
==========

to

==========
Support HEVC through EME

Currently we support HEVC demuxing through regular MSE and ffmpeg code
paths. Now we need to add also support for HEVC playback (and
appropriate media mime type and codec support checks) through EME code
path as well.

BUG=606564

Committed: https://crrev.com/779123fd5a7c241cfffa34e93c786b0ad96956b0
Cr-Commit-Position: refs/heads/master@{#399964}
==========

[commit-bot: I haz the power, 2016-06-15 18:28:51]

Patchset 10 (id:??) landed as
https://crrev.com/779123fd5a7c241cfffa34e93c786b0ad96956b0
Cr-Commit-Position: refs/heads/master@{#399964}