Issue 1659653002: Pass MSE media track info from ChunkDemuxer to blink::SourceBuffer

servolk

Description was changed from ========== Added public WebMediaTrack interface and pass media track info to ...

4 years, 10 months ago (2016-02-18 23:27:24 UTC) #1

servolk

servolk@chromium.org changed reviewers: + philipj@opera.com, wolenetz@chromium.org

4 years, 10 months ago (2016-02-18 23:27:24 UTC) #2

philipj_slow

WebString.h has "BLINK_COMMON_EXPORT operator WTF::AtomicString() const" so it should be possible to convert it to ...

4 years, 10 months ago (2016-02-23 03:51:04 UTC) #4

philipj_slow

Issue 249427 is "Implement HTMLMediaElement.audioTracks & HTMLMediaElement.videoTracks", is this part of the effort to make ...

4 years, 10 months ago (2016-02-23 03:55:44 UTC) #5

philipj_slow

To elaborate some more, it would be nice if to the largest extent possible, MSE ...

4 years, 10 months ago (2016-02-23 03:57:23 UTC) #6

fs

On 2016/02/23 at 03:51:04, philipj wrote: > WebString.h has "BLINK_COMMON_EXPORT operator WTF::AtomicString() const" so it ...

4 years, 10 months ago (2016-02-23 11:18:25 UTC) #7

servolk

On 2016/02/23 03:57:23, philipj_UTC7 wrote: > To elaborate some more, it would be nice if ...

4 years, 10 months ago (2016-02-24 03:27:55 UTC) #8

servolk

On 2016/02/23 11:18:25, fs wrote: > On 2016/02/23 at 03:51:04, philipj wrote: > > WebString.h ...

4 years, 10 months ago (2016-02-24 03:39:41 UTC) #9

On 2016/02/23 11:18:25, fs wrote:
> On 2016/02/23 at 03:51:04, philipj wrote:
> > WebString.h has "BLINK_COMMON_EXPORT operator WTF::AtomicString() const" so
it
> should be possible to convert it to AtomicString.
> 
> I guess it might be pertinent to ask why these needs to be exposed at all -
i.e
> what's actually going to call the WebMediaTrack::wmt* methods (etc)? There's
> some question-marks wrt to lifetimes too it seems - maybe those are answered
> elsewhere? As for the createMediaTrack interface part, it's creating GC'd
> objects which is then promptly returned to the embedder (to just be passed
back
> again - and presumably acted upon?). While it's probably unlikely that GC will
> happen in that window it seems like bad form. It seems this could as well be
> achieved by a collection of "structs" of id/lang/etc that are then pushed to
> initializationSegmentReceived that creates and hooks up the tracks? (Then
maybe
> the existing add*Track interface could use the same struct too.)

Well, as I've explained in my previous comment, the MSE spec formulates the
'init segment received' algorithm in such a way that it assumes we'll have both
the old/current set of media tracks and the new set of media tracks (see
https://w3c.github.io/media-source/#sourcebuffer-init-segment-received). We will
need to have some classes/structs that contain the media track information both
on blink and Chromium levels (we need the on Chromium level, since actual media
track creation is initiated on Chromium level where we do the media stream
parsing). And so I wanted to reuse those existing structs as much as possible
instead of introducing additional structs just for passing data between the
blink and Chromium boundary. I tried to achieve that goal by adding a public
blink::WebMediaTrack interface that would be visible from the Chromium level.
Unfortunately, despite the 'operator WTF::AtomicString' I have been getting a
ton of compile errors when I tried to map WebString fields of
blink::WebMediaTrack directly to the various members of blink::TrackBase. I wish
there was a more straightforward way to achieve this, but this is the simplest
working solution that I've found so far.
I think we can solve the GC problem by holding references to the media tracks,
whose creation was requested from the Chromium level, in an internal temporary
collection in a SourceBuffer (e.g. SourceBuffer::m_incomingMediaTracks) and then
later, once the init segment analysis is finished on Chromium level, we could
use the m_incomingMediaTracks in the MSE init segment algorithm implementation
and move them to m_audio/videoTracks as necessary.

servolk

On 2016/02/24 03:27:55, servolk wrote: > On 2016/02/23 03:57:23, philipj_UTC7 wrote: > > To elaborate ...

4 years, 10 months ago (2016-02-24 05:12:44 UTC) #10

On 2016/02/24 03:27:55, servolk wrote:
> On 2016/02/23 03:57:23, philipj_UTC7 wrote:
> > To elaborate some more, it would be nice if to the largest extent possible,
> MSE
> > behaved like any other kind of WebMediaPlayer backend. Of course no other
kind
> > currently works either, but I'm keen to learn what extra bits might be
> required
> > for MSE that no other kind of backend (like plain HTTP playback) will
require.
> 
> Yeah, of course I understand that goal and I'd also love to unify the MSE and
> non-MSE code paths as much as possible, but unfortunately it's gonna be hard,
if
> possible at all. One of major differences, of course, is that MSE spec
describes
> the 'init segment received' algorithm in such a way that it's necessary to
have
> both the old set of media tracks and the new set of media tracks that are
> present in the newly received init segment (since the algorithm essentially
> requires us to compare those two sets in step #3
> https://w3c.github.io/media-source/#sourcebuffer-init-segment-received). Which
> is very different from the non-MSE use case currently implemented by
> HTMLMediaElement (which assumes that the player will keep only the up-to-date
> set of media tracks in the HTMLMediaElement). Perhaps we should untie the
> TrackList classes as much as possible from the HTMLMediaElement (currently
> TrackListBase has setMediaElement/mediaElement/owner methods, as well as some
> logic for sending track events on behalf of HTMLMediaElement, e.g.
> scheduleTrackEvent/scheduleChangeEvent).

I've spent some more time today looking into how we could unify handling media
tracks in MSE and non-MSE cases and I think something like this might work on
Chromium level:
https://codereview.chromium.org/1727243002/
But I'll wait to see what wolenetz@ and other media owners think about it.
If they are ok with that change, then we could have a similar interface for both
HTMLMediaElement and SourceBuffer, some callback that would let them know that
media track configuration has changed (but still it would probably have to
report the whole set of new media tracks in one invocation, instead of doing it
one-by-one, to simplify implementation of the spec-compliant MSE init segment
algorithm).

servolk

On 2016/02/24 05:12:44, servolk wrote: > On 2016/02/24 03:27:55, servolk wrote: > > On 2016/02/23 ...

4 years, 10 months ago (2016-02-25 02:30:53 UTC) #11

On 2016/02/24 05:12:44, servolk wrote:
> On 2016/02/24 03:27:55, servolk wrote:
> > On 2016/02/23 03:57:23, philipj_UTC7 wrote:
> > > To elaborate some more, it would be nice if to the largest extent
possible,
> > MSE
> > > behaved like any other kind of WebMediaPlayer backend. Of course no other
> kind
> > > currently works either, but I'm keen to learn what extra bits might be
> > required
> > > for MSE that no other kind of backend (like plain HTTP playback) will
> require.
> > 
> > Yeah, of course I understand that goal and I'd also love to unify the MSE
and
> > non-MSE code paths as much as possible, but unfortunately it's gonna be
hard,
> if
> > possible at all. One of major differences, of course, is that MSE spec
> describes
> > the 'init segment received' algorithm in such a way that it's necessary to
> have
> > both the old set of media tracks and the new set of media tracks that are
> > present in the newly received init segment (since the algorithm essentially
> > requires us to compare those two sets in step #3
> > https://w3c.github.io/media-source/#sourcebuffer-init-segment-received).
Which
> > is very different from the non-MSE use case currently implemented by
> > HTMLMediaElement (which assumes that the player will keep only the
up-to-date
> > set of media tracks in the HTMLMediaElement). Perhaps we should untie the
> > TrackList classes as much as possible from the HTMLMediaElement (currently
> > TrackListBase has setMediaElement/mediaElement/owner methods, as well as
some
> > logic for sending track events on behalf of HTMLMediaElement, e.g.
> > scheduleTrackEvent/scheduleChangeEvent).
> 
> I've spent some more time today looking into how we could unify handling media
> tracks in MSE and non-MSE cases and I think something like this might work on
> Chromium level:
> https://codereview.chromium.org/1727243002/
> But I'll wait to see what wolenetz@ and other media owners think about it.
> If they are ok with that change, then we could have a similar interface for
both
> HTMLMediaElement and SourceBuffer, some callback that would let them know that
> media track configuration has changed (but still it would probably have to
> report the whole set of new media tracks in one invocation, instead of doing
it
> one-by-one, to simplify implementation of the spec-compliant MSE init segment
> algorithm).

FYI: I've spent some more time today, and updated the CL
https://codereview.chromium.org/1727243002/, as well as implemented reading
track info from ffmpeg metadata and passing it to blink via
HTMLMediaElement::addAudioTrack/addVideoTrack
https://codereview.chromium.org/1735763002/.
In that latter CL I've also added two new blink-level LayoutTests to check
various properties of audio/videoTrack objects. Although currently some of those
properties are essentially hardcoded (e.g. track.id and .kind), still the
track.language is an honest value that is obtained from ffmpeg and passed all
the way up to blink, so it proves feasibility of this approach.
Also, if you prefer to see the total scope of changes, here is the combined CL
that has all the changes necessary for this:
https://codereview.chromium.org/1735803002/

philipj_slow

OK, I think I have some idea of the scope of the problem now. The ...

4 years, 10 months ago (2016-02-26 12:43:54 UTC) #12

blink-reviews

Yeah, currently some approximation of the init segment algorithm is implemented in Chromium media stack, ...

4 years, 9 months ago (2016-02-28 20:12:09 UTC) #13

Yeah, currently some approximation of the init segment algorithm is
implemented in Chromium media stack, but it's a. not fully spec compliant
b. not reusable by other blink clients. Wolenetz@ said that the spec
compliant init segment algorithm should be implemented on the blink level.
IIUC it would help with both of these points, it would help both make it
more spec compliant (since to make it fully spec compliant we need to have
this algorithm implemented where we have access to all the information
needed for it, and for example track defaults are currently only accessible
in blink, but not on Chromium demuxer/MediaSourceState level). And by the
virtue of being in blink it would be right away shareable/reusable by all
blink clients (also it might be easier to test that way). TBH I don't
really know how important these two point are, Matt can you add your
thoughts on this? Perhaps we could just pass track defaults information
down from the blink level into Chromium and that would make it easier to
implement the init segment algorithm on the Chromium media stack layer?
The course of further development would depend on this decision - if we
decide that we must have the init segment algorithm on blink level, we
would need something like what this CL does (passing media track info from
Chromium demuxers to blink level). If we decide to keep init segment alg on
Chromium level, we might make passing media track info model more aligned
with that of HTMLMediaElement (e.g. passing track info one track at a time,
instead of a complete collection of tracks), but then we would probably
still need to pass more info to Chromium level, e.g. track defaults.

Re decoupling a/v tracks from HTMLMediaElement - we might not necessarily
need to completely decouple them, we just need to ensure there's enough
flexibility to allow us to handle the SB a/v tracks. E.g. as I've explained
in the different CL we might need to fire addtrack/removetrack events on
both HTMLMediaElement and MediaSource. It's difficult to do with the
current model where TrackBase is responsible for firing add/remove/change
track events, but doesn't know anything about MediaSource. But that could
be solved if we moved the code for firing events out of TrackBase and into
HTMLMediaElement, that does know about MediaSource.

On Fri, Feb 26, 2016 at 4:43 AM, <philipj@opera.com> wrote:

> OK, I think I have some idea of the scope of the problem now.
>
> The chunked demuxer which hides somewhere behind WebMediaPlayerImpl is what
> actually finds tracks and their metadata, just like the normal demuxer for
> plain
> HTTP playback. In the plain playback case, HTMLMediaElement needs to know
> about
> the available tracks and their metadata, and HTMLMediaElement can tell WMP
> which
> tracks to enable/disable. MSE is similar, except the tracks must also be
> associated the SourceBuffer. In addition, the "init segment received"
> algorithm
> needs to perform certain checks.
>
> Would it be possible to perform the actual checks for "init segment
> received"
> close to the source (the demuxer?) and merely propagate the result of that
> to
> Blink when it's time to run the init segment received algorithm? Should
> the new
> tracks actually be exposed to the web developer in case of mismatch? It
> seems to
> me analogous to a decoding error and it would make sense to not expose the
> new
> tracks that cannot be used anyway. Would any of these changes simplify
> matters?
>
> If that is possible, then the APIs for track changes should look just
> about the
> same for all WMP implementations, but audio and video tracks should also
> take an
> identifier that from which Blink can find a SourceBuffer.
>
> The idea of decoupling AudioTrack and VideoTrack from HTMLMediaElement is
> also
> an interesting one. It would probably still have to go through
> HTMLMediaElement
> as it's the WebMediaPlayerClient, but it could forward the opaque
> identifier to
> a handler of some kind in the case of MSE, hopefully avoiding any new
> core-modules dependencies.
>
> Sorry that none of this is direct feedback on your code, but sorting out
> the
> high-level design first is probably a good idea.
>
> https://codereview.chromium.org/1659653002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

chromium-reviews

Yeah, currently some approximation of the init segment algorithm is implemented in Chromium media stack, ...

4 years, 9 months ago (2016-02-28 20:12:09 UTC) #14

Yeah, currently some approximation of the init segment algorithm is
implemented in Chromium media stack, but it's a. not fully spec compliant
b. not reusable by other blink clients. Wolenetz@ said that the spec
compliant init segment algorithm should be implemented on the blink level.
IIUC it would help with both of these points, it would help both make it
more spec compliant (since to make it fully spec compliant we need to have
this algorithm implemented where we have access to all the information
needed for it, and for example track defaults are currently only accessible
in blink, but not on Chromium demuxer/MediaSourceState level). And by the
virtue of being in blink it would be right away shareable/reusable by all
blink clients (also it might be easier to test that way). TBH I don't
really know how important these two point are, Matt can you add your
thoughts on this? Perhaps we could just pass track defaults information
down from the blink level into Chromium and that would make it easier to
implement the init segment algorithm on the Chromium media stack layer?
The course of further development would depend on this decision - if we
decide that we must have the init segment algorithm on blink level, we
would need something like what this CL does (passing media track info from
Chromium demuxers to blink level). If we decide to keep init segment alg on
Chromium level, we might make passing media track info model more aligned
with that of HTMLMediaElement (e.g. passing track info one track at a time,
instead of a complete collection of tracks), but then we would probably
still need to pass more info to Chromium level, e.g. track defaults.

Re decoupling a/v tracks from HTMLMediaElement - we might not necessarily
need to completely decouple them, we just need to ensure there's enough
flexibility to allow us to handle the SB a/v tracks. E.g. as I've explained
in the different CL we might need to fire addtrack/removetrack events on
both HTMLMediaElement and MediaSource. It's difficult to do with the
current model where TrackBase is responsible for firing add/remove/change
track events, but doesn't know anything about MediaSource. But that could
be solved if we moved the code for firing events out of TrackBase and into
HTMLMediaElement, that does know about MediaSource.

On Fri, Feb 26, 2016 at 4:43 AM, <philipj@opera.com> wrote:

> OK, I think I have some idea of the scope of the problem now.
>
> The chunked demuxer which hides somewhere behind WebMediaPlayerImpl is what
> actually finds tracks and their metadata, just like the normal demuxer for
> plain
> HTTP playback. In the plain playback case, HTMLMediaElement needs to know
> about
> the available tracks and their metadata, and HTMLMediaElement can tell WMP
> which
> tracks to enable/disable. MSE is similar, except the tracks must also be
> associated the SourceBuffer. In addition, the "init segment received"
> algorithm
> needs to perform certain checks.
>
> Would it be possible to perform the actual checks for "init segment
> received"
> close to the source (the demuxer?) and merely propagate the result of that
> to
> Blink when it's time to run the init segment received algorithm? Should
> the new
> tracks actually be exposed to the web developer in case of mismatch? It
> seems to
> me analogous to a decoding error and it would make sense to not expose the
> new
> tracks that cannot be used anyway. Would any of these changes simplify
> matters?
>
> If that is possible, then the APIs for track changes should look just
> about the
> same for all WMP implementations, but audio and video tracks should also
> take an
> identifier that from which Blink can find a SourceBuffer.
>
> The idea of decoupling AudioTrack and VideoTrack from HTMLMediaElement is
> also
> an interesting one. It would probably still have to go through
> HTMLMediaElement
> as it's the WebMediaPlayerClient, but it could forward the opaque
> identifier to
> a handler of some kind in the case of MSE, hopefully avoiding any new
> core-modules dependencies.
>
> Sorry that none of this is direct feedback on your code, but sorting out
> the
> high-level design first is probably a good idea.
>
> https://codereview.chromium.org/1659653002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

servolk

On 2016/02/28 20:12:09, chromium-reviews wrote: > Yeah, currently some approximation of the init segment algorithm ...

4 years, 9 months ago (2016-03-02 03:22:54 UTC) #15

On 2016/02/28 20:12:09, chromium-reviews wrote:
> Yeah, currently some approximation of the init segment algorithm is
> implemented in Chromium media stack, but it's a. not fully spec compliant
> b. not reusable by other blink clients. Wolenetz@ said that the spec
> compliant init segment algorithm should be implemented on the blink level.
> IIUC it would help with both of these points, it would help both make it
> more spec compliant (since to make it fully spec compliant we need to have
> this algorithm implemented where we have access to all the information
> needed for it, and for example track defaults are currently only accessible
> in blink, but not on Chromium demuxer/MediaSourceState level). And by the
> virtue of being in blink it would be right away shareable/reusable by all
> blink clients (also it might be easier to test that way). TBH I don't
> really know how important these two point are, Matt can you add your
> thoughts on this? Perhaps we could just pass track defaults information
> down from the blink level into Chromium and that would make it easier to
> implement the init segment algorithm on the Chromium media stack layer?
> The course of further development would depend on this decision - if we
> decide that we must have the init segment algorithm on blink level, we
> would need something like what this CL does (passing media track info from
> Chromium demuxers to blink level). If we decide to keep init segment alg on
> Chromium level, we might make passing media track info model more aligned
> with that of HTMLMediaElement (e.g. passing track info one track at a time,
> instead of a complete collection of tracks), but then we would probably
> still need to pass more info to Chromium level, e.g. track defaults.
> 
> Re decoupling a/v tracks from HTMLMediaElement - we might not necessarily
> need to completely decouple them, we just need to ensure there's enough
> flexibility to allow us to handle the SB a/v tracks. E.g. as I've explained
> in the different CL we might need to fire addtrack/removetrack events on
> both HTMLMediaElement and MediaSource. It's difficult to do with the
> current model where TrackBase is responsible for firing add/remove/change
> track events, but doesn't know anything about MediaSource. But that could
> be solved if we moved the code for firing events out of TrackBase and into
> HTMLMediaElement, that does know about MediaSource.
> 
> 
> On Fri, Feb 26, 2016 at 4:43 AM, <mailto:philipj@opera.com> wrote:
> 
> > OK, I think I have some idea of the scope of the problem now.
> >
> > The chunked demuxer which hides somewhere behind WebMediaPlayerImpl is what
> > actually finds tracks and their metadata, just like the normal demuxer for
> > plain
> > HTTP playback. In the plain playback case, HTMLMediaElement needs to know
> > about
> > the available tracks and their metadata, and HTMLMediaElement can tell WMP
> > which
> > tracks to enable/disable. MSE is similar, except the tracks must also be
> > associated the SourceBuffer. In addition, the "init segment received"
> > algorithm
> > needs to perform certain checks.
> >
> > Would it be possible to perform the actual checks for "init segment
> > received"
> > close to the source (the demuxer?) and merely propagate the result of that
> > to
> > Blink when it's time to run the init segment received algorithm? Should
> > the new
> > tracks actually be exposed to the web developer in case of mismatch? It
> > seems to
> > me analogous to a decoding error and it would make sense to not expose the
> > new
> > tracks that cannot be used anyway. Would any of these changes simplify
> > matters?
> >
> > If that is possible, then the APIs for track changes should look just
> > about the
> > same for all WMP implementations, but audio and video tracks should also
> > take an
> > identifier that from which Blink can find a SourceBuffer.
> >
> > The idea of decoupling AudioTrack and VideoTrack from HTMLMediaElement is
> > also
> > an interesting one. It would probably still have to go through
> > HTMLMediaElement
> > as it's the WebMediaPlayerClient, but it could forward the opaque
> > identifier to
> > a handler of some kind in the case of MSE, hopefully avoiding any new
> > core-modules dependencies.
> >
> > Sorry that none of this is direct feedback on your code, but sorting out
> > the
> > high-level design first is probably a good idea.
> >
> > https://codereview.chromium.org/1659653002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Ok, I've reworked this a bit more today, please take a look. What do you think
of this approach?
In the latest patchset I've got rid of the WebMediaTrack interface and went with
a simpler approach where we keep the blink track objects in blink and only pass
blink trackId values back to Chromium when creating new tracks. This way we
avoid issues with track objects potentially being garbage collected (since now
we'll have strong refs to pending tracks in SourceBuffer::m_pendingTracks), and
we'll also need only minimal changes to the public interfaces (we'll only need
to move TrackType enum to WebMediaPlayer, so that it's visible in Chromium).

servolk

Description was changed from ========== Added public WebMediaTrack interface and pass media track info to ...

4 years, 9 months ago (2016-03-02 03:23:52 UTC) #16

servolk

Description was changed from ========== Pass MSE media track info from ChunkDemuxer to SourceBuffer BUG=249427 ...

4 years, 9 months ago (2016-03-02 03:24:22 UTC) #17

servolk

Description was changed from ========== Pass MSE media track info from ChunkDemuxer to SourceBuffer BUG=249428 ...

4 years, 9 months ago (2016-03-03 02:55:22 UTC) #18

philipj_slow

wolenetz@, can you comment? It seems like this group of changes are motivated by moving ...

4 years, 9 months ago (2016-03-07 11:50:21 UTC) #19

philipj_slow

On 2016/02/28 20:12:09, blink-reviews wrote: > Yeah, currently some approximation of the init segment algorithm ...

4 years, 9 months ago (2016-03-07 12:00:15 UTC) #20

On 2016/02/28 20:12:09, blink-reviews wrote:
> Yeah, currently some approximation of the init segment algorithm is
> implemented in Chromium media stack, but it's a. not fully spec compliant
> b. not reusable by other blink clients. Wolenetz@ said that the spec
> compliant init segment algorithm should be implemented on the blink level.
> IIUC it would help with both of these points, it would help both make it
> more spec compliant (since to make it fully spec compliant we need to have
> this algorithm implemented where we have access to all the information
> needed for it, and for example track defaults are currently only accessible
> in blink, but not on Chromium demuxer/MediaSourceState level). And by the
> virtue of being in blink it would be right away shareable/reusable by all
> blink clients (also it might be easier to test that way). TBH I don't
> really know how important these two point are, Matt can you add your
> thoughts on this? Perhaps we could just pass track defaults information
> down from the blink level into Chromium and that would make it easier to
> implement the init segment algorithm on the Chromium media stack layer?
> The course of further development would depend on this decision - if we
> decide that we must have the init segment algorithm on blink level, we
> would need something like what this CL does (passing media track info from
> Chromium demuxers to blink level). If we decide to keep init segment alg on
> Chromium level, we might make passing media track info model more aligned
> with that of HTMLMediaElement (e.g. passing track info one track at a time,
> instead of a complete collection of tracks), but then we would probably
> still need to pass more info to Chromium level, e.g. track defaults.

Here I think I'm missing something. By defaults do you mean something like the
fact that "subtitles" is the default kind for subtitles? Whatever the defaults
are, what will Chromium need them for? The rules for verifying that tracks
change only in sane ways in the spec seem to only look at the number of tracks
of each type (audio/video/text) and that IDs match.

It seemed initially to me like it would be a simple matter to keep those
specific checks on the Chromium side and merely tell Blink to run the failure
steps if the tracks didn't add up, but perhaps there's more to it?

wolenetz

On 2016/03/07 12:00:15, philipj_UTC7 wrote: > On 2016/02/28 20:12:09, blink-reviews wrote: > > Yeah, currently ...

4 years, 9 months ago (2016-03-12 00:41:36 UTC) #21

On 2016/03/07 12:00:15, philipj_UTC7 wrote:
> On 2016/02/28 20:12:09, blink-reviews wrote:
> > Yeah, currently some approximation of the init segment algorithm is
> > implemented in Chromium media stack, but it's a. not fully spec compliant
> > b. not reusable by other blink clients. Wolenetz@ said that the spec
> > compliant init segment algorithm should be implemented on the blink level.
> > IIUC it would help with both of these points, it would help both make it
> > more spec compliant (since to make it fully spec compliant we need to have
> > this algorithm implemented where we have access to all the information
> > needed for it, and for example track defaults are currently only accessible
> > in blink, but not on Chromium demuxer/MediaSourceState level). And by the
> > virtue of being in blink it would be right away shareable/reusable by all
> > blink clients (also it might be easier to test that way). TBH I don't
> > really know how important these two point are, Matt can you add your
> > thoughts on this? Perhaps we could just pass track defaults information
> > down from the blink level into Chromium and that would make it easier to
> > implement the init segment algorithm on the Chromium media stack layer?
> > The course of further development would depend on this decision - if we
> > decide that we must have the init segment algorithm on blink level, we
> > would need something like what this CL does (passing media track info from
> > Chromium demuxers to blink level). If we decide to keep init segment alg on
> > Chromium level, we might make passing media track info model more aligned
> > with that of HTMLMediaElement (e.g. passing track info one track at a time,
> > instead of a complete collection of tracks), but then we would probably
> > still need to pass more info to Chromium level, e.g. track defaults.
> 
> Here I think I'm missing something. By defaults do you mean something like the
> fact that "subtitles" is the default kind for subtitles? Whatever the defaults
> are, what will Chromium need them for? The rules for verifying that tracks
> change only in sane ways in the spec seem to only look at the number of tracks
> of each type (audio/video/text) and that IDs match.
> 
> It seemed initially to me like it would be a simple matter to keep those
> specific checks on the Chromium side and merely tell Blink to run the failure
> steps if the tracks didn't add up, but perhaps there's more to it?

Hmm. I stand corrected; I agree philipj@'s suggested approach is valid, too:
Looking at this again, we can indeed lessen the scope of the change needed for
compliant initialization segment received processing to 1) doing the
error-checking on Chromium, and 2) then signalling Blink to do the rest (like
populating the track lists with track objects with defaults applied and unique
IDs generated, and queuing events and so forth). IIRC, at one time there was a
version of the initialization segment received algorithm spec (or my reading of
it) which may have allowed error case to occur later during the actual
population of those track lists/events/etc. I don't see that now though.
For reference of a previously not-landed WIP CL that was preparing for better
compliance, see https://codereview.chromium.org/361023003/

Caveat: are there blink embedders (Opera?) which might benefit from even the
error checking steps being done on Blink side? I don't think so, given the
approach suggested by philipj@, as well as our current everything-is-in-Chromium
impl.

wolenetz

On 2016/03/12 00:41:36, wolenetz wrote: > On 2016/03/07 12:00:15, philipj_UTC7 wrote: > > On 2016/02/28 ...

4 years, 9 months ago (2016-03-12 01:02:41 UTC) #22

On 2016/03/12 00:41:36, wolenetz wrote:
> On 2016/03/07 12:00:15, philipj_UTC7 wrote:
> > On 2016/02/28 20:12:09, blink-reviews wrote:
> > > Yeah, currently some approximation of the init segment algorithm is
> > > implemented in Chromium media stack, but it's a. not fully spec compliant
> > > b. not reusable by other blink clients. Wolenetz@ said that the spec
> > > compliant init segment algorithm should be implemented on the blink level.
> > > IIUC it would help with both of these points, it would help both make it
> > > more spec compliant (since to make it fully spec compliant we need to have
> > > this algorithm implemented where we have access to all the information
> > > needed for it, and for example track defaults are currently only
accessible
> > > in blink, but not on Chromium demuxer/MediaSourceState level). And by the
> > > virtue of being in blink it would be right away shareable/reusable by all
> > > blink clients (also it might be easier to test that way). TBH I don't
> > > really know how important these two point are, Matt can you add your
> > > thoughts on this? Perhaps we could just pass track defaults information
> > > down from the blink level into Chromium and that would make it easier to
> > > implement the init segment algorithm on the Chromium media stack layer?
> > > The course of further development would depend on this decision - if we
> > > decide that we must have the init segment algorithm on blink level, we
> > > would need something like what this CL does (passing media track info from
> > > Chromium demuxers to blink level). If we decide to keep init segment alg
on
> > > Chromium level, we might make passing media track info model more aligned
> > > with that of HTMLMediaElement (e.g. passing track info one track at a
time,
> > > instead of a complete collection of tracks), but then we would probably
> > > still need to pass more info to Chromium level, e.g. track defaults.
> > 
> > Here I think I'm missing something. By defaults do you mean something like
the
> > fact that "subtitles" is the default kind for subtitles? Whatever the
defaults
> > are, what will Chromium need them for? The rules for verifying that tracks
> > change only in sane ways in the spec seem to only look at the number of
tracks
> > of each type (audio/video/text) and that IDs match.
> > 
> > It seemed initially to me like it would be a simple matter to keep those
> > specific checks on the Chromium side and merely tell Blink to run the
failure
> > steps if the tracks didn't add up, but perhaps there's more to it?
> 
> Hmm. I stand corrected; I agree philipj@'s suggested approach is valid, too:
> Looking at this again, we can indeed lessen the scope of the change needed for
> compliant initialization segment received processing to 1) doing the
> error-checking on Chromium, and 2) then signalling Blink to do the rest (like
> populating the track lists with track objects with defaults applied and unique
> IDs generated, and queuing events and so forth). IIRC, at one time there was a
> version of the initialization segment received algorithm spec (or my reading
of
> it) which may have allowed error case to occur later during the actual
> population of those track lists/events/etc. I don't see that now though.
> For reference of a previously not-landed WIP CL that was preparing for better
> compliance, see https://codereview.chromium.org/361023003/
> 
> Caveat: are there blink embedders (Opera?) which might benefit from even the
> error checking steps being done on Blink side? I don't think so, given the
> approach suggested by philipj@, as well as our current
everything-is-in-Chromium
> impl.

Hmm. I don't seem to have actually fully agreed with philipj@, on re-read of his
suggestion and my response. :) \
To be clear, definitely there is initsegment received algorithm work required on
the Blink side (as well as on Chromium side).

The bulk of the error checking (e.g. compatible codec, bytestream-specific
stuff) needs to be Chromium side (and is both currently and in this CL). If
embedders might benefit, we could duplicate some of the basic "number and type"
error checking on Blink side of this, too, but I'd prefer no duplicate code and
to separate the concerns precisely here. To that end:
The TrackDefaults application IMO needs to be Blink-side (yes, Chromium side
SourceBuffer/ChunkDemuxer/etc don't need language, kind, etc stuff). Only once
Chromium thinks there's a valid InitSegment should it then signal Blink to do
the TrackDefault and track list populations, etc. By nature, it's split across
Chromium and Blink. If there's a question "once Chromium has done step 3 of the
algorithm, should Chromium make granular calls into Blink for each track to get
the track created, then populated with trackdefault stuff, then added into
parent HTMLMediaElement, and transition the associated SourceBuffer into
activeSourceBuffers in the right sequence according to all those steps in the
spec algorithm?", then my answer would be that I think these particular steps
are best left to Blink (specifically those in the major algorithm step # 5). The
management of active track->readystate transition (steps 4 and 7) is already in
Chromium IIUC.

servolk

On 2016/03/12 01:02:41, wolenetz wrote: > On 2016/03/12 00:41:36, wolenetz wrote: > > On 2016/03/07 ...

4 years, 9 months ago (2016-03-18 01:13:52 UTC) #23

On 2016/03/12 01:02:41, wolenetz wrote:
> On 2016/03/12 00:41:36, wolenetz wrote:
> > On 2016/03/07 12:00:15, philipj_UTC7 wrote:
> > > On 2016/02/28 20:12:09, blink-reviews wrote:
> > > > Yeah, currently some approximation of the init segment algorithm is
> > > > implemented in Chromium media stack, but it's a. not fully spec
compliant
> > > > b. not reusable by other blink clients. Wolenetz@ said that the spec
> > > > compliant init segment algorithm should be implemented on the blink
level.
> > > > IIUC it would help with both of these points, it would help both make it
> > > > more spec compliant (since to make it fully spec compliant we need to
have
> > > > this algorithm implemented where we have access to all the information
> > > > needed for it, and for example track defaults are currently only
> accessible
> > > > in blink, but not on Chromium demuxer/MediaSourceState level). And by
the
> > > > virtue of being in blink it would be right away shareable/reusable by
all
> > > > blink clients (also it might be easier to test that way). TBH I don't
> > > > really know how important these two point are, Matt can you add your
> > > > thoughts on this? Perhaps we could just pass track defaults information
> > > > down from the blink level into Chromium and that would make it easier to
> > > > implement the init segment algorithm on the Chromium media stack layer?
> > > > The course of further development would depend on this decision - if we
> > > > decide that we must have the init segment algorithm on blink level, we
> > > > would need something like what this CL does (passing media track info
from
> > > > Chromium demuxers to blink level). If we decide to keep init segment alg
> on
> > > > Chromium level, we might make passing media track info model more
aligned
> > > > with that of HTMLMediaElement (e.g. passing track info one track at a
> time,
> > > > instead of a complete collection of tracks), but then we would probably
> > > > still need to pass more info to Chromium level, e.g. track defaults.
> > > 
> > > Here I think I'm missing something. By defaults do you mean something like
> the
> > > fact that "subtitles" is the default kind for subtitles? Whatever the
> defaults
> > > are, what will Chromium need them for? The rules for verifying that tracks
> > > change only in sane ways in the spec seem to only look at the number of
> tracks
> > > of each type (audio/video/text) and that IDs match.
> > > 
> > > It seemed initially to me like it would be a simple matter to keep those
> > > specific checks on the Chromium side and merely tell Blink to run the
> failure
> > > steps if the tracks didn't add up, but perhaps there's more to it?
> > 
> > Hmm. I stand corrected; I agree philipj@'s suggested approach is valid, too:
> > Looking at this again, we can indeed lessen the scope of the change needed
for
> > compliant initialization segment received processing to 1) doing the
> > error-checking on Chromium, and 2) then signalling Blink to do the rest
(like
> > populating the track lists with track objects with defaults applied and
unique
> > IDs generated, and queuing events and so forth). IIRC, at one time there was
a
> > version of the initialization segment received algorithm spec (or my reading
> of
> > it) which may have allowed error case to occur later during the actual
> > population of those track lists/events/etc. I don't see that now though.
> > For reference of a previously not-landed WIP CL that was preparing for
better
> > compliance, see https://codereview.chromium.org/361023003/
> > 
> > Caveat: are there blink embedders (Opera?) which might benefit from even the
> > error checking steps being done on Blink side? I don't think so, given the
> > approach suggested by philipj@, as well as our current
> everything-is-in-Chromium
> > impl.
> 
> Hmm. I don't seem to have actually fully agreed with philipj@, on re-read of
his
> suggestion and my response. :) \
> To be clear, definitely there is initsegment received algorithm work required
on
> the Blink side (as well as on Chromium side).
> 
> The bulk of the error checking (e.g. compatible codec, bytestream-specific
> stuff) needs to be Chromium side (and is both currently and in this CL). If
> embedders might benefit, we could duplicate some of the basic "number and
type"
> error checking on Blink side of this, too, but I'd prefer no duplicate code
and
> to separate the concerns precisely here. To that end:
> The TrackDefaults application IMO needs to be Blink-side (yes, Chromium side
> SourceBuffer/ChunkDemuxer/etc don't need language, kind, etc stuff). Only once
> Chromium thinks there's a valid InitSegment should it then signal Blink to do
> the TrackDefault and track list populations, etc. By nature, it's split across
> Chromium and Blink. If there's a question "once Chromium has done step 3 of
the
> algorithm, should Chromium make granular calls into Blink for each track to
get
> the track created, then populated with trackdefault stuff, then added into
> parent HTMLMediaElement, and transition the associated SourceBuffer into
> activeSourceBuffers in the right sequence according to all those steps in the
> spec algorithm?", then my answer would be that I think these particular steps
> are best left to Blink (specifically those in the major algorithm step # 5).
The
> management of active track->readystate transition (steps 4 and 7) is already
in
> Chromium IIUC.

Ok, I think we are getting a little sidetracked here. These are all valid points
about implementing the init segment algorithm in blink, but note that this CL
doesn't actually implement the 'init segment algorithm'. The main point of this
CL is to implement transfer of media track information from Chromium level to
blink HTMLMediaElement/SourceBuffer. This is going to allow us to expose actual
media track info obtained from actual bitstreams via demuxers, instead of the
current placeholder tracks generated by HTMLMediaElement. Let's move this
discussion about the init segment algorithm to the relevant CL
(https://codereview.chromium.org/1678523003/, it's a bit updated, but I'm
planning to update it soon). In the meantime this CL is still useful on it's
own. For example today I've been able to implement muting/unmuting audio streams
via HTML5 media track APIs by building on top of this CL see
https://codereview.chromium.org/1812543003/ for better idea of the next steps.

servolk

On 2016/03/18 01:13:52, servolk wrote: > On 2016/03/12 01:02:41, wolenetz wrote: > > On 2016/03/12 ...

4 years, 9 months ago (2016-03-22 20:52:41 UTC) #24

On 2016/03/18 01:13:52, servolk wrote:
> On 2016/03/12 01:02:41, wolenetz wrote:
> > On 2016/03/12 00:41:36, wolenetz wrote:
> > > On 2016/03/07 12:00:15, philipj_UTC7 wrote:
> > > > On 2016/02/28 20:12:09, blink-reviews wrote:
> > > > > Yeah, currently some approximation of the init segment algorithm is
> > > > > implemented in Chromium media stack, but it's a. not fully spec
> compliant
> > > > > b. not reusable by other blink clients. Wolenetz@ said that the spec
> > > > > compliant init segment algorithm should be implemented on the blink
> level.
> > > > > IIUC it would help with both of these points, it would help both make
it
> > > > > more spec compliant (since to make it fully spec compliant we need to
> have
> > > > > this algorithm implemented where we have access to all the information
> > > > > needed for it, and for example track defaults are currently only
> > accessible
> > > > > in blink, but not on Chromium demuxer/MediaSourceState level). And by
> the
> > > > > virtue of being in blink it would be right away shareable/reusable by
> all
> > > > > blink clients (also it might be easier to test that way). TBH I don't
> > > > > really know how important these two point are, Matt can you add your
> > > > > thoughts on this? Perhaps we could just pass track defaults
information
> > > > > down from the blink level into Chromium and that would make it easier
to
> > > > > implement the init segment algorithm on the Chromium media stack
layer?
> > > > > The course of further development would depend on this decision - if
we
> > > > > decide that we must have the init segment algorithm on blink level, we
> > > > > would need something like what this CL does (passing media track info
> from
> > > > > Chromium demuxers to blink level). If we decide to keep init segment
alg
> > on
> > > > > Chromium level, we might make passing media track info model more
> aligned
> > > > > with that of HTMLMediaElement (e.g. passing track info one track at a
> > time,
> > > > > instead of a complete collection of tracks), but then we would
probably
> > > > > still need to pass more info to Chromium level, e.g. track defaults.
> > > > 
> > > > Here I think I'm missing something. By defaults do you mean something
like
> > the
> > > > fact that "subtitles" is the default kind for subtitles? Whatever the
> > defaults
> > > > are, what will Chromium need them for? The rules for verifying that
tracks
> > > > change only in sane ways in the spec seem to only look at the number of
> > tracks
> > > > of each type (audio/video/text) and that IDs match.
> > > > 
> > > > It seemed initially to me like it would be a simple matter to keep those
> > > > specific checks on the Chromium side and merely tell Blink to run the
> > failure
> > > > steps if the tracks didn't add up, but perhaps there's more to it?
> > > 
> > > Hmm. I stand corrected; I agree philipj@'s suggested approach is valid,
too:
> > > Looking at this again, we can indeed lessen the scope of the change needed
> for
> > > compliant initialization segment received processing to 1) doing the
> > > error-checking on Chromium, and 2) then signalling Blink to do the rest
> (like
> > > populating the track lists with track objects with defaults applied and
> unique
> > > IDs generated, and queuing events and so forth). IIRC, at one time there
was
> a
> > > version of the initialization segment received algorithm spec (or my
reading
> > of
> > > it) which may have allowed error case to occur later during the actual
> > > population of those track lists/events/etc. I don't see that now though.
> > > For reference of a previously not-landed WIP CL that was preparing for
> better
> > > compliance, see https://codereview.chromium.org/361023003/
> > > 
> > > Caveat: are there blink embedders (Opera?) which might benefit from even
the
> > > error checking steps being done on Blink side? I don't think so, given the
> > > approach suggested by philipj@, as well as our current
> > everything-is-in-Chromium
> > > impl.
> > 
> > Hmm. I don't seem to have actually fully agreed with philipj@, on re-read of
> his
> > suggestion and my response. :) \
> > To be clear, definitely there is initsegment received algorithm work
required
> on
> > the Blink side (as well as on Chromium side).
> > 
> > The bulk of the error checking (e.g. compatible codec, bytestream-specific
> > stuff) needs to be Chromium side (and is both currently and in this CL). If
> > embedders might benefit, we could duplicate some of the basic "number and
> type"
> > error checking on Blink side of this, too, but I'd prefer no duplicate code
> and
> > to separate the concerns precisely here. To that end:
> > The TrackDefaults application IMO needs to be Blink-side (yes, Chromium side
> > SourceBuffer/ChunkDemuxer/etc don't need language, kind, etc stuff). Only
once
> > Chromium thinks there's a valid InitSegment should it then signal Blink to
do
> > the TrackDefault and track list populations, etc. By nature, it's split
across
> > Chromium and Blink. If there's a question "once Chromium has done step 3 of
> the
> > algorithm, should Chromium make granular calls into Blink for each track to
> get
> > the track created, then populated with trackdefault stuff, then added into
> > parent HTMLMediaElement, and transition the associated SourceBuffer into
> > activeSourceBuffers in the right sequence according to all those steps in
the
> > spec algorithm?", then my answer would be that I think these particular
steps
> > are best left to Blink (specifically those in the major algorithm step # 5).
> The
> > management of active track->readystate transition (steps 4 and 7) is already
> in
> > Chromium IIUC.
> 
> Ok, I think we are getting a little sidetracked here. These are all valid
points
> about implementing the init segment algorithm in blink, but note that this CL
> doesn't actually implement the 'init segment algorithm'. The main point of
this
> CL is to implement transfer of media track information from Chromium level to
> blink HTMLMediaElement/SourceBuffer. This is going to allow us to expose
actual
> media track info obtained from actual bitstreams via demuxers, instead of the
> current placeholder tracks generated by HTMLMediaElement. Let's move this
> discussion about the init segment algorithm to the relevant CL
> (https://codereview.chromium.org/1678523003/, it's a bit updated, but I'm
> planning to update it soon). In the meantime this CL is still useful on it's
> own. For example today I've been able to implement muting/unmuting audio
streams
> via HTML5 media track APIs by building on top of this CL see
> https://codereview.chromium.org/1812543003/ for better idea of the next steps.

ping

wolenetz

On 2016/03/22 20:52:41, servolk wrote: > On 2016/03/18 01:13:52, servolk wrote: > > On 2016/03/12 ...

4 years, 9 months ago (2016-03-22 23:44:40 UTC) #25

On 2016/03/22 20:52:41, servolk wrote:
> On 2016/03/18 01:13:52, servolk wrote:
> > On 2016/03/12 01:02:41, wolenetz wrote:
> > > On 2016/03/12 00:41:36, wolenetz wrote:
> > > > On 2016/03/07 12:00:15, philipj_UTC7 wrote:
> > > > > On 2016/02/28 20:12:09, blink-reviews wrote:
> > > > > > Yeah, currently some approximation of the init segment algorithm is
> > > > > > implemented in Chromium media stack, but it's a. not fully spec
> > compliant
> > > > > > b. not reusable by other blink clients. Wolenetz@ said that the spec
> > > > > > compliant init segment algorithm should be implemented on the blink
> > level.
> > > > > > IIUC it would help with both of these points, it would help both
make
> it
> > > > > > more spec compliant (since to make it fully spec compliant we need
to
> > have
> > > > > > this algorithm implemented where we have access to all the
information
> > > > > > needed for it, and for example track defaults are currently only
> > > accessible
> > > > > > in blink, but not on Chromium demuxer/MediaSourceState level). And
by
> > the
> > > > > > virtue of being in blink it would be right away shareable/reusable
by
> > all
> > > > > > blink clients (also it might be easier to test that way). TBH I
don't
> > > > > > really know how important these two point are, Matt can you add your
> > > > > > thoughts on this? Perhaps we could just pass track defaults
> information
> > > > > > down from the blink level into Chromium and that would make it
easier
> to
> > > > > > implement the init segment algorithm on the Chromium media stack
> layer?
> > > > > > The course of further development would depend on this decision - if
> we
> > > > > > decide that we must have the init segment algorithm on blink level,
we
> > > > > > would need something like what this CL does (passing media track
info
> > from
> > > > > > Chromium demuxers to blink level). If we decide to keep init segment
> alg
> > > on
> > > > > > Chromium level, we might make passing media track info model more
> > aligned
> > > > > > with that of HTMLMediaElement (e.g. passing track info one track at
a
> > > time,
> > > > > > instead of a complete collection of tracks), but then we would
> probably
> > > > > > still need to pass more info to Chromium level, e.g. track defaults.
> > > > > 
> > > > > Here I think I'm missing something. By defaults do you mean something
> like
> > > the
> > > > > fact that "subtitles" is the default kind for subtitles? Whatever the
> > > defaults
> > > > > are, what will Chromium need them for? The rules for verifying that
> tracks
> > > > > change only in sane ways in the spec seem to only look at the number
of
> > > tracks
> > > > > of each type (audio/video/text) and that IDs match.
> > > > > 
> > > > > It seemed initially to me like it would be a simple matter to keep
those
> > > > > specific checks on the Chromium side and merely tell Blink to run the
> > > failure
> > > > > steps if the tracks didn't add up, but perhaps there's more to it?
> > > > 
> > > > Hmm. I stand corrected; I agree philipj@'s suggested approach is valid,
> too:
> > > > Looking at this again, we can indeed lessen the scope of the change
needed
> > for
> > > > compliant initialization segment received processing to 1) doing the
> > > > error-checking on Chromium, and 2) then signalling Blink to do the rest
> > (like
> > > > populating the track lists with track objects with defaults applied and
> > unique
> > > > IDs generated, and queuing events and so forth). IIRC, at one time there
> was
> > a
> > > > version of the initialization segment received algorithm spec (or my
> reading
> > > of
> > > > it) which may have allowed error case to occur later during the actual
> > > > population of those track lists/events/etc. I don't see that now though.
> > > > For reference of a previously not-landed WIP CL that was preparing for
> > better
> > > > compliance, see https://codereview.chromium.org/361023003/
> > > > 
> > > > Caveat: are there blink embedders (Opera?) which might benefit from even
> the
> > > > error checking steps being done on Blink side? I don't think so, given
the
> > > > approach suggested by philipj@, as well as our current
> > > everything-is-in-Chromium
> > > > impl.
> > > 
> > > Hmm. I don't seem to have actually fully agreed with philipj@, on re-read
of
> > his
> > > suggestion and my response. :) \
> > > To be clear, definitely there is initsegment received algorithm work
> required
> > on
> > > the Blink side (as well as on Chromium side).
> > > 
> > > The bulk of the error checking (e.g. compatible codec, bytestream-specific
> > > stuff) needs to be Chromium side (and is both currently and in this CL).
If
> > > embedders might benefit, we could duplicate some of the basic "number and
> > type"
> > > error checking on Blink side of this, too, but I'd prefer no duplicate
code
> > and
> > > to separate the concerns precisely here. To that end:
> > > The TrackDefaults application IMO needs to be Blink-side (yes, Chromium
side
> > > SourceBuffer/ChunkDemuxer/etc don't need language, kind, etc stuff). Only
> once
> > > Chromium thinks there's a valid InitSegment should it then signal Blink to
> do
> > > the TrackDefault and track list populations, etc. By nature, it's split
> across
> > > Chromium and Blink. If there's a question "once Chromium has done step 3
of
> > the
> > > algorithm, should Chromium make granular calls into Blink for each track
to
> > get
> > > the track created, then populated with trackdefault stuff, then added into
> > > parent HTMLMediaElement, and transition the associated SourceBuffer into
> > > activeSourceBuffers in the right sequence according to all those steps in
> the
> > > spec algorithm?", then my answer would be that I think these particular
> steps
> > > are best left to Blink (specifically those in the major algorithm step #
5).
> > The
> > > management of active track->readystate transition (steps 4 and 7) is
already
> > in
> > > Chromium IIUC.
> > 
> > Ok, I think we are getting a little sidetracked here. These are all valid
> points
> > about implementing the init segment algorithm in blink, but note that this
CL
> > doesn't actually implement the 'init segment algorithm'. The main point of
> this
> > CL is to implement transfer of media track information from Chromium level
to
> > blink HTMLMediaElement/SourceBuffer. This is going to allow us to expose
> actual
> > media track info obtained from actual bitstreams via demuxers, instead of
the
> > current placeholder tracks generated by HTMLMediaElement. Let's move this
> > discussion about the init segment algorithm to the relevant CL
> > (https://codereview.chromium.org/1678523003/, it's a bit updated, but I'm
> > planning to update it soon). In the meantime this CL is still useful on it's
> > own. For example today I've been able to implement muting/unmuting audio
> streams
> > via HTML5 media track APIs by building on top of this CL see
> > https://codereview.chromium.org/1812543003/ for better idea of the next
steps.
> 
> ping

Sorry - I'll get to this review first thing tomorrow morning pacific timezone.
I also think the CL was on the right track, regardless of the sidetrack
discussion :)

wolenetz

lgtm % nits. I look forward to seeing something more than the primitive init segment ...

4 years, 9 months ago (2016-03-23 19:34:13 UTC) #26

servolk

https://codereview.chromium.org/1659653002/diff/230001/media/blink/webmediaplayer_impl.cc File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1659653002/diff/230001/media/blink/webmediaplayer_impl.cc#newcode994 media/blink/webmediaplayer_impl.cc:994: scoped_ptr<::media::TextTrack> text_track(new TextTrackImpl( On 2016/03/23 19:34:12, wolenetz wrote: > ...

4 years, 9 months ago (2016-03-24 01:31:56 UTC) #27

servolk

The patchset sent to the CQ was uploaded after l-g-t-m from wolenetz@chromium.org Link to the ...

4 years, 9 months ago (2016-03-24 01:32:06 UTC) #29

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1659653002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1659653002/270001

4 years, 9 months ago (2016-03-24 01:32:14 UTC) #30

commit-bot: I haz the power

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

4 years, 9 months ago (2016-03-24 01:42:38 UTC) #31

commit-bot: I haz the power

Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/160326)

4 years, 9 months ago (2016-03-24 01:42:39 UTC) #32

servolk

On 2016/03/24 01:42:39, commit-bot: I haz the power wrote: > Try jobs failed on following ...

4 years, 9 months ago (2016-03-24 01:47:27 UTC) #33

wolenetz

https://codereview.chromium.org/1659653002/diff/230001/media/blink/websourcebuffer_impl.cc File media/blink/websourcebuffer_impl.cc (right): https://codereview.chromium.org/1659653002/diff/230001/media/blink/websourcebuffer_impl.cc#newcode178 media/blink/websourcebuffer_impl.cc:178: blink::WebVector<blink::WebMediaPlayer::TrackId> blinkTrackIds( On 2016/03/24 01:31:55, servolk wrote: > On ...

4 years, 9 months ago (2016-03-24 01:54:07 UTC) #34

https://codereview.chromium.org/1659653002/diff/230001/media/blink/websourceb...
File media/blink/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/1659653002/diff/230001/media/blink/websourceb...
media/blink/websourcebuffer_impl.cc:178:
blink::WebVector<blink::WebMediaPlayer::TrackId> blinkTrackIds(
On 2016/03/24 01:31:55, servolk wrote:
> On 2016/03/23 19:34:12, wolenetz wrote:
> > nit: DCHECK_LT(0, tracks->tracks().size() ?
> 
> No need for this dcheck here, since the case of empty media tracks set is
> explicitly called out in MSE spec and handled in
> client_->initializationSegmentReceived (see step 2 at
> https://www.w3.org/TR/media-source/#sourcebuffer-init-segment-received).

Acknowledged.

https://codereview.chromium.org/1659653002/diff/230001/media/blink/websourceb...
media/blink/websourcebuffer_impl.cc:182: blinkTrackIds[i++] =
client_->createMediaTrack(
On 2016/03/24 01:31:55, servolk wrote:
> On 2016/03/23 19:34:13, wolenetz wrote:
> > Are these blinkTrackIds only valid within the processing of the
pendingTracks
> in
> > blink? Or does Chromium need to know these at some point, too?
> 
> Chromium does need to know these, since media track status changes are
reported
> by blink through giving us a set of blink track ids. So we need to be able to
> map those track ids to our internal objects (MediaTrack or DemuxerStream), see
> https://codereview.chromium.org/1812543003/ for an example of how that could
be
> done.

Acknowledged.

https://codereview.chromium.org/1659653002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/1659653002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:519: for (const
auto& track : m_pendingTracks) {
On 2016/03/24 01:31:56, servolk wrote:
> On 2016/03/23 19:34:13, wolenetz wrote:
> > Good enough for now, but I suggest that a map might be a better structure.
> This
> > lookup is n^2 vs n with map. Alternatively, if we *don't* need the blink
> > TrackIDs in Chromium websourcebufferimpl, then why not just say "process all
> > pending, in the order they were added since last execution of init segment
> > received algorithm?"
> 
> I agree, but for now we support only 2 media tracks max (max 1 video and 1
> audio), so I'm not too concerned about perf here. This is going to change when
> we implement MSE spec-compliant 'init segment received' algorithm and make the
> necessary changes to support multiple audio and/or video tracks. This is good
> enough for now to ensure we don't break tests and existing functionality.

Acknowledged.

wolenetz

https://codereview.chromium.org/1659653002/diff/230001/third_party/WebKit/Source/modules/mediasource/SourceBuffer.h File third_party/WebKit/Source/modules/mediasource/SourceBuffer.h (right): https://codereview.chromium.org/1659653002/diff/230001/third_party/WebKit/Source/modules/mediasource/SourceBuffer.h#newcode149 third_party/WebKit/Source/modules/mediasource/SourceBuffer.h:149: HeapVector<WebMediaPlayer::TrackId> m_audioTrackIds; On 2016/03/24 01:31:56, servolk wrote: > On ...

4 years, 9 months ago (2016-03-24 01:57:43 UTC) #35

philipj_slow

On 2016/03/12 00:41:36, wolenetz wrote: > On 2016/03/07 12:00:15, philipj_UTC7 wrote: > > On 2016/02/28 ...

4 years, 9 months ago (2016-03-24 05:01:12 UTC) #36

On 2016/03/12 00:41:36, wolenetz wrote:
> On 2016/03/07 12:00:15, philipj_UTC7 wrote:
> > On 2016/02/28 20:12:09, blink-reviews wrote:
> > > Yeah, currently some approximation of the init segment algorithm is
> > > implemented in Chromium media stack, but it's a. not fully spec compliant
> > > b. not reusable by other blink clients. Wolenetz@ said that the spec
> > > compliant init segment algorithm should be implemented on the blink level.
> > > IIUC it would help with both of these points, it would help both make it
> > > more spec compliant (since to make it fully spec compliant we need to have
> > > this algorithm implemented where we have access to all the information
> > > needed for it, and for example track defaults are currently only
accessible
> > > in blink, but not on Chromium demuxer/MediaSourceState level). And by the
> > > virtue of being in blink it would be right away shareable/reusable by all
> > > blink clients (also it might be easier to test that way). TBH I don't
> > > really know how important these two point are, Matt can you add your
> > > thoughts on this? Perhaps we could just pass track defaults information
> > > down from the blink level into Chromium and that would make it easier to
> > > implement the init segment algorithm on the Chromium media stack layer?
> > > The course of further development would depend on this decision - if we
> > > decide that we must have the init segment algorithm on blink level, we
> > > would need something like what this CL does (passing media track info from
> > > Chromium demuxers to blink level). If we decide to keep init segment alg
on
> > > Chromium level, we might make passing media track info model more aligned
> > > with that of HTMLMediaElement (e.g. passing track info one track at a
time,
> > > instead of a complete collection of tracks), but then we would probably
> > > still need to pass more info to Chromium level, e.g. track defaults.
> > 
> > Here I think I'm missing something. By defaults do you mean something like
the
> > fact that "subtitles" is the default kind for subtitles? Whatever the
defaults
> > are, what will Chromium need them for? The rules for verifying that tracks
> > change only in sane ways in the spec seem to only look at the number of
tracks
> > of each type (audio/video/text) and that IDs match.
> > 
> > It seemed initially to me like it would be a simple matter to keep those
> > specific checks on the Chromium side and merely tell Blink to run the
failure
> > steps if the tracks didn't add up, but perhaps there's more to it?
> 
> Hmm. I stand corrected; I agree philipj@'s suggested approach is valid, too:
> Looking at this again, we can indeed lessen the scope of the change needed for
> compliant initialization segment received processing to 1) doing the
> error-checking on Chromium, and 2) then signalling Blink to do the rest (like
> populating the track lists with track objects with defaults applied and unique
> IDs generated, and queuing events and so forth). IIRC, at one time there was a
> version of the initialization segment received algorithm spec (or my reading
of
> it) which may have allowed error case to occur later during the actual
> population of those track lists/events/etc. I don't see that now though.
> For reference of a previously not-landed WIP CL that was preparing for better
> compliance, see https://codereview.chromium.org/361023003/
> 
> Caveat: are there blink embedders (Opera?) which might benefit from even the
> error checking steps being done on Blink side? I don't think so, given the
> approach suggested by philipj@, as well as our current
everything-is-in-Chromium
> impl.

I don't think so either, because we are not using Blink in isolation, but most
of Chromium. There are other WebMediaPlayer implementations for some products,
but to check that the track constraints are upheld doesn't seem like any more of
a burden than sending all of the track information to Blink for verification on
that side.

philipj_slow

https://codereview.chromium.org/1659653002/diff/270001/third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1659653002/diff/270001/third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp#newcode510 third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:510: m_source->mediaElement()->audioTracks().remove(trackId); Can't this end up adding and removing the ...

4 years, 9 months ago (2016-03-24 05:56:00 UTC) #37

https://codereview.chromium.org/1659653002/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/1659653002/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:510:
m_source->mediaElement()->audioTracks().remove(trackId);
Can't this end up adding and removing the same track, firing spurious events?
Unless I'm misunderstanding things still, the eventual shape of things should be
that we get some tracks with the first call to initializationSegmentReceived,
and they can never change after that because that should have been an error. So
could you assert that there are no existing tracks? And maybe an early return
somewhere above if this isn't the first init segment?

(I'm not sure if initializationSegmentReceived is going to end up called
multiple times or not, or if it's the responsibility of this code to handle the
situation.)

https://codereview.chromium.org/1659653002/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:555:
m_pendingTracks.append(result);
These only end up used in initializationSegmentReceived, where they are
referenced by id. For an API that's more obvious in what it does, have you
looked into simply passing a vector of the actual track data to
initializationSegmentReceived, so that the tracks are created there? A
WebVector<std::tuple<type, id, kind, label, language>> should do it, probably no
need for a new struct or class.

The track IDs aren't used on the Chromium side now, but if they are later
needed, a vector of assigned track IDs could be returned. This *looks* like a
simplification to me, if it actually complicates matters please clue me in :)

https://codereview.chromium.org/1659653002/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasource/SourceBuffer.h (right):

https://codereview.chromium.org/1659653002/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.h:106:
WebMediaPlayer::TrackId createMediaTrack(WebMediaPlayer::TrackType, WebString,
WebString, WebString, WebString) override;
The arguments should be named when not redundant with the type.

https://codereview.chromium.org/1659653002/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.h:149:
HeapVector<WebMediaPlayer::TrackId> m_audioTrackIds;
I think these can be plain Vector<>, WebMediaPlayer::TrackId is just a typedef
for unsigned int.

philipj_slow

On 2016/03/12 01:02:41, wolenetz wrote: > Hmm. I don't seem to have actually fully agreed ...

4 years, 9 months ago (2016-03-24 05:59:29 UTC) #38

servolk

On 2016/03/24 05:59:29, philipj_OOO_til_March29 wrote: > On 2016/03/12 01:02:41, wolenetz wrote: > > > Hmm. ...

4 years, 9 months ago (2016-03-24 21:56:53 UTC) #39

On 2016/03/24 05:59:29, philipj_OOO_til_March29 wrote:
> On 2016/03/12 01:02:41, wolenetz wrote:
> 
> > Hmm. I don't seem to have actually fully agreed with philipj@, on re-read of
> his
> > suggestion and my response. :) \
> > To be clear, definitely there is initsegment received algorithm work
required
> on
> > the Blink side (as well as on Chromium side).
> > 
> > The bulk of the error checking (e.g. compatible codec, bytestream-specific
> > stuff) needs to be Chromium side (and is both currently and in this CL). If
> > embedders might benefit, we could duplicate some of the basic "number and
> type"
> > error checking on Blink side of this, too, but I'd prefer no duplicate code
> and
> > to separate the concerns precisely here. To that end:
> > The TrackDefaults application IMO needs to be Blink-side (yes, Chromium side
> > SourceBuffer/ChunkDemuxer/etc don't need language, kind, etc stuff). Only
once
> > Chromium thinks there's a valid InitSegment should it then signal Blink to
do
> > the TrackDefault and track list populations, etc. By nature, it's split
across
> > Chromium and Blink. If there's a question "once Chromium has done step 3 of
> the
> > algorithm, should Chromium make granular calls into Blink for each track to
> get
> > the track created, then populated with trackdefault stuff, then added into
> > parent HTMLMediaElement, and transition the associated SourceBuffer into
> > activeSourceBuffers in the right sequence according to all those steps in
the
> > spec algorithm?", then my answer would be that I think these particular
steps
> > are best left to Blink (specifically those in the major algorithm step # 5).
> The
> > management of active track->readystate transition (steps 4 and 7) is already
> in
> > Chromium IIUC.
> 
> So, maybe this isn't in scope for this CL, but to make this a bit more
concrete,
> are we agreeing at least that checking that the number and types of tracks are
> the same can live on the Chromium side?

Yes, we can do that on the Chromium side. But that is just one of the steps of
MSE 'init segment received' algorithm. As I've explained earlier some other
steps might need access to SourceBuffer::trackDefaults collection, which is
currently not exposed to Chromium and is only accessible from blink. So if we go
down that path we would have to split MSE init segment handling between blink
and Chromium. But since this is out of scope of this CL, I think we can postpone
that discussion/decision until later.

servolk

Description was changed from ========== Pass MSE media track info from ChunkDemuxer to blink::SourceBuffer Also ...

4 years, 9 months ago (2016-03-25 02:12:50 UTC) #40

servolk

https://codereview.chromium.org/1659653002/diff/270001/third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1659653002/diff/270001/third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp#newcode510 third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:510: m_source->mediaElement()->audioTracks().remove(trackId); On 2016/03/24 05:56:00, philipj_OOO_til_March29 wrote: > Can't this ...

4 years, 8 months ago (2016-03-29 01:26:33 UTC) #41

https://codereview.chromium.org/1659653002/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/1659653002/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:510:
m_source->mediaElement()->audioTracks().remove(trackId);
On 2016/03/24 05:56:00, philipj_OOO_til_March29 wrote:
> Can't this end up adding and removing the same track, firing spurious events?
> Unless I'm misunderstanding things still, the eventual shape of things should
be
> that we get some tracks with the first call to initializationSegmentReceived,
> and they can never change after that because that should have been an error.
So
> could you assert that there are no existing tracks? And maybe an early return
> somewhere above if this isn't the first init segment?
> 
> (I'm not sure if initializationSegmentReceived is going to end up called
> multiple times or not, or if it's the responsibility of this code to handle
the
> situation.)

initializationSegmentReceived is definitely going to be invoked multiple times,
it gets invoked every time ChunkDemuxer receives a new init segment, which might
happen in the middle of playback, for example if the player decides to switch to
a different bitrate/quality level. And you are right, that in the previous
patchsets it would cause multiple change events to be fired. But now I've
reworked it a bit more (+ see below) and in the latest patchset of this CL that
will no longer happen. We will add media tracks only once, and then subsequent
init segments should be able to reuse existing tracks.

https://codereview.chromium.org/1659653002/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:555:
m_pendingTracks.append(result);
On 2016/03/24 05:56:00, philipj_OOO_til_March29 wrote:
> These only end up used in initializationSegmentReceived, where they are
> referenced by id. For an API that's more obvious in what it does, have you
> looked into simply passing a vector of the actual track data to
> initializationSegmentReceived, so that the tracks are created there? A
> WebVector<std::tuple<type, id, kind, label, language>> should do it, probably
no
> need for a new struct or class.
> 
> The track IDs aren't used on the Chromium side now, but if they are later
> needed, a vector of assigned track IDs could be returned. This *looks* like a
> simplification to me, if it actually complicates matters please clue me in :)

Yes, the same functionality can be achieved in a slightly different way. I have
reworked this CL and the latest patchset implements your suggestion (passing
track info as a bunch of std::tuples and returning blink track ids as a vector).
The track IDs are not used on Chromium side in this CL, but that is going to
change very soon. See the subsequent CLs that depend on this one, they required
some minor updates after reworking this CL, of course, but I've rebased and
uploaded latest versions of all the CLs. The track control CL
(https://codereview.chromium.org/1812543003/) is using track IDs to allow
muting/unmuting audio through Javascript MediaTrack APIs.

https://codereview.chromium.org/1659653002/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasource/SourceBuffer.h (right):

https://codereview.chromium.org/1659653002/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.h:106:
WebMediaPlayer::TrackId createMediaTrack(WebMediaPlayer::TrackType, WebString,
WebString, WebString, WebString) override;
On 2016/03/24 05:56:00, philipj_OOO_til_March29 wrote:
> The arguments should be named when not redundant with the type.

Done.

https://codereview.chromium.org/1659653002/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.h:149:
HeapVector<WebMediaPlayer::TrackId> m_audioTrackIds;
On 2016/03/24 05:56:00, philipj_OOO_til_March29 wrote:
> I think these can be plain Vector<>, WebMediaPlayer::TrackId is just a typedef
> for unsigned int.

Actually, I've just removed those in the latest patchset.

philipj_slow

On 2016/03/24 21:56:53, servolk wrote: > On 2016/03/24 05:59:29, philipj_OOO_til_March29 wrote: > > On 2016/03/12 ...

4 years, 8 months ago (2016-03-29 09:36:35 UTC) #42

On 2016/03/24 21:56:53, servolk wrote:
> On 2016/03/24 05:59:29, philipj_OOO_til_March29 wrote:
> > On 2016/03/12 01:02:41, wolenetz wrote:
> > 
> > > Hmm. I don't seem to have actually fully agreed with philipj@, on re-read
of
> > his
> > > suggestion and my response. :) \
> > > To be clear, definitely there is initsegment received algorithm work
> required
> > on
> > > the Blink side (as well as on Chromium side).
> > > 
> > > The bulk of the error checking (e.g. compatible codec, bytestream-specific
> > > stuff) needs to be Chromium side (and is both currently and in this CL).
If
> > > embedders might benefit, we could duplicate some of the basic "number and
> > type"
> > > error checking on Blink side of this, too, but I'd prefer no duplicate
code
> > and
> > > to separate the concerns precisely here. To that end:
> > > The TrackDefaults application IMO needs to be Blink-side (yes, Chromium
side
> > > SourceBuffer/ChunkDemuxer/etc don't need language, kind, etc stuff). Only
> once
> > > Chromium thinks there's a valid InitSegment should it then signal Blink to
> do
> > > the TrackDefault and track list populations, etc. By nature, it's split
> across
> > > Chromium and Blink. If there's a question "once Chromium has done step 3
of
> > the
> > > algorithm, should Chromium make granular calls into Blink for each track
to
> > get
> > > the track created, then populated with trackdefault stuff, then added into
> > > parent HTMLMediaElement, and transition the associated SourceBuffer into
> > > activeSourceBuffers in the right sequence according to all those steps in
> the
> > > spec algorithm?", then my answer would be that I think these particular
> steps
> > > are best left to Blink (specifically those in the major algorithm step #
5).
> > The
> > > management of active track->readystate transition (steps 4 and 7) is
already
> > in
> > > Chromium IIUC.
> > 
> > So, maybe this isn't in scope for this CL, but to make this a bit more
> concrete,
> > are we agreeing at least that checking that the number and types of tracks
are
> > the same can live on the Chromium side?
> 
> Yes, we can do that on the Chromium side. But that is just one of the steps of
> MSE 'init segment received' algorithm. As I've explained earlier some other
> steps might need access to SourceBuffer::trackDefaults collection, which is
> currently not exposed to Chromium and is only accessible from blink. So if we
go
> down that path we would have to split MSE init segment handling between blink
> and Chromium. But since this is out of scope of this CL, I think we can
postpone
> that discussion/decision until later.

OK, there's a bunch of things under "For each audio track in the initialization
segment" and "For each video track in the initialization segment" and I guess
those will be implemented in Blink? If so it probably does make sense to do as
much as possible of this in Blink.

philipj_slow

Looking pretty good now. I'll say LGTM so that you can land this if further ...

4 years, 8 months ago (2016-03-29 11:59:22 UTC) #43

servolk

https://codereview.chromium.org/1659653002/diff/390001/third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1659653002/diff/390001/third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp#newcode130 third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:130: m_audioTracks = AudioTrackList::create(*m_source->mediaElement()); On 2016/03/29 11:59:21, philipj_UTC7 wrote: > ...

4 years, 8 months ago (2016-03-29 17:30:34 UTC) #44

https://codereview.chromium.org/1659653002/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/1659653002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:130:
m_audioTracks = AudioTrackList::create(*m_source->mediaElement());
On 2016/03/29 11:59:21, philipj_UTC7 wrote:
> Looks like you could put this in the initializer list as
source->mediaElement(),
> unless doing the ASSERTS first is important?

Yeah, I wanted to check (via ASSERT) that we have a valid
m_source->mediaElement(), so I'd prefer to keep it here, instead of the
initializer list.

https://codereview.chromium.org/1659653002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:498: // I.e. we
only need to search by TrackID if there is more than one track, otherwise we can
assume that the only
On 2016/03/29 11:59:21, philipj_UTC7 wrote:
> Is this an optimization, or is it expected to ever be observable? Just using
> trackList.getTrackById() if that works seems simpler.
> 
> Comments if findTrackById needs to stay:
>  - getTrackById takes a const String&, so this could too to move the
conversion
> closer to where the WebString came from.
>  - Link to
> https://w3c.github.io/media-source/#sourcebuffer-init-segment-received (see
> other links in this file)
>  - Naming could be more clear about how it's different from getTrackById.

wolenetz@ mentioned that MSE spec allows bytestream track id to change if there
is only a single track of a given kind. E.g. if you have only a single audio
track, and then you receive a new init segment with a single audio track, then
you must assume it's the same audio track as before, even if bytestream id is
different. And after reading the MSE spec, especially the quoted sentence, I'm
inclined to agree with it. This function just implements that logic.

https://codereview.chromium.org/1659653002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:513: //
https://www.w3.org/TR/media-source/#sourcebuffer-init-segment-received
On 2016/03/29 11:59:21, philipj_UTC7 wrote:
> https://w3c.github.io/media-source/#sourcebuffer-init-segment-received here
too

Done.

https://codereview.chromium.org/1659653002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:530: audioTrack =
findTrackById(audioTracks(), id);
On 2016/03/29 11:59:21, philipj_UTC7 wrote:
> How about making it part of the API that
> if(m_firstInitializationSegmentReceived), an empty vector is returned instead?
I
> assume the Chromium side of this won't have any use for the IDs the second
time
> around anyway?

No, see the comments above for findExistingTrackById. We can't return the empty
vector here. It's important to return the track ids to Chromium for each init
segment, so that Chromium could update mappings between blink track ids and
Chromium media tracks/demuxer streams in case there was a media track with the
new bytestream id in the new init segment that got matched and assigned a track
id from the previous/existing track that was in the previous init segment.

https://codereview.chromium.org/1659653002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:550: const char*
logActionStr = m_firstInitializationSegmentReceived ? "using existing" :
"added";
On 2016/03/29 11:59:21, philipj_UTC7 wrote:
> The trackBase variable is only for logging it seems, can you move the logging
to
> before the loop, use #if !LOG_DISABLED to avoid (void) suppression and just
use
> the inputs instead of trackBase->?

Well, actually one important thing that we want to see in the log is the
trackBase->trackId(), I'd say it's more important than all the other track
properties, since it's necessary to understand which demuxer stream/media track
on the Chromium side is represented by this blink track object. So I'd prefer to
keep this logging here, where we've already created/found the track and thus
know it's trackId.
Though I agree, we can make it look a little nicer with the #if !LOG_DISABLED.

servolk

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

4 years, 8 months ago (2016-03-29 17:33:45 UTC) #45

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1659653002/430001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1659653002/430001

4 years, 8 months ago (2016-03-29 17:33:58 UTC) #46

commit-bot: I haz the power

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

4 years, 8 months ago (2016-03-29 18:44:07 UTC) #47

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oilpan_rel/builds/22781)

4 years, 8 months ago (2016-03-29 18:44:09 UTC) #48

servolk

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

4 years, 8 months ago (2016-03-29 21:40:10 UTC) #49

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1659653002/450001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1659653002/450001

4 years, 8 months ago (2016-03-29 21:40:39 UTC) #50

commit-bot: I haz the power

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

4 years, 8 months ago (2016-03-30 00:29:07 UTC) #51

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 8 months ago (2016-03-30 00:29:08 UTC) #52

wolenetz

On 2016/03/24 05:59:29, philipj_UTC7 wrote: > On 2016/03/12 01:02:41, wolenetz wrote: > > > Hmm. ...

4 years, 8 months ago (2016-03-30 00:47:14 UTC) #53

On 2016/03/24 05:59:29, philipj_UTC7 wrote:
> On 2016/03/12 01:02:41, wolenetz wrote:
> 
> > Hmm. I don't seem to have actually fully agreed with philipj@, on re-read of
> his
> > suggestion and my response. :) \
> > To be clear, definitely there is initsegment received algorithm work
required
> on
> > the Blink side (as well as on Chromium side).
> > 
> > The bulk of the error checking (e.g. compatible codec, bytestream-specific
> > stuff) needs to be Chromium side (and is both currently and in this CL). If
> > embedders might benefit, we could duplicate some of the basic "number and
> type"
> > error checking on Blink side of this, too, but I'd prefer no duplicate code
> and
> > to separate the concerns precisely here. To that end:
> > The TrackDefaults application IMO needs to be Blink-side (yes, Chromium side
> > SourceBuffer/ChunkDemuxer/etc don't need language, kind, etc stuff). Only
once
> > Chromium thinks there's a valid InitSegment should it then signal Blink to
do
> > the TrackDefault and track list populations, etc. By nature, it's split
across
> > Chromium and Blink. If there's a question "once Chromium has done step 3 of
> the
> > algorithm, should Chromium make granular calls into Blink for each track to
> get
> > the track created, then populated with trackdefault stuff, then added into
> > parent HTMLMediaElement, and transition the associated SourceBuffer into
> > activeSourceBuffers in the right sequence according to all those steps in
the
> > spec algorithm?", then my answer would be that I think these particular
steps
> > are best left to Blink (specifically those in the major algorithm step # 5).
> The
> > management of active track->readystate transition (steps 4 and 7) is already
> in
> > Chromium IIUC.
> 
> So, maybe this isn't in scope for this CL, but to make this a bit more
concrete,
> are we agreeing at least that checking that the number and types of tracks are
> the same can live on the Chromium side?

Yes

wolenetz

On 2016/03/29 09:36:35, philipj_UTC7 wrote: > On 2016/03/24 21:56:53, servolk wrote: > > On 2016/03/24 ...

4 years, 8 months ago (2016-03-30 00:49:45 UTC) #54

On 2016/03/29 09:36:35, philipj_UTC7 wrote:
> On 2016/03/24 21:56:53, servolk wrote:
> > On 2016/03/24 05:59:29, philipj_OOO_til_March29 wrote:
> > > On 2016/03/12 01:02:41, wolenetz wrote:
> > > 
> > > > Hmm. I don't seem to have actually fully agreed with philipj@, on
re-read
> of
> > > his
> > > > suggestion and my response. :) \
> > > > To be clear, definitely there is initsegment received algorithm work
> > required
> > > on
> > > > the Blink side (as well as on Chromium side).
> > > > 
> > > > The bulk of the error checking (e.g. compatible codec,
bytestream-specific
> > > > stuff) needs to be Chromium side (and is both currently and in this CL).
> If
> > > > embedders might benefit, we could duplicate some of the basic "number
and
> > > type"
> > > > error checking on Blink side of this, too, but I'd prefer no duplicate
> code
> > > and
> > > > to separate the concerns precisely here. To that end:
> > > > The TrackDefaults application IMO needs to be Blink-side (yes, Chromium
> side
> > > > SourceBuffer/ChunkDemuxer/etc don't need language, kind, etc stuff).
Only
> > once
> > > > Chromium thinks there's a valid InitSegment should it then signal Blink
to
> > do
> > > > the TrackDefault and track list populations, etc. By nature, it's split
> > across
> > > > Chromium and Blink. If there's a question "once Chromium has done step 3
> of
> > > the
> > > > algorithm, should Chromium make granular calls into Blink for each track
> to
> > > get
> > > > the track created, then populated with trackdefault stuff, then added
into
> > > > parent HTMLMediaElement, and transition the associated SourceBuffer into
> > > > activeSourceBuffers in the right sequence according to all those steps
in
> > the
> > > > spec algorithm?", then my answer would be that I think these particular
> > steps
> > > > are best left to Blink (specifically those in the major algorithm step #
> 5).
> > > The
> > > > management of active track->readystate transition (steps 4 and 7) is
> already
> > > in
> > > > Chromium IIUC.
> > > 
> > > So, maybe this isn't in scope for this CL, but to make this a bit more
> > concrete,
> > > are we agreeing at least that checking that the number and types of tracks
> are
> > > the same can live on the Chromium side?
> > 
> > Yes, we can do that on the Chromium side. But that is just one of the steps
of
> > MSE 'init segment received' algorithm. As I've explained earlier some other
> > steps might need access to SourceBuffer::trackDefaults collection, which is
> > currently not exposed to Chromium and is only accessible from blink. So if
we
> go
> > down that path we would have to split MSE init segment handling between
blink
> > and Chromium. But since this is out of scope of this CL, I think we can
> postpone
> > that discussion/decision until later.
> 
> OK, there's a bunch of things under "For each audio track in the
initialization
> segment" and "For each video track in the initialization segment" and I guess
> those will be implemented in Blink? If so it probably does make sense to do as
> much as possible of this in Blink.

:) Yes.

wolenetz

lgtm2 % a new tiny nit https://codereview.chromium.org/1659653002/diff/450001/third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1659653002/diff/450001/third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp#newcode509 third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:509: WTF_LOG(Media, "SourceBuffer::initializationSegmentReceived2 %p ...

4 years, 8 months ago (2016-03-30 01:06:15 UTC) #55

servolk

https://codereview.chromium.org/1659653002/diff/450001/third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1659653002/diff/450001/third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp#newcode509 third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:509: WTF_LOG(Media, "SourceBuffer::initializationSegmentReceived2 %p tracks=%zu", this, newTracks.size()); On 2016/03/30 01:06:15, ...

4 years, 8 months ago (2016-03-30 01:10:37 UTC) #56

servolk

The patchset sent to the CQ was uploaded after l-g-t-m from wolenetz@chromium.org, philipj@opera.com Link to ...

4 years, 8 months ago (2016-03-30 01:10:56 UTC) #58

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1659653002/470001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1659653002/470001

4 years, 8 months ago (2016-03-30 01:11:11 UTC) #59

commit-bot: I haz the power

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

4 years, 8 months ago (2016-03-30 07:12:47 UTC) #60

commit-bot: I haz the power

Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/builds/124005)

4 years, 8 months ago (2016-03-30 07:12:48 UTC) #61

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1659653002/470001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1659653002/470001

4 years, 8 months ago (2016-03-30 16:08:17 UTC) #63

commit-bot: I haz the power

Description was changed from ========== Pass MSE media track info from ChunkDemuxer to blink::SourceBuffer Also ...

4 years, 8 months ago (2016-03-30 17:13:54 UTC) #64

commit-bot: I haz the power

Description was changed from ========== Pass MSE media track info from ChunkDemuxer to blink::SourceBuffer Also ...

4 years, 8 months ago (2016-03-30 17:16:18 UTC) #66

commit-bot: I haz the power

Patchset 25 (id:??) landed as https://crrev.com/92333c0e5ca6b7768e5bcf6d41ff8fc733ebbfc1 Cr-Commit-Position: refs/heads/master@{#384014}

4 years, 8 months ago (2016-03-30 17:16:20 UTC) #67

esprehn

esprehn@chromium.org changed reviewers: + esprehn@chromium.org

4 years, 8 months ago (2016-04-12 18:44:08 UTC) #68

esprehn

https://codereview.chromium.org/1659653002/diff/470001/third_party/WebKit/public/platform/WebSourceBufferClient.h File third_party/WebKit/public/platform/WebSourceBufferClient.h (right): https://codereview.chromium.org/1659653002/diff/470001/third_party/WebKit/public/platform/WebSourceBufferClient.h#newcode21 third_party/WebKit/public/platform/WebSourceBufferClient.h:21: typedef std::tuple<WebMediaPlayer::TrackType, WebString, WebString, WebString, WebString> MediaTrackInfo; please define ...

4 years, 8 months ago (2016-04-12 18:44:10 UTC) #69

servolk

4 years, 8 months ago (2016-04-15 20:34:26 UTC) #70

Message was sent while issue was closed.

https://codereview.chromium.org/1659653002/diff/470001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebSourceBufferClient.h (right):

https://codereview.chromium.org/1659653002/diff/470001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebSourceBufferClient.h:21: typedef
std::tuple<WebMediaPlayer::TrackType, WebString, WebString, WebString,
WebString> MediaTrackInfo;
On 2016/04/12 18:44:10, esprehn wrote:
> please define structs instead of making huge tuples. The code in
> WebSourceBufferImpl::InitSegmentReceived should create structs to pass them
in.

Ok, but this CL has already been merged, so I've created a separate CL to update
this:
https://codereview.chromium.org/1897533002

https://codereview.chromium.org/1659653002/diff/470001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebSourceBufferClient.h:26: virtual
std::vector<WebMediaPlayer::TrackId> initializationSegmentReceived(const
std::vector<MediaTrackInfo>& tracks) = 0;
On 2016/04/12 18:44:10, esprehn wrote:
> This should be WebVector, not std::vector.

Done: https://codereview.chromium.org/1897533002

Issue 1659653002: Pass MSE media track info from ChunkDemuxer to blink::SourceBuffer (Closed)

Description

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : rebase #

Patch Set 4 : wip #

Patch Set 5 : rebase #

Patch Set 6 : wip #

Patch Set 7 : rebase #

Patch Set 8 : rebase #

Patch Set 9 : Cleanup and bugfixes #

Patch Set 10 : Added primitive init segment handling to make tracks usable in MSE #

Patch Set 11 : Fixed tests #

Patch Set 12 : Added basic unit test for a/v tracks created by MediaSource/SourceBuffer #

Patch Set 13 : rebase #

Patch Set 14 : CR feedback #

Patch Set 15 : CR feedback2 #

Patch Set 16 : Fixed createMediaTrack params #

Patch Set 17 : Avoid re-creating media track for each new init segment #

Patch Set 18 : Fixed tests #

Patch Set 19 : rebase #

Patch Set 20 : Pass media track info into initSegmentReceived via a vector of tuples #

Patch Set 21 : buildfix #

Patch Set 22 : CR feedback + test fix #

Patch Set 23 : CR feedback + test fix 2 #

Patch Set 24 : Rebase + fix non-oilpan codepath #

Patch Set 25 : nit #

Messages