Move ChunkDemuxer handling from WMPProxy to WMPI and remove ChunkDemuxerClient

Move ChunkDemuxer handling from WMPProxy to WMPI and remove ChunkDemuxerClient


BUG=148662
TEST=ChunkDemuxerTest, PipelineIntegrationTest, and Media Source LayoutTests all still pass.

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

[acolwell GONE FROM CHROMIUM, 2012-09-12 14:05:11]

Boom! Let the happy dance begin.

http://codereview.chromium.org/10905236/diff/9001/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10905236/diff/9001/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:114: static void OnDemuxerNeedKeyTrampoline(
This is necessary because the scoped_array<uint8> with media::BindToLoop makes
the compiler very angry. According to BindToLoop's docs this appears to be
expected.

[Ami GONE FROM CHROMIUM, 2012-09-12 18:45:34]

I like it!

http://codereview.chromium.org/10905236/diff/12002/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/10905236/diff/12002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:537: DCHECK(!open_cb.is_null());
s/cb/cb_/ (in general, this style makes  it easier to detect accidental
self-assignments in initializer lists)

http://codereview.chromium.org/10905236/diff/12002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:538: DCHECK(!need_key_cb.is_null());
ditto

http://codereview.chromium.org/10905236/diff/12002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:555: open_cb.Run();
Is this just good hygiene or is there a reason open_cb_ must be Run() not under
lock?

If need to retain the unlocked nature, does it make sense to instead do:
MessageLoop::current()->PostTask(FROM_HERE, base::ResetAndReturn(&open_cb_));
and drop the extra {}+indent?

http://codereview.chromium.org/10905236/diff/12002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:693: base::Bind(&ChunkDemuxer::OnNeedKey,
base::Unretained(this)),
possible to just pass need_key_cb_ here and drop OnNeedKey entirely?

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

http://codereview.chromium.org/10905236/diff/12002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.h:38: ChunkDemuxer(const base::Closure& open_cb,
const NeedKeyCB& need_key_cb);
doco when callbacks are called

http://codereview.chromium.org/10905236/diff/12002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.h:38: ChunkDemuxer(const base::Closure& open_cb,
const NeedKeyCB& need_key_cb);
is it easy to see why these are not needed in Demuxer itself (and thus belong in
Initialize() instead of the ctor)?

Put another way, do we want to support EME independent of MSE?

http://codereview.chromium.org/10905236/diff/12002/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/10905236/diff/12002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:408: if (demuxer_) {
braces unnecessary

http://codereview.chromium.org/10905236/diff/12002/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10905236/diff/12002/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:110: const std::string&,
what's with these std::strings??

http://codereview.chromium.org/10905236/diff/12002/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:270: base::Bind(&OnDemuxerNeedKeyTrampoline,
why not BIND_TO_RENDER_LOOP for this as well?

[acolwell GONE FROM CHROMIUM, 2012-09-12 22:03:53]

http://codereview.chromium.org/10905236/diff/12002/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/10905236/diff/12002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:537: DCHECK(!open_cb.is_null());
On 2012/09/12 18:45:34, Ami Fischman wrote:
> s/cb/cb_/ (in general, this style makes  it easier to detect accidental
> self-assignments in initializer lists)

Done.

http://codereview.chromium.org/10905236/diff/12002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:538: DCHECK(!need_key_cb.is_null());
On 2012/09/12 18:45:34, Ami Fischman wrote:
> ditto

Done.

http://codereview.chromium.org/10905236/diff/12002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:555: open_cb.Run();
On 2012/09/12 18:45:34, Ami Fischman wrote:
> Is this just good hygiene or is there a reason open_cb_ must be Run() not
under
> lock?

This was intended to be a good hygiene thing and was required by
PipelineIntegrationTest. I updated PipelineIntegrationTest to post to a message
loop like WMPI does. This allows me to call it under lock and deindent like you
suggested.

> 
> If need to retain the unlocked nature, does it make sense to instead do:
> MessageLoop::current()->PostTask(FROM_HERE, base::ResetAndReturn(&open_cb_));
> and drop the extra {}+indent?

http://codereview.chromium.org/10905236/diff/12002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:693: base::Bind(&ChunkDemuxer::OnNeedKey,
base::Unretained(this)),
On 2012/09/12 18:45:34, Ami Fischman wrote:
> possible to just pass need_key_cb_ here and drop OnNeedKey entirely?
Not yet. There is signature mismatch here that needs to be fixed in a separate
CL. Added a TODO for this.

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

http://codereview.chromium.org/10905236/diff/12002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.h:38: ChunkDemuxer(const base::Closure& open_cb,
const NeedKeyCB& need_key_cb);
On 2012/09/12 18:45:34, Ami Fischman wrote:
> doco when callbacks are called

Done.

http://codereview.chromium.org/10905236/diff/12002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.h:38: ChunkDemuxer(const base::Closure& open_cb,
const NeedKeyCB& need_key_cb);
On 2012/09/12 18:45:34, Ami Fischman wrote:
> is it easy to see why these are not needed in Demuxer itself (and thus belong
in
> Initialize() instead of the ctor)?
> 
> Put another way, do we want to support EME independent of MSE?
Eventually I believe the need_key_cb could be moved to Initialize(), but I think
that should be a separate CL since it would cause a new set of cascading changes
that aren't really related to getting rid of ChunkDemuxerClient.

http://codereview.chromium.org/10905236/diff/12002/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/10905236/diff/12002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:408: if (demuxer_) {
On 2012/09/12 18:45:34, Ami Fischman wrote:
> braces unnecessary

Done.

http://codereview.chromium.org/10905236/diff/12002/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10905236/diff/12002/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:110: const std::string&,
On 2012/09/12 18:45:34, Ami Fischman wrote:
> what's with these std::strings??
These are for the key_system and session_id. I'm not sure why they are always
empty strings for this code path. I'm just replicating the behavior that was in
WebMediaPlayerProxy::DemuxerNeedKey.

http://codereview.chromium.org/10905236/diff/12002/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:270: base::Bind(&OnDemuxerNeedKeyTrampoline,
On 2012/09/12 18:45:34, Ami Fischman wrote:
> why not BIND_TO_RENDER_LOOP for this as well?
BindToLoop doesn't like the scoped_array<> in the callback signature.
BindToLoop's docs explicitly say it can't be used with arrays because it copies
its arguments.

[Ami GONE FROM CHROMIUM, 2012-09-12 22:29:03]

lgtm

http://codereview.chromium.org/10905236/diff/12002/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10905236/diff/12002/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:110: const std::string&,
On 2012/09/12 22:03:53, acolwell wrote:
> On 2012/09/12 18:45:34, Ami Fischman wrote:
> > what's with these std::strings??
> These are for the key_system and session_id. I'm not sure why they are always
> empty strings for this code path. I'm just replicating the behavior that was
in
> WebMediaPlayerProxy::DemuxerNeedKey.

Feel like adding a TODO to figure out whether these need to die or whether they
are placeholders for future functionality?  (in which case they should probably
die, too, unless that future is almost upon us...)

http://codereview.chromium.org/10905236/diff/12002/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:270: base::Bind(&OnDemuxerNeedKeyTrampoline,
On 2012/09/12 22:03:53, acolwell wrote:
> On 2012/09/12 18:45:34, Ami Fischman wrote:
> > why not BIND_TO_RENDER_LOOP for this as well?
> BindToLoop doesn't like the scoped_array<> in the callback signature.
> BindToLoop's docs explicitly say it can't be used with arrays because it
copies
> its arguments.

This isn't an array, it's a scoped_array.
I'm surprised that no amount of base::Passed()'ing made this work for you.

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

http://codereview.chromium.org/10905236/diff/12002/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10905236/diff/12002/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:110: const std::string&,
On 2012/09/12 22:29:03, Ami Fischman wrote:
> On 2012/09/12 22:03:53, acolwell wrote:
> > On 2012/09/12 18:45:34, Ami Fischman wrote:
> > > what's with these std::strings??
> > These are for the key_system and session_id. I'm not sure why they are
always
> > empty strings for this code path. I'm just replicating the behavior that was
> in
> > WebMediaPlayerProxy::DemuxerNeedKey.
> 
> Feel like adding a TODO to figure out whether these need to die or whether
they
> are placeholders for future functionality?  (in which case they should
probably
> die, too, unless that future is almost upon us...)

Done.

http://codereview.chromium.org/10905236/diff/12002/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:270: base::Bind(&OnDemuxerNeedKeyTrampoline,
On 2012/09/12 22:29:03, Ami Fischman wrote:
> On 2012/09/12 22:03:53, acolwell wrote:
> > On 2012/09/12 18:45:34, Ami Fischman wrote:
> > > why not BIND_TO_RENDER_LOOP for this as well?
> > BindToLoop doesn't like the scoped_array<> in the callback signature.
> > BindToLoop's docs explicitly say it can't be used with arrays because it
> copies
> > its arguments.
> 
> This isn't an array, it's a scoped_array.
> I'm surprised that no amount of base::Passed()'ing made this work for you.
I was too. I'll left it as is for now and will change it if we figure out a way
to make it work with BindToLoop.