Implement sourceAddId() & sourceRemoveId()

Update MediaSource API implementation to match v0.4 spec.
 - Adding support for sourceAddId() & sourceRemoveId().
 - Adding id parameter to sourceAppend() signature.

BUG=122897
TEST=Covered by LayoutTests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132965

[scherkus (not reviewing), 2012-04-12 20:58:32]

http://codereview.chromium.org/10066019/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/10066019/diff/1/media/filters/chunk_demuxer.cc...
media/filters/chunk_demuxer.cc:461: ChunkDemuxer::AddIdStatus
ChunkDemuxer::AddId(const std::string& id,
potential spec-changing comment based on implementation experience coming up!

is the type checking intentionally supposed to differ from canPlayType() logic?
as in, the types supported by this API will be a strict subset of the types
supported by the regular engine?

is that what we want?

if it was required that they match (somehow, and with a bit of hand waving
here), it would seem like a good thing to keep clients using the same logic by
checking canPlayType() before feed in data (whether by setting src or media
source), and getting MediaErrors if you shoved in data that the engine doesn't
support

we don't need to change anything here today but rather food for thought (also I
don't want to have separate parsing code for these strings -- for example WebKit
parses the codecs= string for ports today where as in this model the port has to
parse the codecs= string -- not good!)

http://codereview.chromium.org/10066019/diff/1/media/filters/chunk_demuxer.h
File media/filters/chunk_demuxer.h (right):

http://codereview.chromium.org/10066019/diff/1/media/filters/chunk_demuxer.h#...
media/filters/chunk_demuxer.h:28: AisNoError,         // ID added w/o error.
I'm calling silliness on the naming here!

1) Why not "Status" -- are we going to have multiple status codes very soon? I
don't see anything in the spec that hints at additional status codes
2) Constants are kCamelCase
3) "Ais" is confusing: is it "A prefix, the word 'is'" (as in "A, isNoError") or
short for AddIdStatus (as in "AddIdStatus NoError")
4) kOK is a pithier way of saying AisNoError (bonus: matches the new status
codes added to BufferedResourceLoader!)

If we go with just "Status" we could clear up all this stuff by having:

enum Status {
  kOK,
  kNotSupported,
  kReachedIdLimit,
};

http://codereview.chromium.org/10066019/diff/1/media/filters/chunk_demuxer.h#...
media/filters/chunk_demuxer.h:53: // is called in an invalid state.
docs need updating

http://codereview.chromium.org/10066019/diff/1/webkit/media/webmediaplayer_im...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10066019/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:648: DCHECK(!id.isEmpty());
are these actually DCHECK worthy?

are we protected against WebKit or JS code? if JS code can call addId("", "")
and that code hits a DCHECK here that's bad and we should remove the DCHECK

http://codereview.chromium.org/10066019/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:651: return
static_cast<WebKit::WebMediaPlayer::AddIdStatus>(
do we have compile asserts to back the static_cast<> up?

check out COMPILE_ASSERT_MATCHING_ENUM in this file

http://codereview.chromium.org/10066019/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:670: DCHECK(!id.isEmpty());
ditto on these DCHECKs

http://codereview.chromium.org/10066019/diff/1/webkit/media/webmediaplayer_pr...
File webkit/media/webmediaplayer_proxy.cc (right):

http://codereview.chromium.org/10066019/diff/1/webkit/media/webmediaplayer_pr...
webkit/media/webmediaplayer_proxy.cc:194: return
media::ChunkDemuxer::AisNotSupported;
would we ever get to this case? ditto below

it seems like WebKit should prevent this by firing a bad state exception if the
element wasn't set up for media source, etc... similar to setting currentTime
fires INVALID_STATE_ERR if there's no src

the only other time chunk_demuxer_ would be NULL is if DemuxerClosedTask() was
executed -- at that point would it still be valid to call addID(), etc... ?


basically I'm on a mission to remove a lot of these "well, we might be deleted"
checks from WMPI/WMPP. Would you mind if we remove the checks for media source
and we implement things the right way? :)

[acolwell GONE FROM CHROMIUM, 2012-04-12 22:43:12]

http://codereview.chromium.org/10066019/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/10066019/diff/1/media/filters/chunk_demuxer.cc...
media/filters/chunk_demuxer.cc:461: ChunkDemuxer::AddIdStatus
ChunkDemuxer::AddId(const std::string& id,
On 2012/04/12 20:58:32, scherkus wrote:
> potential spec-changing comment based on implementation experience coming up!
> 
> is the type checking intentionally supposed to differ from canPlayType()
logic?

|type|'s primary function is to identify the format of the data that will be
appended. There may be extra parameters that are added to the mimetypes passed
here to indicate expected splicing behavior. The exact details will be worked
out when we address the "Capability Dectection" open issue in section 8.2

> as in, the types supported by this API will be a strict subset of the types
> supported by the regular engine?

Yes it will be a subset of the canPlayType() set.

> 
> is that what we want?
Yes. The engine supports WAV & MP3, but we don't necessarily want to support
those since they don't have timestamps embedded in the bytestream which at a
minimum means that seeking can't be properly supported.

> 
> if it was required that they match (somehow, and with a bit of hand waving
> here), it would seem like a good thing to keep clients using the same logic by
> checking canPlayType() before feed in data (whether by setting src or media
> source), and getting MediaErrors if you shoved in data that the engine doesn't
> support
canPlayType() can be used to discover whether a format is supported, but |type|
indicates what format will be appended for a particular ID. This could & likely
will be different on a per ID basis.

> 
> we don't need to change anything here today but rather food for thought (also
I
> don't want to have separate parsing code for these strings -- for example
WebKit
> parses the codecs= string for ports today where as in this model the port has
to
> parse the codecs= string -- not good!)

My plan was to add code to net/base/mime_util.h that parses parameters like
Webkit's ContentType. Depending on how the capability discussions go we may care
about parmeters other than just codecs so we'll need a more generic solution.

http://codereview.chromium.org/10066019/diff/1/media/filters/chunk_demuxer.h
File media/filters/chunk_demuxer.h (right):

http://codereview.chromium.org/10066019/diff/1/media/filters/chunk_demuxer.h#...
media/filters/chunk_demuxer.h:28: AisNoError,         // ID added w/o error.
On 2012/04/12 20:58:32, scherkus wrote:
> I'm calling silliness on the naming here!
> 
> 1) Why not "Status" -- are we going to have multiple status codes very soon? I
> don't see anything in the spec that hints at additional status codes
> 2) Constants are kCamelCase
> 3) "Ais" is confusing: is it "A prefix, the word 'is'" (as in "A, isNoError")
or
> short for AddIdStatus (as in "AddIdStatus NoError")
> 4) kOK is a pithier way of saying AisNoError (bonus: matches the new status
> codes added to BufferedResourceLoader!)
> 
> If we go with just "Status" we could clear up all this stuff by having:
> 
> enum Status {
>   kOK,
>   kNotSupported,
>   kReachedIdLimit,
> };

Done.

http://codereview.chromium.org/10066019/diff/1/media/filters/chunk_demuxer.h#...
media/filters/chunk_demuxer.h:53: // is called in an invalid state.
On 2012/04/12 20:58:32, scherkus wrote:
> docs need updating

Done.

http://codereview.chromium.org/10066019/diff/1/webkit/media/webmediaplayer_im...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10066019/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:648: DCHECK(!id.isEmpty());
On 2012/04/12 20:58:32, scherkus wrote:
> are these actually DCHECK worthy?
> 
> are we protected against WebKit or JS code? if JS code can call addId("", "")
> and that code hits a DCHECK here that's bad and we should remove the DCHECK

Done.

http://codereview.chromium.org/10066019/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:651: return
static_cast<WebKit::WebMediaPlayer::AddIdStatus>(
On 2012/04/12 20:58:32, scherkus wrote:
> do we have compile asserts to back the static_cast<> up?
> 
> check out COMPILE_ASSERT_MATCHING_ENUM in this file

Done.

http://codereview.chromium.org/10066019/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:670: DCHECK(!id.isEmpty());
On 2012/04/12 20:58:32, scherkus wrote:
> ditto on these DCHECKs

Done.

http://codereview.chromium.org/10066019/diff/1/webkit/media/webmediaplayer_pr...
File webkit/media/webmediaplayer_proxy.cc (right):

http://codereview.chromium.org/10066019/diff/1/webkit/media/webmediaplayer_pr...
webkit/media/webmediaplayer_proxy.cc:194: return
media::ChunkDemuxer::AisNotSupported;
On 2012/04/12 20:58:32, scherkus wrote:
> would we ever get to this case? ditto below
> 
> it seems like WebKit should prevent this by firing a bad state exception if
the
> element wasn't set up for media source, etc... similar to setting currentTime
> fires INVALID_STATE_ERR if there's no src
> 
> the only other time chunk_demuxer_ would be NULL is if DemuxerClosedTask() was
> executed -- at that point would it still be valid to call addID(), etc... ?
> 
> 
> basically I'm on a mission to remove a lot of these "well, we might be
deleted"
> checks from WMPI/WMPP. Would you mind if we remove the checks for media source
> and we implement things the right way? :)

I'll remove them and we'll see what happens. I put them in there a while ago
because I was running into cases where Append() was getting called when
chunk_demuxer_ was null. I believe it had something to do with error handling
where WebKit doesn't know about the error yet, the demuxer gets destroyed, and
JavaScript is running from an event handler and calls Append(). I don't remember
the exact scenario though.

If the crashes come back then I'll see if I can come up with a better solution
and capture the case in a unit test. :)

[scherkus (not reviewing), 2012-04-13 02:36:19]

LGTM w/ nit + reply!

http://codereview.chromium.org/10066019/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/10066019/diff/1/media/filters/chunk_demuxer.cc...
media/filters/chunk_demuxer.cc:461: ChunkDemuxer::AddIdStatus
ChunkDemuxer::AddId(const std::string& id,
On 2012/04/12 22:43:12, acolwell wrote:
> On 2012/04/12 20:58:32, scherkus wrote:
> > potential spec-changing comment based on implementation experience coming
up!
> > 
> > is the type checking intentionally supposed to differ from canPlayType()
> logic?
> 
> |type|'s primary function is to identify the format of the data that will be
> appended. There may be extra parameters that are added to the mimetypes passed
> here to indicate expected splicing behavior. The exact details will be worked
> out when we address the "Capability Dectection" open issue in section 8.2
> 
> > as in, the types supported by this API will be a strict subset of the types
> > supported by the regular engine?
> 
> Yes it will be a subset of the canPlayType() set.
> 
> > 
> > is that what we want?
> Yes. The engine supports WAV & MP3, but we don't necessarily want to support
> those since they don't have timestamps embedded in the bytestream which at a
> minimum means that seeking can't be properly supported.
> 
> > 
> > if it was required that they match (somehow, and with a bit of hand waving
> > here), it would seem like a good thing to keep clients using the same logic
by
> > checking canPlayType() before feed in data (whether by setting src or media
> > source), and getting MediaErrors if you shoved in data that the engine
doesn't
> > support
> canPlayType() can be used to discover whether a format is supported, but
|type|
> indicates what format will be appended for a particular ID. This could &
likely
> will be different on a per ID basis.
> 
> > 
> > we don't need to change anything here today but rather food for thought
(also
> I
> > don't want to have separate parsing code for these strings -- for example
> WebKit
> > parses the codecs= string for ports today where as in this model the port
has
> to
> > parse the codecs= string -- not good!)
> 
> My plan was to add code to net/base/mime_util.h that parses parameters like
> Webkit's ContentType. Depending on how the capability discussions go we may
care
> about parmeters other than just codecs so we'll need a more generic solution.

well said parsing code can still go into WK for all ports -- the embeddeer API
for adding an ID simply expands to include a few extra string args that include
pre-parsed args for things we care about (there's only so many ways to parse
those strings so why not share the wealth?)

but anyway we can tackle that in the future

http://codereview.chromium.org/10066019/diff/6001/media/filters/chunk_demuxer.h
File media/filters/chunk_demuxer.h (right):

http://codereview.chromium.org/10066019/diff/6001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.h:52: // the mimetype for the data that we intend to
append for this ID.
nit: MIME type

[acolwell GONE FROM CHROMIUM, 2012-04-13 16:23:09]

http://codereview.chromium.org/10066019/diff/6001/media/filters/chunk_demuxer.h
File media/filters/chunk_demuxer.h (right):

http://codereview.chromium.org/10066019/diff/6001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.h:52: // the mimetype for the data that we intend to
append for this ID.
On 2012/04/13 02:36:19, scherkus wrote:
> nit: MIME type

Done.

[commit-bot: I haz the power, 2012-04-19 06:16:14]

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

[commit-bot: I haz the power, 2012-04-19 08:07:37]

Change committed as 132965