MSE: Start letting SourceBuffer begin to do initialization segment received algorithm

MSE: Start letting SourceBuffer begin to do initialization segment received algorithm

This change is the first in a three-sided set of changes, and introduces
WebSourceBufferClient for handling media engine notifications
of newly received initialization segments.
It adds logic to make the addition of a SourceBuffer into
MediaSource.activeSourceBuffers more compliant once all three sides
land: addition will be done per the MSE spec's first initialization
flag logic rather than other non-compliant approaches like at
addSourceBuffer() time or on transition to HAVE_METADATA. Insertion
logic is included to preserve correct ordering of
activeSourceBuffers.
Updated and new layout tests are included.

R=acolwell@chromium.org,philipj@opera.com,tkent@chromium.org
TEST=No mediasource layout test regression
BUG=249428, 397243
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181861

[acolwell GONE FROM CHROMIUM, 2014-09-09 22:23:22]

This looks good to me w/ a few minor nits.

https://codereview.chromium.org/552943002/diff/20001/Source/modules/mediasour...
File Source/modules/mediasource/MediaSource.cpp (right):

https://codereview.chromium.org/552943002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/MediaSource.cpp:496: if
(m_sourceBuffers->find(m_activeSourceBuffers->item(insertPosition)) >=
indexInSourceBuffers)
nit: Reverse and merge into while condition?

https://codereview.chromium.org/552943002/diff/20001/Source/modules/mediasour...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/552943002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:119: if (m_webSourceBuffer)
nit: I don't think you'll need this. You should always get a
removedFromMediaSource() call before this destructor call.

https://codereview.chromium.org/552943002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:424:
m_webSourceBuffer->setClient(0);
nit: Seems like this could be part of removedFromMediaSource(). In otherwords
the client shouldn't be called after removedFromMediaSource() is called.

[wolenetz, 2014-09-10 00:30:10]

wolenetz@chromium.org changed reviewers:
+ abarth@chromium.org

[wolenetz, 2014-09-10 00:42:29]

PTAL @ PS4:
acolwell@, philipj@: everything (Thank you for earlier reviews, acolwell@!)
abarth@: OWNERS review for public/

This is first of 3-sided blink->chromium->blink set of changes:
Part 1: https://codereview.chromium.org/552943002/
Part 2: https://codereview.chromium.org/547223003/
Part 3: https://codereview.chromium.org/562503002/

damienv@, gunsch@: FYI - this set of changes should not regress Chromium
mp2ts_stream_parser. Please be aware that MSE spec doesn't seem to allow for
SourceBuffer.abort() to trigger initialization segment received algorithm as
part of emitting any in-parser coded frames during abort, so I have not included
such support in the chromium CL (part 2 of this set).

https://codereview.chromium.org/552943002/diff/20001/Source/modules/mediasour...
File Source/modules/mediasource/MediaSource.cpp (right):

https://codereview.chromium.org/552943002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/MediaSource.cpp:496: if
(m_sourceBuffers->find(m_activeSourceBuffers->item(insertPosition)) >=
indexInSourceBuffers)
On 2014/09/09 22:23:21, acolwell wrote:
> nit: Reverse and merge into while condition?

Done.

https://codereview.chromium.org/552943002/diff/20001/Source/modules/mediasour...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/552943002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:119: if (m_webSourceBuffer)
On 2014/09/09 22:23:21, acolwell wrote:
> nit: I don't think you'll need this. You should always get a
> removedFromMediaSource() call before this destructor call.

Done.

https://codereview.chromium.org/552943002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:424:
m_webSourceBuffer->setClient(0);
On 2014/09/09 22:23:21, acolwell wrote:
> nit: Seems like this could be part of removedFromMediaSource(). In otherwords
> the client shouldn't be called after removedFromMediaSource() is called.

Done.

[acolwell GONE FROM CHROMIUM, 2014-09-10 17:16:27]

lgtm % nits

https://codereview.chromium.org/552943002/diff/60001/LayoutTests/http/tests/m...
File
LayoutTests/http/tests/media/media-source/mediasource-append-legacystream.html
(right):

https://codereview.chromium.org/552943002/diff/60001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-append-legacystream.html:18:
assert_equals(xhr.responseType, 'legacystream', 'Verify response type was
set.');
nit: Please don't update do the " -> ' conversion in this CL.

https://codereview.chromium.org/552943002/diff/60001/LayoutTests/http/tests/m...
File LayoutTests/http/tests/media/media-source/mediasource-sourcebufferlist.html
(right):

https://codereview.chromium.org/552943002/diff/60001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-sourcebufferlist.html:15:
assert_equals(mediaSource.sourceBuffers.length, expected.length, 'sourceBuffers
length');
ditto re:  " -> '

https://codereview.chromium.org/552943002/diff/60001/LayoutTests/http/tests/m...
File LayoutTests/http/tests/media/media-source/mediasource-util.js (right):

https://codereview.chromium.org/552943002/diff/60001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-util.js:363:
window['mediasource_testafterdataloaded_internal'] = function(testFunction,
description, failOnMediaElementError, options)
Don't make this available on the window object since this is an internal method
now.

https://codereview.chromium.org/552943002/diff/60001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-util.js:390:
window['mediasource_testafterdataloaded_nofailonerror'] = function(testFunction,
description, options)
any reason not to use |options| for this instead of creating a different method?

https://codereview.chromium.org/552943002/diff/60001/Source/modules/mediasour...
File Source/modules/mediasource/MediaSource.cpp (right):

https://codereview.chromium.org/552943002/diff/60001/Source/modules/mediasour...
Source/modules/mediasource/MediaSource.cpp:502: &&
m_sourceBuffers->find(m_activeSourceBuffers->item(insertPosition)) <
indexInSourceBuffers)
nit: Add {} since this is a multiline statement.

https://codereview.chromium.org/552943002/diff/60001/Source/modules/mediasour...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/552943002/diff/60001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:437: if
(!m_firstInitializationSegment) {
nit: s/m_first/m_receivedFirst/ to avoid confusion. I know this matches the
spec, but the name is confusing and I'm going to update the spec.

https://codereview.chromium.org/552943002/diff/60001/public/platform/WebSourc...
File public/platform/WebSourceBuffer.h (right):

https://codereview.chromium.org/552943002/diff/60001/public/platform/WebSourc...
public/platform/WebSourceBuffer.h:49: virtual void
setClient(WebSourceBufferClient*)
nit: Add a comment indicating this will only be called once and only with a
valid pointer.

https://codereview.chromium.org/552943002/diff/60001/public/platform/WebSourc...
public/platform/WebSourceBuffer.h:71: virtual void removedFromMediaSource() = 0;
nit: Add a comment indicating that the client passed to setClient() should never
be called after this method is called.

[wolenetz, 2014-09-10 21:18:22]

philipj@ and abarth@, PTAL @ PS5.
philipj: everything
abarth: public/

Thanks!

https://codereview.chromium.org/552943002/diff/60001/LayoutTests/http/tests/m...
File
LayoutTests/http/tests/media/media-source/mediasource-append-legacystream.html
(right):

https://codereview.chromium.org/552943002/diff/60001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-append-legacystream.html:18:
assert_equals(xhr.responseType, 'legacystream', 'Verify response type was
set.');
On 2014/09/10 17:16:26, acolwell wrote:
> nit: Please don't update do the " -> ' conversion in this CL.

Done.

https://codereview.chromium.org/552943002/diff/60001/LayoutTests/http/tests/m...
File LayoutTests/http/tests/media/media-source/mediasource-sourcebufferlist.html
(right):

https://codereview.chromium.org/552943002/diff/60001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-sourcebufferlist.html:15:
assert_equals(mediaSource.sourceBuffers.length, expected.length, 'sourceBuffers
length');
On 2014/09/10 17:16:26, acolwell wrote:
> ditto re:  " -> '

Done.

https://codereview.chromium.org/552943002/diff/60001/LayoutTests/http/tests/m...
File LayoutTests/http/tests/media/media-source/mediasource-util.js (right):

https://codereview.chromium.org/552943002/diff/60001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-util.js:363:
window['mediasource_testafterdataloaded_internal'] = function(testFunction,
description, failOnMediaElementError, options)
On 2014/09/10 17:16:26, acolwell wrote:
> Don't make this available on the window object since this is an internal
method
> now.

Done (see next comment).

https://codereview.chromium.org/552943002/diff/60001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-util.js:390:
window['mediasource_testafterdataloaded_nofailonerror'] = function(testFunction,
description, options)
On 2014/09/10 17:16:26, acolwell wrote:
> any reason not to use |options| for this instead of creating a different
method?

Done (renamed options->properties to match async_test() naming, and added a
recognizer for a new property to the original mediasource_testafterdataloaded()
function). So long as there's no name collision on this new property vs
test_harness.js over time, code is simplified.

https://codereview.chromium.org/552943002/diff/60001/Source/modules/mediasour...
File Source/modules/mediasource/MediaSource.cpp (right):

https://codereview.chromium.org/552943002/diff/60001/Source/modules/mediasour...
Source/modules/mediasource/MediaSource.cpp:502: &&
m_sourceBuffers->find(m_activeSourceBuffers->item(insertPosition)) <
indexInSourceBuffers)
On 2014/09/10 17:16:26, acolwell wrote:
> nit: Add {} since this is a multiline statement.

Done.

https://codereview.chromium.org/552943002/diff/60001/Source/modules/mediasour...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/552943002/diff/60001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:437: if
(!m_firstInitializationSegment) {
On 2014/09/10 17:16:27, acolwell wrote:
> nit: s/m_first/m_receivedFirst/ to avoid confusion. I know this matches the
> spec, but the name is confusing and I'm going to update the spec.

Done as s/Segment/SegmentReceived/ to match the spec update.

https://codereview.chromium.org/552943002/diff/60001/public/platform/WebSourc...
File public/platform/WebSourceBuffer.h (right):

https://codereview.chromium.org/552943002/diff/60001/public/platform/WebSourc...
public/platform/WebSourceBuffer.h:49: virtual void
setClient(WebSourceBufferClient*)
On 2014/09/10 17:16:27, acolwell wrote:
> nit: Add a comment indicating this will only be called once and only with a
> valid pointer.

Done.

https://codereview.chromium.org/552943002/diff/60001/public/platform/WebSourc...
public/platform/WebSourceBuffer.h:71: virtual void removedFromMediaSource() = 0;
On 2014/09/10 17:16:27, acolwell wrote:
> nit: Add a comment indicating that the client passed to setClient() should
never
> be called after this method is called.

Done.

[philipj_slow, 2014-09-11 13:02:29]

It's great if you can start the trybots to catch any failing tests early. (I
usually do git cl upload; git cl try.)

[philipj_slow, 2014-09-11 13:38:32]

LGTM

I've read through the code and skimmed the tests, but since I haven't worked
with this code myself you'll have to forgive the not so insightful questions. It
might take a few MediaSource reviews before I'm able to actually find any bugs
for you :)

https://codereview.chromium.org/552943002/diff/70001/Source/modules/mediasour...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/552943002/diff/70001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:117: ASSERT(!m_webSourceBuffer);
Is this a drive-by fix, or did something change so that this assert now holds
true? I don't see it.

https://codereview.chromium.org/552943002/diff/70001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:432: // FIXME: Make steps 1-7
synchronous with this call.
Yeah, that algorithm does a lot that's apparently done elsewhere, or not at all
:)

https://codereview.chromium.org/552943002/diff/70001/public/platform/WebSourc...
File public/platform/WebSourceBuffer.h (right):

https://codereview.chromium.org/552943002/diff/70001/public/platform/WebSourc...
public/platform/WebSourceBuffer.h:49: // This will only be called once and only
with a valid pointer.
Maybe say that it's never null, since valid is a bit vague and could mean just
not stale?

https://codereview.chromium.org/552943002/diff/70001/public/platform/WebSourc...
public/platform/WebSourceBuffer.h:72: // The client passed to setClient() should
never be called after this method
I don't understand this comment. Does setClient() transfer ownership of the
WebSourceBufferClient to the WebSourceBuffer, but one is allowed to borrow a
reference until the removedFromMediaSource() call?

[wolenetz, 2014-09-11 21:41:17]

abarth@, PTAL @ patch set 6, as OWNERS reviewer for public/

philipj@: I believe I've addressed all of your comments and have your l*tm, so
will land this upon public/ OWNERS review by an owner.
I usually bias towards overdoing git-cl-try, this CL was an exception. I'm
sorry. I'm happy to have you as MSE reviewer! The Blink code is fairly
straightforward. The complexity for me is adapting/fixing the Chromium code as
the Blink code becomes compliant with the spec.

https://codereview.chromium.org/552943002/diff/70001/Source/modules/mediasour...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/552943002/diff/70001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:117: ASSERT(!m_webSourceBuffer);
On 2014/09/11 13:38:32, philipj wrote:
> Is this a drive-by fix, or did something change so that this assert now holds
> true? I don't see it.

This is a drive-by for added clarification of |m_webSourceBuffer|'s non-null
lifetime. If isRemoved() assertion fails, above, this assertion will also fail
with current code, since removedFromMediaSource() clears both. I'm fine with
removing this line if you wish.

https://codereview.chromium.org/552943002/diff/70001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:432: // FIXME: Make steps 1-7
synchronous with this call.
On 2014/09/11 13:38:32, philipj wrote:
> Yeah, that algorithm does a lot that's apparently done elsewhere, or not at
all
> :)

I'll be working on subsequent CLs that improve compliance around this. Chromium
currently hops threads to set duration and transition to HAVE_METADATA as a
result of receiving init segment(s). Both, along with adding the necessary
tracks, would best be done here now that it's plumbed into Blink. |m_source|
knows the element it's attached to, so I can remove those Chromium thread hops
(and centralize in a maintainable, readable location the core of the
initSegmentReceived algorithm.)

https://codereview.chromium.org/552943002/diff/70001/public/platform/WebSourc...
File public/platform/WebSourceBuffer.h (right):

https://codereview.chromium.org/552943002/diff/70001/public/platform/WebSourc...
public/platform/WebSourceBuffer.h:49: // This will only be called once and only
with a valid pointer.
On 2014/09/11 13:38:32, philipj wrote:
> Maybe say that it's never null, since valid is a bit vague and could mean just
> not stale?

Done.

https://codereview.chromium.org/552943002/diff/70001/public/platform/WebSourc...
public/platform/WebSourceBuffer.h:72: // The client passed to setClient() should
never be called after this method
On 2014/09/11 13:38:32, philipj wrote:
> I don't understand this comment. Does setClient() transfer ownership of the
> WebSourceBufferClient to the WebSourceBuffer, but one is allowed to borrow a
> reference until the removedFromMediaSource() call?

I've clarified this a bit more. I'm trying to indicate that there is no
ownership of the WebSourceBufferClient transferred to the WebSourceBuffer by
setClient(). setClient() passes a raw pointer. The WebSourceBuffer should not
use this raw pointer after removedFromMediaSource() is called; the pointer is
only 'valid' between setClient() and removedFromMediaSource(). WebSourceBuffer
does not own the client object.

[wolenetz, 2014-09-11 22:56:18]

wolenetz@chromium.org changed reviewers:
+ jamesr@chromium.org
- abarth@chromium.org

[wolenetz, 2014-09-11 23:09:28]

wolenetz@chromium.org changed reviewers:
+ tkent@chromium.org
- jamesr@chromium.org

[wolenetz, 2014-09-11 23:12:22]

Both abarth@ and jamesr@ have indicated their unavailability to CR the
public/platform piece of this CL.
tkent@, please CR public/platform.
Thank you!

[tkent, 2014-09-12 00:25:48]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-09-12 00:25:49]

lgtm

[commit-bot: I haz the power, 2014-09-12 00:27:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/552943002/90001

[commit-bot: I haz the power, 2014-09-12 00:36:29]

Committed patchset #6 (id:90001) as 181861

[philipj_slow, 2014-09-12 08:45:28]

This has landed, just want to acknowledge that I'm happy with the changes.

https://codereview.chromium.org/552943002/diff/70001/Source/modules/mediasour...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/552943002/diff/70001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:117: ASSERT(!m_webSourceBuffer);
On 2014/09/11 21:41:17, wolenetz wrote:
> On 2014/09/11 13:38:32, philipj wrote:
> > Is this a drive-by fix, or did something change so that this assert now
holds
> > true? I don't see it.
> 
> This is a drive-by for added clarification of |m_webSourceBuffer|'s non-null
> lifetime. If isRemoved() assertion fails, above, this assertion will also fail
> with current code, since removedFromMediaSource() clears both. I'm fine with
> removing this line if you wish.

A little drive-by fixing is fine, just wanted to understand :)

https://codereview.chromium.org/552943002/diff/70001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:432: // FIXME: Make steps 1-7
synchronous with this call.
On 2014/09/11 21:41:17, wolenetz wrote:
> On 2014/09/11 13:38:32, philipj wrote:
> > Yeah, that algorithm does a lot that's apparently done elsewhere, or not at
> all
> > :)
> 
> I'll be working on subsequent CLs that improve compliance around this.
Chromium
> currently hops threads to set duration and transition to HAVE_METADATA as a
> result of receiving init segment(s). Both, along with adding the necessary
> tracks, would best be done here now that it's plumbed into Blink. |m_source|
> knows the element it's attached to, so I can remove those Chromium thread hops
> (and centralize in a maintainable, readable location the core of the
> initSegmentReceived algorithm.)

Looking forward to those changes!

https://codereview.chromium.org/552943002/diff/70001/public/platform/WebSourc...
File public/platform/WebSourceBuffer.h (right):

https://codereview.chromium.org/552943002/diff/70001/public/platform/WebSourc...
public/platform/WebSourceBuffer.h:72: // The client passed to setClient() should
never be called after this method
On 2014/09/11 21:41:17, wolenetz wrote:
> On 2014/09/11 13:38:32, philipj wrote:
> > I don't understand this comment. Does setClient() transfer ownership of the
> > WebSourceBufferClient to the WebSourceBuffer, but one is allowed to borrow a
> > reference until the removedFromMediaSource() call?
> 
> I've clarified this a bit more. I'm trying to indicate that there is no
> ownership of the WebSourceBufferClient transferred to the WebSourceBuffer by
> setClient(). setClient() passes a raw pointer. The WebSourceBuffer should not
> use this raw pointer after removedFromMediaSource() is called; the pointer is
> only 'valid' between setClient() and removedFromMediaSource(). WebSourceBuffer
> does not own the client object.

That's simple enough, and the updated comments are great, thanks!