New media/base/decode_capabilities.h|cc

New media/base/decode_capabilities.h|cc

Media/ should describe its decode capabilities. Embedders may
override via MediaClient. For time being, this preserves the
simple logic from content::RenderMediaClient. In the long run
it will check profiles for actual decoder support and include
android specific logic currently in MimeUtilInternal.

BUG=695264
Review-Url: https://codereview.chromium.org/2758873003
Cr-Commit-Position: refs/heads/master@{#459352}
Committed: https://chromium.googlesource.com/chromium/src/+/115663853cd6d59c87f6b605381d52498c179bfb

[chcunningham, 2017-03-17 21:47:16]

The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 21:47:52]

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

[chcunningham, 2017-03-17 21:48:33]

chcunningham@chromium.org changed reviewers:
+ hubbe@chromium.org

[chcunningham, 2017-03-17 21:48:34]

Setting up a core part of the MediaCapabilities skeleton :)

[commit-bot: I haz the power, 2017-03-18 01:33:20]

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

[commit-bot: I haz the power, 2017-03-18 01:33:20]

Dry run: This issue passed the CQ dry run.

[chcunningham, 2017-03-21 22:23:50]

Friendly ping.

[hubbe, 2017-03-23 18:14:31]

https://codereview.chromium.org/2758873003/diff/1/content/renderer/media/rend...
File content/renderer/media/render_media_client.cc (left):

https://codereview.chromium.org/2758873003/diff/1/content/renderer/media/rend...
content/renderer/media/render_media_client.cc:96: bool
IsColorSpaceSupported(const media::VideoColorSpace& color_space) {
I'm confused, where did this code go?

https://codereview.chromium.org/2758873003/diff/1/media/base/decode_capabilit...
File media/base/decode_capabilities.h (right):

https://codereview.chromium.org/2758873003/diff/1/media/base/decode_capabilit...
media/base/decode_capabilities.h:17: struct MEDIA_EXPORT VideoConfig {
Might be nice to update VideoDecoderConfig to use one of these later.

[chcunningham, 2017-03-23 19:20:53]

https://codereview.chromium.org/2758873003/diff/1/content/renderer/media/rend...
File content/renderer/media/render_media_client.cc (left):

https://codereview.chromium.org/2758873003/diff/1/content/renderer/media/rend...
content/renderer/media/render_media_client.cc:96: bool
IsColorSpaceSupported(const media::VideoColorSpace& color_space) {
On 2017/03/23 18:14:30, hubbe wrote:
> I'm confused, where did this code go?

Its now in media/base/decode_capabiliites.cc.

The idea is that these color/decode capabilities are really default capabilities
for media/.  media/ should be in the business of describing those capabilities
(rather than doing it here in content/).

Embedders of content who wish to *customize* media capabilities should describe
their additions/subtractions in their specific MediaClient implementation. 

This design is discussed a bit more between me and xhwang in this CL
https://codereview.chromium.org/2712983004/

https://codereview.chromium.org/2758873003/diff/1/media/base/decode_capabilit...
File media/base/decode_capabilities.h (right):

https://codereview.chromium.org/2758873003/diff/1/media/base/decode_capabilit...
media/base/decode_capabilities.h:17: struct MEDIA_EXPORT VideoConfig {
On 2017/03/23 18:14:30, hubbe wrote:
> Might be nice to update VideoDecoderConfig to use one of these later.

I actually tried to add level to VideoDecoderConfig, but it was a pretty ugly
and massive change. Most users of VideoDecoderConfig don't care about level and
its constructor already has over 10 parameters. Dale recommended a new struct
like you see here. Down the road  MediaCapabilities queries will continue to
grow to contain things not needed/wanted in VideoDecoderConfig.

[hubbe, 2017-03-23 19:33:00]

https://codereview.chromium.org/2758873003/diff/1/content/renderer/media/rend...
File content/renderer/media/render_media_client.cc (left):

https://codereview.chromium.org/2758873003/diff/1/content/renderer/media/rend...
content/renderer/media/render_media_client.cc:96: bool
IsColorSpaceSupported(const media::VideoColorSpace& color_space) {
On 2017/03/23 19:20:53, chcunningham wrote:
> On 2017/03/23 18:14:30, hubbe wrote:
> > I'm confused, where did this code go?
> 
> Its now in media/base/decode_capabiliites.cc.
>

I don't see that in this CL though.
I expected a LARGE RED BLOCK to be matched by a LARGE GREEN BLOCK elsewhere...

 
> The idea is that these color/decode capabilities are really default
capabilities
> for media/.  media/ should be in the business of describing those capabilities
> (rather than doing it here in content/).
> 
> Embedders of content who wish to *customize* media capabilities should
describe
> their additions/subtractions in their specific MediaClient implementation. 
> 
> This design is discussed a bit more between me and xhwang in this CL
> https://codereview.chromium.org/2712983004/

https://codereview.chromium.org/2758873003/diff/1/media/base/decode_capabilit...
File media/base/decode_capabilities.h (right):

https://codereview.chromium.org/2758873003/diff/1/media/base/decode_capabilit...
media/base/decode_capabilities.h:17: struct MEDIA_EXPORT VideoConfig {
On 2017/03/23 19:20:53, chcunningham wrote:
> On 2017/03/23 18:14:30, hubbe wrote:
> > Might be nice to update VideoDecoderConfig to use one of these later.
> 
> I actually tried to add level to VideoDecoderConfig, but it was a pretty ugly
> and massive change. Most users of VideoDecoderConfig don't care about level
and
> its constructor already has over 10 parameters. Dale recommended a new struct
> like you see here. Down the road  MediaCapabilities queries will continue to
> grow to contain things not needed/wanted in VideoDecoderConfig.

Acknowledged.

[chcunningham, 2017-03-23 19:37:47]

https://codereview.chromium.org/2758873003/diff/1/content/renderer/media/rend...
File content/renderer/media/render_media_client.cc (left):

https://codereview.chromium.org/2758873003/diff/1/content/renderer/media/rend...
content/renderer/media/render_media_client.cc:96: bool
IsColorSpaceSupported(const media::VideoColorSpace& color_space) {
On 2017/03/23 19:33:00, hubbe wrote:
> On 2017/03/23 19:20:53, chcunningham wrote:
> > On 2017/03/23 18:14:30, hubbe wrote:
> > > I'm confused, where did this code go?
> > 
> > Its now in media/base/decode_capabiliites.cc.
> >
> 
> I don't see that in this CL though.
> I expected a LARGE RED BLOCK to be matched by a LARGE GREEN BLOCK elsewhere...
> 
>  
> > The idea is that these color/decode capabilities are really default
> capabilities
> > for media/.  media/ should be in the business of describing those
capabilities
> > (rather than doing it here in content/).
> > 
> > Embedders of content who wish to *customize* media capabilities should
> describe
> > their additions/subtractions in their specific MediaClient implementation. 
> > 
> > This design is discussed a bit more between me and xhwang in this CL
> > https://codereview.chromium.org/2712983004/

Unless I've messed it up, you should find all of this IsColorSpaceSupported
here, but you will have to "expand all" to see it. Its not a large green block
becuase git is detecting decode_capabilities as a rename of render_media_client
- which is probably what we want anyway.

https://codereview.chromium.org/2758873003/diff/1/media/base/decode_capabilit...

[hubbe, 2017-03-23 19:42:42]

lgtm

https://codereview.chromium.org/2758873003/diff/1/content/renderer/media/rend...
File content/renderer/media/render_media_client.cc (left):

https://codereview.chromium.org/2758873003/diff/1/content/renderer/media/rend...
content/renderer/media/render_media_client.cc:96: bool
IsColorSpaceSupported(const media::VideoColorSpace& color_space) {
On 2017/03/23 19:37:47, chcunningham wrote:
> On 2017/03/23 19:33:00, hubbe wrote:
> > On 2017/03/23 19:20:53, chcunningham wrote:
> > > On 2017/03/23 18:14:30, hubbe wrote:
> > > > I'm confused, where did this code go?
> > > 
> > > Its now in media/base/decode_capabiliites.cc.
> > >
> > 
> > I don't see that in this CL though.
> > I expected a LARGE RED BLOCK to be matched by a LARGE GREEN BLOCK
elsewhere...
> > 
> >  
> > > The idea is that these color/decode capabilities are really default
> > capabilities
> > > for media/.  media/ should be in the business of describing those
> capabilities
> > > (rather than doing it here in content/).
> > > 
> > > Embedders of content who wish to *customize* media capabilities should
> > describe
> > > their additions/subtractions in their specific MediaClient implementation.

> > > 
> > > This design is discussed a bit more between me and xhwang in this CL
> > > https://codereview.chromium.org/2712983004/
> 
> Unless I've messed it up, you should find all of this IsColorSpaceSupported
> here, but you will have to "expand all" to see it. Its not a large green block
> becuase git is detecting decode_capabilities as a rename of
render_media_client
> - which is probably what we want anyway.
> 
>
https://codereview.chromium.org/2758873003/diff/1/media/base/decode_capabilit...

Ah, super confusing.

[chcunningham, 2017-03-24 01:09:36]

The CQ bit was checked by chcunningham@chromium.org

[chcunningham, 2017-03-24 01:09:37]

The patchset sent to the CQ was uploaded after l-g-t-m from hubbe@chromium.org
Link to the patchset: https://codereview.chromium.org/2758873003/#ps20001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-24 01:09:56]

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, 2017-03-24 03:22:05]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490317776716450,
"parent_rev": "3e655172a781a71727269de3416ffba84d285ddd", "commit_rev":
"115663853cd6d59c87f6b605381d52498c179bfb"}

[commit-bot: I haz the power, 2017-03-24 03:23:28]

Description was changed from

==========
New media/base/decode_capabilities.h|cc

Media/ should describe its decode capabilities. Embedders may
override via MediaClient. For time being, this preserves the
simple logic from content::RenderMediaClient. In the long run
it will check profiles for actual decoder support and include
android specific logic currently in MimeUtilInternal.

BUG=695264
==========

to

==========
New media/base/decode_capabilities.h|cc

Media/ should describe its decode capabilities. Embedders may
override via MediaClient. For time being, this preserves the
simple logic from content::RenderMediaClient. In the long run
it will check profiles for actual decoder support and include
android specific logic currently in MimeUtilInternal.

BUG=695264

Review-Url: https://codereview.chromium.org/2758873003
Cr-Commit-Position: refs/heads/master@{#459352}
Committed:
https://chromium.googlesource.com/chromium/src/+/115663853cd6d59c87f6b605381d...
==========

[commit-bot: I haz the power, 2017-03-24 03:23:30]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/115663853cd6d59c87f6b605381d...