Add Chromium-side changes for MediaSource::isTypeSupported()

Add Chromium-side changes for MediaSource::isTypeSupported()


BUG=172687
TEST=Existing tests pass & will be tested by a LayoutTest when the WebKit side lands.


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

[acolwell GONE FROM CHROMIUM, 2013-03-14 00:03:10]

Please take a look.

darin@ for webkit/glue changes.
scherkus@ the rest.

Here are the WebKit-side changes:
https://bugs.webkit.org/show_bug.cgi?id=112301

darin@ if have time to look at the WebKit changes as well I'd appreciate it.
They are pretty trivial.

[darin (slow to review), 2013-03-14 00:14:01]

https://codereview.chromium.org/12713004/diff/17001/webkit/glue/webkitplatfor...
File webkit/glue/webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/12713004/diff/17001/webkit/glue/webkitplatfor...
webkit/glue/webkitplatformsupport_impl.h:130: virtual bool
isTypeSupportedForMediaSource(
Why isn't this a function on WebMIMERegistry?

[acolwell GONE FROM CHROMIUM, 2013-03-14 00:25:50]

On 2013/03/14 00:14:01, darin wrote:
>
https://codereview.chromium.org/12713004/diff/17001/webkit/glue/webkitplatfor...
> File webkit/glue/webkitplatformsupport_impl.h (right):
> 
>
https://codereview.chromium.org/12713004/diff/17001/webkit/glue/webkitplatfor...
> webkit/glue/webkitplatformsupport_impl.h:130: virtual bool
> isTypeSupportedForMediaSource(
> Why isn't this a function on WebMIMERegistry?
Because I didn't realize that was the place to put it. :) I'll rework these
changes & the WebKit ones to use the WebMIMERegistry. Thanks.

[darin (slow to review), 2013-03-14 00:36:58]

Cool =)


On Wed, Mar 13, 2013 at 5:25 PM, <acolwell@chromium.org> wrote:

> On 2013/03/14 00:14:01, darin wrote:
>
> https://codereview.chromium.**org/12713004/diff/17001/**webkit/glue/**
>
webkitplatformsupport_impl.h<https://codereview.chromium.org/12713004/diff/17001/webkit/glue/webkitplatformsupport_impl.h>
>
>> File webkit/glue/**webkitplatformsupport_impl.h (right):
>>
>
>
> https://codereview.chromium.**org/12713004/diff/17001/**webkit/glue/**
>
webkitplatformsupport_impl.h#**newcode130<https://codereview.chromium.org/12713004/diff/17001/webkit/glue/webkitplatformsupport_impl.h#newcode130>
>
>> webkit/glue/**webkitplatformsupport_impl.h:**130: virtual bool
>> isTypeSupportedForMediaSource(
>> Why isn't this a function on WebMIMERegistry?
>>
> Because I didn't realize that was the place to put it. :) I'll rework these
> changes & the WebKit ones to use the WebMIMERegistry. Thanks.
>
>
https://codereview.chromium.**org/12713004/<https://codereview.chromium.org/1...
>

[darin (slow to review), 2013-03-14 00:37:38]

You might also think about a name that fits the model of the others...
"supportsBlahBlah"


On Wed, Mar 13, 2013 at 5:36 PM, Darin Fisher <darin@chromium.org> wrote:

> Cool =)
>
>
> On Wed, Mar 13, 2013 at 5:25 PM, <acolwell@chromium.org> wrote:
>
>> On 2013/03/14 00:14:01, darin wrote:
>>
>> https://codereview.chromium.**org/12713004/diff/17001/**webkit/glue/**
>>
webkitplatformsupport_impl.h<https://codereview.chromium.org/12713004/diff/17001/webkit/glue/webkitplatformsupport_impl.h>
>>
>>> File webkit/glue/**webkitplatformsupport_impl.h (right):
>>>
>>
>>
>> https://codereview.chromium.**org/12713004/diff/17001/**webkit/glue/**
>>
webkitplatformsupport_impl.h#**newcode130<https://codereview.chromium.org/12713004/diff/17001/webkit/glue/webkitplatformsupport_impl.h#newcode130>
>>
>>> webkit/glue/**webkitplatformsupport_impl.h:**130: virtual bool
>>> isTypeSupportedForMediaSource(
>>> Why isn't this a function on WebMIMERegistry?
>>>
>> Because I didn't realize that was the place to put it. :) I'll rework
>> these
>> changes & the WebKit ones to use the WebMIMERegistry. Thanks.
>>
>>
https://codereview.chromium.**org/12713004/<https://codereview.chromium.org/1...
>>
>
>

[scherkus (not reviewing), 2013-03-14 01:15:26]

https://codereview.chromium.org/12713004/diff/17001/media/filters/stream_pars...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/12713004/diff/17001/media/filters/stream_pars...
media/filters/stream_parser_factory.cc:10: #if defined(GOOGLE_CHROME_BUILD) ||
defined(USE_PROPRIETARY_CODECS)
we typically have these #if #includes after all the #includes and not inline

https://codereview.chromium.org/12713004/diff/17001/media/filters/stream_pars...
File media/filters/stream_parser_factory.h (right):

https://codereview.chromium.org/12713004/diff/17001/media/filters/stream_pars...
media/filters/stream_parser_factory.h:14: #include "media/base/stream_parser.h"
fwd decl instead of #include?

https://codereview.chromium.org/12713004/diff/17001/media/filters/stream_pars...
media/filters/stream_parser_factory.h:18: class StreamParserFactory {
can we MEDIA_EXPORT here instead?

https://codereview.chromium.org/12713004/diff/17001/webkit/media/webmediasour...
File webkit/media/webmediasourceclient_impl.cc (right):

https://codereview.chromium.org/12713004/diff/17001/webkit/media/webmediasour...
webkit/media/webmediasourceclient_impl.cc:129: demuxer_->AddParser(id,
stream_parser.Pass(), has_audio, has_video));
where is ChunkDemuxer::AddParser() defined? I'm not seeing it anywhere :\

(I was curious if has_audio/has_video can be part of stream_parser instead of
being carried around along the side as out-params in SPF::Create() and
CD::AddParser())

[acolwell GONE FROM CHROMIUM, 2013-03-14 18:34:34]

Addressed comments and updated code to expose the functionality through
WebMimeRegistry.

https://codereview.chromium.org/12713004/diff/17001/media/filters/stream_pars...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/12713004/diff/17001/media/filters/stream_pars...
media/filters/stream_parser_factory.cc:10: #if defined(GOOGLE_CHROME_BUILD) ||
defined(USE_PROPRIETARY_CODECS)
On 2013/03/14 01:15:26, scherkus wrote:
> we typically have these #if #includes after all the #includes and not inline

Done.

https://codereview.chromium.org/12713004/diff/17001/media/filters/stream_pars...
File media/filters/stream_parser_factory.h (right):

https://codereview.chromium.org/12713004/diff/17001/media/filters/stream_pars...
media/filters/stream_parser_factory.h:14: #include "media/base/stream_parser.h"
On 2013/03/14 01:15:26, scherkus wrote:
> fwd decl instead of #include?

Done.

https://codereview.chromium.org/12713004/diff/17001/media/filters/stream_pars...
media/filters/stream_parser_factory.h:18: class StreamParserFactory {
On 2013/03/14 01:15:26, scherkus wrote:
> can we MEDIA_EXPORT here instead?

Done.

https://codereview.chromium.org/12713004/diff/17001/webkit/glue/webkitplatfor...
File webkit/glue/webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/12713004/diff/17001/webkit/glue/webkitplatfor...
webkit/glue/webkitplatformsupport_impl.h:130: virtual bool
isTypeSupportedForMediaSource(
On 2013/03/14 00:14:01, darin wrote:
> Why isn't this a function on WebMIMERegistry?

Done.

https://codereview.chromium.org/12713004/diff/17001/webkit/media/webmediasour...
File webkit/media/webmediasourceclient_impl.cc (right):

https://codereview.chromium.org/12713004/diff/17001/webkit/media/webmediasour...
webkit/media/webmediasourceclient_impl.cc:129: demuxer_->AddParser(id,
stream_parser.Pass(), has_audio, has_video));
On 2013/03/14 01:15:26, scherkus wrote:
> where is ChunkDemuxer::AddParser() defined? I'm not seeing it anywhere :\
> 
> (I was curious if has_audio/has_video can be part of stream_parser instead of
> being carried around along the side as out-params in SPF::Create() and
> CD::AddParser())
This was accidentally left in for a refactor that I later decided to revert.
I've put this back to AddId() for now.

The has_audio/has_video information doesn't really belong in the StreamParser.
It is only used by the ChunkDemuxer to enforce one video and one audio stream. I
expect this to go away in a later refactoring CL where I extract the
stream_parser and SourceBufferStream code from ChunkDemuxer and move it here.

[scherkus (not reviewing), 2013-03-14 18:49:40]

one q

https://codereview.chromium.org/12713004/diff/56001/media/filters/stream_pars...
File media/filters/stream_parser_factory.h (right):

https://codereview.chromium.org/12713004/diff/56001/media/filters/stream_pars...
media/filters/stream_parser_factory.h:19: class MEDIA_EXPORT StreamParserFactory
{
needs docs for funcs (maybe move the ones in the .cc to the .h?)

https://codereview.chromium.org/12713004/diff/56001/webkit/glue/simple_webmim...
File webkit/glue/simple_webmimeregistry_impl.cc (right):

https://codereview.chromium.org/12713004/diff/56001/webkit/glue/simple_webmim...
webkit/glue/simple_webmimeregistry_impl.cc:118: return
webkit_media::WebMediaSourceClientImpl::isTypeSupported(
why not call media::StreamParserFactory::IsTypeSupported(type, codecs) directly
instead of going through WMSCImpl?

[acolwell GONE FROM CHROMIUM, 2013-03-14 20:08:49]

https://codereview.chromium.org/12713004/diff/56001/media/filters/stream_pars...
File media/filters/stream_parser_factory.h (right):

https://codereview.chromium.org/12713004/diff/56001/media/filters/stream_pars...
media/filters/stream_parser_factory.h:19: class MEDIA_EXPORT StreamParserFactory
{
On 2013/03/14 18:49:40, scherkus wrote:
> needs docs for funcs (maybe move the ones in the .cc to the .h?)

Done.

https://codereview.chromium.org/12713004/diff/56001/webkit/glue/simple_webmim...
File webkit/glue/simple_webmimeregistry_impl.cc (right):

https://codereview.chromium.org/12713004/diff/56001/webkit/glue/simple_webmim...
webkit/glue/simple_webmimeregistry_impl.cc:118: return
webkit_media::WebMediaSourceClientImpl::isTypeSupported(
On 2013/03/14 18:49:40, scherkus wrote:
> why not call media::StreamParserFactory::IsTypeSupported(type, codecs)
directly
> instead of going through WMSCImpl?
git cl upload said "no!" I think it is because code in "webkit/glue" is not
allowed to depend on stuff in "media/". It explicitly complained about this.

https://codereview.chromium.org/12713004/diff/52007/media/filters/stream_pars...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/12713004/diff/52007/media/filters/stream_pars...
media/filters/stream_parser_factory.cc:61: #if defined(OS_ANDROID)
I plan on getting rid of these shortly, just not in this CL. I plan on removing
any FFmpeg dependencies in ChunkDemuxer & WebMStreamParser & WebMClusterParser
so that this isn't necessary.

[scherkus (not reviewing), 2013-03-14 20:21:03]

https://codereview.chromium.org/12713004/diff/56001/webkit/glue/simple_webmim...
File webkit/glue/simple_webmimeregistry_impl.cc (right):

https://codereview.chromium.org/12713004/diff/56001/webkit/glue/simple_webmim...
webkit/glue/simple_webmimeregistry_impl.cc:118: return
webkit_media::WebMediaSourceClientImpl::isTypeSupported(
On 2013/03/14 20:08:49, acolwell wrote:
> On 2013/03/14 18:49:40, scherkus wrote:
> > why not call media::StreamParserFactory::IsTypeSupported(type, codecs)
> directly
> > instead of going through WMSCImpl?
> git cl upload said "no!" I think it is because code in "webkit/glue" is not
> allowed to depend on stuff in "media/". It explicitly complained about this.

well that's because media/ isn't in webkit/glue/DEPS but you're working around
it via webkit/media/DEPS

I don't think the indirection is doing us much -- I think poking a small hole in
webkit/glue/DEPS to depend on media/filters/stream_parser_factory.h should be
fine (darin: please shout if you disagree!)

[acolwell GONE FROM CHROMIUM, 2013-03-14 20:56:35]

https://codereview.chromium.org/12713004/diff/56001/webkit/glue/simple_webmim...
File webkit/glue/simple_webmimeregistry_impl.cc (right):

https://codereview.chromium.org/12713004/diff/56001/webkit/glue/simple_webmim...
webkit/glue/simple_webmimeregistry_impl.cc:118: return
webkit_media::WebMediaSourceClientImpl::isTypeSupported(
On 2013/03/14 20:21:03, scherkus wrote:
> On 2013/03/14 20:08:49, acolwell wrote:
> > On 2013/03/14 18:49:40, scherkus wrote:
> > > why not call media::StreamParserFactory::IsTypeSupported(type, codecs)
> > directly
> > > instead of going through WMSCImpl?
> > git cl upload said "no!" I think it is because code in "webkit/glue" is not
> > allowed to depend on stuff in "media/". It explicitly complained about this.
> 
> well that's because media/ isn't in webkit/glue/DEPS but you're working around
> it via webkit/media/DEPS
> 
> I don't think the indirection is doing us much -- I think poking a small hole
in
> webkit/glue/DEPS to depend on media/filters/stream_parser_factory.h should be
> fine (darin: please shout if you disagree!)
Done.

[scherkus (not reviewing), 2013-03-14 22:08:33]

lgtm but I would tighten up that DEPS rule so other stuff doesn't creep in
On Mar 14, 2013 1:56 PM, <acolwell@chromium.org> wrote:

>
> https://codereview.chromium.**org/12713004/diff/56001/**
>
webkit/glue/simple_**webmimeregistry_impl.cc<https://codereview.chromium.org/12713004/diff/56001/webkit/glue/simple_webmimeregistry_impl.cc>
> File webkit/glue/simple_**webmimeregistry_impl.cc (right):
>
> https://codereview.chromium.**org/12713004/diff/56001/**
>
webkit/glue/simple_**webmimeregistry_impl.cc#**newcode118<https://codereview.chromium.org/12713004/diff/56001/webkit/glue/simple_webmimeregistry_impl.cc#newcode118>
> webkit/glue/simple_**webmimeregistry_impl.cc:118: return
> webkit_media::**WebMediaSourceClientImpl::**isTypeSupported(
> On 2013/03/14 20:21:03, scherkus wrote:
>
>> On 2013/03/14 20:08:49, acolwell wrote:
>> > On 2013/03/14 18:49:40, scherkus wrote:
>> > > why not call media::StreamParserFactory::**IsTypeSupported(type,
>>
> codecs)
>
>> > directly
>> > > instead of going through WMSCImpl?
>> > git cl upload said "no!" I think it is because code in "webkit/glue"
>>
> is not
>
>> > allowed to depend on stuff in "media/". It explicitly complained
>>
> about this.
>
>  well that's because media/ isn't in webkit/glue/DEPS but you're
>>
> working around
>
>> it via webkit/media/DEPS
>>
>
>  I don't think the indirection is doing us much -- I think poking a
>>
> small hole in
>
>> webkit/glue/DEPS to depend on media/filters/stream_parser_**factory.h
>>
> should be
>
>> fine (darin: please shout if you disagree!)
>>
> Done.
>
>
https://codereview.chromium.**org/12713004/<https://codereview.chromium.org/1...
>

[acolwell GONE FROM CHROMIUM, 2013-03-15 00:10:48]

On 2013/03/14 22:08:33, scherkus wrote:
> lgtm but I would tighten up that DEPS rule so other stuff doesn't creep in

Done.

[acolwell GONE FROM CHROMIUM, 2013-03-15 16:42:03]

darin@ could you please look at the content/ and webkit/glue changes and give me
an owners LGTM.

Thanks.

[darin (slow to review), 2013-03-15 21:25:52]

LGTM

https://codereview.chromium.org/12713004/diff/80001/webkit/glue/DEPS
File webkit/glue/DEPS (right):

https://codereview.chromium.org/12713004/diff/80001/webkit/glue/DEPS#newcode3
webkit/glue/DEPS:3: "+media/filters/stream_parser_factory.h",
is there a reason to be so granular here?  why is this the only valid include
from media/filters?

[acolwell GONE FROM CHROMIUM, 2013-03-15 21:34:18]

https://codereview.chromium.org/12713004/diff/80001/webkit/glue/DEPS
File webkit/glue/DEPS (right):

https://codereview.chromium.org/12713004/diff/80001/webkit/glue/DEPS#newcode3
webkit/glue/DEPS:3: "+media/filters/stream_parser_factory.h",
On 2013/03/15 21:25:52, darin wrote:
> is there a reason to be so granular here?  why is this the only valid include
> from media/filters?
No. It was just the only header this directory needed. I've changed this to
media/filters.

[commit-bot: I haz the power, 2013-03-15 21:35:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/12713004/86001

[commit-bot: I haz the power, 2013-03-15 23:54:54]

Change committed as 188512