1261900 - WebM using MSE does not play properly with BlockDuration

Status: VERIFIED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[modmaker]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.110 Safari/537.36

Steps to reproduce:

Play the video below using MSE.  The last frame in each Cluster is a BlockGroup that contains the BlockDuration and a ReferenceBlock.

Navigate to the player link below.  Click 'Load Stream' and the video should load and play properly.  Below are the media links for the DASH manifest and the original WebM media.

Example Player:
http://shaka-player-demo.appspot.com/?asset=https://storage.googleapis.com/shaka-demo-assets/_bugs/firefox-webm-only/dash.mpd

DASH Manifest:
https://storage.googleapis.com/shaka-demo-assets/_bugs/firefox-webm-only/dash.mpd

WebM Video:
https://storage.googleapis.com/shaka-demo-assets/_bugs/firefox-webm-only/v-0576p-1000k-vp9.webm


Actual results:

The first segment is buffered correctly and plays.  However future segments do not report error, but do not extend the duration.  It is not possible to seek to these regions because it is considered unbuffered. This also does not produce multiple buffered ranges.

The player will buffer all of the segments and once it has it will call EndOfStream.  Because each append does not change the duration, the stream will be reduced to the 12 seconds that can play, even though the stream is 1 minute long.


Expected results:

The segments should continue to append successfully and change the duration.  The video should play for 1 minute.  Playing on Chrome will correctly play for 1 minute.  So does playing the video directly (i.e. not using MSE).

Comment 1

[Jean-Yves Avenard [:jya]]

kinetik the issue appears to be how nestegg demux.

The WebMBufferedParser properly identify all the clusters and the start and end time for each cluster along the offset position of each.
From this we create a MediaResource with just the init segment (byte 0 to 285) followed by the first cluster.
The first cluster is identified to go from 285 to 712421 (712136 bytes long)
We then use the WebMDemuxer to demux all samples. when reading the last packet, nestegg_read_packet returns 0 and set ctx->ancestor to NULL.

We then add to the MediaResource the second cluster (from position 712421 and is 712127 bytes long), and against start to demux.

however nestegg_read_packet returns -1 after starting second cluster because ctx->ancestor is not set.

:kinetik, could you have a look and see what's wrong with this? thank you.

Comment 2

[Jean-Yves Avenard [:jya]]

I made a small sample page:
http://people.mozilla.org/~jyavenard/tests/mse_webm/blockgroup.html

webm video also available there: https://people.mozilla.org/~jyavenard/mediatest/webm/v-0576p-1000k-vp9.webm

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1261900: [webm] P1. Use block duration if known. r?kinetik

This allows to resume demuxing once more data is added to the resource.

Review commit: https://reviewboard.mozilla.org/r/44255/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44255/

Comment 4

[Matthew Gregan [:kinetik]]

It's kind of unfortunate that we rely on the behaviour you describe where libnestegg sees an EOS but then is expected to continue parsing later as it's not the intended use of the API; once nestegg_read_packet returns 0 you're supposed to finish using that libnestegg context.

Bug 1257699 introduced the ctx->ancestor check to make the EOS handling more robust, before that ne_parse or ne_peek_element would just return EOS anyway - reverting that change doesn't seem to make the test behave any differently, anyway.

To make this work as-is, perhaps there needs to be a way for the io_read callback to signal that there's no more data available but EOS has not been reached, but the caller would have to promise this only happened at the end of a Cluster to ensure the parser could be resumed safely as libnestegg is not desired to resume parsing from arbitrary offsets.

Comment 5

[Jean-Yves Avenard [:jya]]

I had a quick shot at it, feel free to dismiss it entirely if you have a better approach.

The issue is that ne_parse always attempt to peek at the next element, and consider EOS as final (it will put the context in a way that can't be resumed).

I added a quick EOS detection so that if we hit EOS just as we attempt to read a new element id we abort early and leave the context as-is so that when new data is added to the resource and we attempt to demux it can resume nicely.

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8738023 [details]
MozReview Request: Bug 1261900: [webm] P1. Use block duration if known. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44255/diff/1-2/

Comment 7

[Jean-Yves Avenard [:jya]]

sorry for the conflict in comments. I had narrowed the EOS detection so actual error aren't treated as eos.

Comment 8

[Matthew Gregan [:kinetik]]

Comment on attachment 8738023 [details]
MozReview Request: Bug 1261900: [webm] P1. Use block duration if known. r?kinetik

https://reviewboard.mozilla.org/r/44255/#review40933

I don't think this is the approach I want to take dealing with this, but I haven't had time to think through the changes I would make to explain them fully here.  The rough idea is that the io_read callback would grow a new way to signal the no-data but not-EOS case.  Handling it at the point of trying to read the next element ID is on the right track, but it probably needs to only happen at points libnestegg expects to pause and resume parsing (aka "suspend" elements), where the caller is promising that the next element that will occur after signalling no-more-data would be either a (Simple)Block or a new Cluster and anything else would be treated as a stream error.

Comment 9

[Jean-Yves Avenard [:jya]]

Per IRC, bug 1257699 will likely regress current webm/MSE when dealing with partial media segment.

as not only would we hit EOS, but we would hit EOS on a possibly partial element.

Comment 10

[Jean-Yves Avenard [:jya]]

(In reply to Matthew Gregan [:kinetik] from comment #8)
> trying to read the next element ID is on the right track, but it probably
> needs to only happen at points libnestegg expects to pause and resume
> parsing (aka "suspend" elements), where the caller is promising that the
> next element that will occur after signalling no-more-data would be either a

Upon reflection, this is not something we can promise.

It is possible the MSE only receive a webm file, truncated at random. So where EOS occurs is never defined.

Test 30, of YouTube compliance test, with webm on.
https://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/2016.html?timestamp=1461237361297

will append media segment truncated at random (and just had a crash, which will be fixed in my P2).

YouTube also when seeking will append a media segment with about 1s. A typical youtube media segment is 5s long, it is my guess that to get about 1s worth, they simply look at the size and divide by 5.

As such, we must be able to resume anywhere within the file when demuxing.

An alternative, but that's not really pretty, would be that every time a new segment is appended, we would drop all frames currently demuxed and stored in the track buffer. Re-create a demuxer and redemux it all.

The only other demuxer MSE can deal with is the mp4 one, which can resume anywhere. So my preference at this stage is to make nestegg able to handle this case.
The downsides of clearing the trackbuffer each time being pretty awful.

Comment 11

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1261900: [MSE] P2. Prevent assertion if first media segment contains no usable frames. r=gerald

Encountered while using YouTube MSE/webm compliance test.

Review commit: https://reviewboard.mozilla.org/r/48611/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48611/

Comment 12

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1261900: P3. Re-add MediaDataDemuxer::GetEvictionOffset() API. r=gerald

API was removed in bug 1204419.

Review commit: https://reviewboard.mozilla.org/r/48613/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48613/

Comment 13

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1261900: [MSE] P4. Only evict no longer used data from resource. r=gerald

Review commit: https://reviewboard.mozilla.org/r/48615/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48615/

Comment 14

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1261900: [MSE/webm] P5. Re-add WebMTrackDemuxer::GetEvictionOffset. r=gerald

Review commit: https://reviewboard.mozilla.org/r/48617/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48617/

Comment 15

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1261900: [MSE/webm] P6. Don't unnecessarily calculate the next keyframe time. r?kinetik

MSE only uses the webm demuxer to demux all samples at once. Attempting to find the next keyframe as such always fail.

Review commit: https://reviewboard.mozilla.org/r/48619/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48619/

Comment 16

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1261900: Update in-tree libnestegg. r?kinetik

This commit is just for review purposes, the changes will go upstream.

Review commit: https://reviewboard.mozilla.org/r/48621/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48621/

Comment 17

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1261900: [MSE/webm] P8. Allow WebMDemuxer to resume demuxing even after encountering EOS. r?kinetik

Our nestegg demuxer is not designed to continue demuxing once it has hit an error or EOS. However, being able to resume demuxing is a fundamental requirement of the MSE architecture as it is possible to receive partial media segments (webm cluster) at any time or a complete media segment but incomplete webm file.

We get around this limitation by saving the nestegg memory context prior demuxing and detecting when nestegg hit EOS. If so, we restore the saved memory context so that nestegg can resume; in effect it's as if nestegg didn't hit EOS.

Review commit: https://reviewboard.mozilla.org/r/48623/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48623/

Comment 18

[Jean-Yves Avenard [:jya]]

Comment on attachment 8738023 [details]
MozReview Request: Bug 1261900: [webm] P1. Use block duration if known. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44255/diff/2-3/

Comment 19

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8744553 [details]
MozReview Request: Bug 1261900: [MSE] P2. Prevent assertion if first media segment contains no usable frames. r=gerald

https://reviewboard.mozilla.org/r/48611/#review45361

r+ with typo fixed in check-in comment: "should first media segment contained" -> "should first media segment contain" or "if first media segment contains".

Comment 20

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8744554 [details]
MozReview Request: Bug 1261900: P3. Re-add MediaDataDemuxer::GetEvictionOffset() API. r=gerald

https://reviewboard.mozilla.org/r/48613/#review45363

::: dom/media/MediaDataDemuxer.h:206
(Diff revision 1)
>  
>    virtual media::TimeIntervals GetBuffered() = 0;
>  
> +  // By default, it is assumed that the entire resource can be evicted once
> +  // all samples have been demuxed.
> +  virtual int64_t GetEvictionOffset(const media::TimeUnit& aTime)

Could this method be const? (Might not be possible if some overrides may modify their object, like the old MP4TrackDemuxer::GetEvictionOffset from before bug 1204419.)

Comment 21

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8744555 [details]
MozReview Request: Bug 1261900: [MSE] P4. Only evict no longer used data from resource. r=gerald

https://reviewboard.mozilla.org/r/48615/#review45365

Comment 22

[Jean-Yves Avenard [:jya]]

bug 1265043 is widevine, which doesn't support webm so can't be related to this bug.

Comment 23

[Jean-Yves Avenard [:jya]]

thank you for the reviews !

I used the old code as-is. We could make the webm GetEvictionOffset const, though the internal GetOffsetForTime isn't const and use a reentrant monitor (which could be removed now though)

Comment 24

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8744556 [details]
MozReview Request: Bug 1261900: [MSE/webm] P5. Re-add WebMTrackDemuxer::GetEvictionOffset. r=gerald

https://reviewboard.mozilla.org/r/48617/#review45367

::: dom/media/webm/WebMDemuxer.cpp:1038
(Diff revision 1)
>  
> +int64_t
> +WebMTrackDemuxer::GetEvictionOffset(const media::TimeUnit& aTime)
> +{
> +  int64_t offset;
> +  if (!mParent->GetOffsetForTime(aTime.ToNanoseconds(), &offset)) {

I'll just have to trust you about giving nanoseconds to GetOffsetForTime -- or you should find a reviewer who knows this.

Rant: I find it difficult to know which units are used around WebMDemuxer (e.g. just looking at one pair of member vars: mCodecDelay is in microseconds, but mSeekPreroll is in nanoseconds; Other things seem to use time scales).
I would really encourage trying to name variables&parameters with units where possible (e.g. mCodecDelay_ms, mSeekPreroll_ns, aTime_ns?), or even better: use different types or an auto-converting type for different time units, to statically catch potential errors.

Comment 25

[Jean-Yves Avenard [:jya]]

Comment on attachment 8738023 [details]
MozReview Request: Bug 1261900: [webm] P1. Use block duration if known. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44255/diff/3-4/

Comment 26

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744553 [details]
MozReview Request: Bug 1261900: [MSE] P2. Prevent assertion if first media segment contains no usable frames. r=gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48611/diff/1-2/

Comment 27

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744554 [details]
MozReview Request: Bug 1261900: P3. Re-add MediaDataDemuxer::GetEvictionOffset() API. r=gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48613/diff/1-2/

Comment 28

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744555 [details]
MozReview Request: Bug 1261900: [MSE] P4. Only evict no longer used data from resource. r=gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48615/diff/1-2/

Comment 29

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744556 [details]
MozReview Request: Bug 1261900: [MSE/webm] P5. Re-add WebMTrackDemuxer::GetEvictionOffset. r=gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48617/diff/1-2/

Comment 30

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744557 [details]
MozReview Request: Bug 1261900: [MSE/webm] P6. Don't unnecessarily calculate the next keyframe time. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48619/diff/1-2/

Comment 31

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744558 [details]
MozReview Request: Bug 1261900: Update in-tree libnestegg.  r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48621/diff/1-2/

Comment 32

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744559 [details]
MozReview Request: Bug 1261900: [MSE/webm] P8. Allow WebMDemuxer to resume demuxing even after encountering EOS. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48623/diff/1-2/

Comment 33

[Jean-Yves Avenard [:jya]]

(In reply to Gerald Squelart [:gerald] from comment #24)
> Comment on attachment 8744556 [details]
> MozReview Request: Bug 1261900: [MSE/webm] P5. Re-add
> WebMTrackDemuxer::GetEvictionOffset. r=gerald
> 
> https://reviewboard.mozilla.org/r/48617/#review45367
> 
> ::: dom/media/webm/WebMDemuxer.cpp:1038
> (Diff revision 1)
> >  
> > +int64_t
> > +WebMTrackDemuxer::GetEvictionOffset(const media::TimeUnit& aTime)
> > +{
> > +  int64_t offset;
> > +  if (!mParent->GetOffsetForTime(aTime.ToNanoseconds(), &offset)) {
> 
> I'll just have to trust you about giving nanoseconds to GetOffsetForTime --
> or you should find a reviewer who knows this.

This is not new code. I just reverted the commits from bug 1204419. Which is why I got you to review.

The WebM container store all timing information in nanoseconds (http://www.webmproject.org/docs/container/) and so most timing out there uses nanoseconds too. While everything is based on timescales, the timescale itself is in nanoseconds

That code got almost transferred straight from the WebMReader ; need to update to TimeUnit thorough the media code (there's a bug for that opened by Dan)

mCodecDelay is something used by the Opus codec, which expects microseconds. So it's stored in the webm in nanoseconds, and passed to opus in microseconds.

simply no? :)

Comment 34

[Matthew Gregan [:kinetik]]

Comment on attachment 8738023 [details]
MozReview Request: Bug 1261900: [webm] P1. Use block duration if known. r?kinetik

https://reviewboard.mozilla.org/r/44255/#review45623

We really need to merge the blockgroup nestegg branch and land that in Gecko before nestegg_packet_duration will work reliably.

::: dom/media/webm/WebMDemuxer.cpp:526
(Diff revision 4)
>    if (aType == TrackInfo::kAudioTrack) {
>      RefPtr<NesteggPacketHolder> next_holder(NextPacket(aType));
>      if (next_holder) {
>        next_tstamp = next_holder->Timestamp();
>        PushAudioPacket(next_holder);
> +    } else if (duration > 0) {

The block duration could be zero, theoretically, in which case we'd estimate an incorrect duration here.

::: dom/media/webm/WebMDemuxer.cpp:541
(Diff revision 4)
>    } else if (aType == TrackInfo::kVideoTrack) {
>      RefPtr<NesteggPacketHolder> next_holder(NextPacket(aType));
>      if (next_holder) {
>        next_tstamp = next_holder->Timestamp();
>        PushVideoPacket(next_holder);
> +    } else if (duration > 0) {

Ditto.

Comment 35

[Matthew Gregan [:kinetik]]

Comment on attachment 8744557 [details]
MozReview Request: Bug 1261900: [MSE/webm] P6. Don't unnecessarily calculate the next keyframe time. r?kinetik

https://reviewboard.mozilla.org/r/48619/#review45625

Comment 36

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/44255/#review45623

> The block duration could be zero, theoretically, in which case we'd estimate an incorrect duration here.

Right now, nestegg is almost always reporting 0. But having said that, when the block duration is 0, it just keep using the previous behavior; that is either estimate the duration based on the previous duration (plain webm) or wait until the next block to know the start time (MSE) except if there won't be future data, in which case it use the previous duration.

So if the duration is really 0, then the next block would have the same starting time as the current block. So it's all good.

Comment 37

[Matthew Gregan [:kinetik]]

(In reply to Jean-Yves Avenard [:jya] from comment #36)
> https://reviewboard.mozilla.org/r/44255/#review45623
> 
> > The block duration could be zero, theoretically, in which case we'd estimate an incorrect duration here.
> 
> Right now, nestegg is almost always reporting 0.

Yes, because of my first comment in comment 34.

> But having said that, when
> the block duration is 0, it just keep using the previous behavior; that is
> either estimate the duration based on the previous duration (plain webm) or
> wait until the next block to know the start time (MSE) except if there won't
> be future data, in which case it use the previous duration.
> 
> So if the duration is really 0, then the next block would have the same
> starting time as the current block. So it's all good.

Except if there's no next block, in which case it'll use the estimate and produce the wrong value.

Comment 38

[Matthew Gregan [:kinetik]]

Comment on attachment 8744559 [details]
MozReview Request: Bug 1261900: [MSE/webm] P8. Allow WebMDemuxer to resume demuxing even after encountering EOS. r?kinetik

https://reviewboard.mozilla.org/r/48623/#review45711

::: dom/media/webm/WebMDemuxer.h:221
(Diff revision 2)
>    Maybe<uint32_t> mLastSeenFrameHeight;
>    // This will be populated only if a resolution change occurs, otherwise it
>    // will be left as null so the original metadata is used
>    RefPtr<SharedTrackInfo> mSharedVideoTrackInfo;
> +
> +  // Set to true if nestegg read's callback hit EOS.

No "'s", I think

::: dom/media/webm/WebMDemuxer.cpp:677
(Diff revision 2)
>  RefPtr<NesteggPacketHolder>
>  WebMDemuxer::DemuxPacket()
>  {
> +  // Wrapper class that will save the current nestegg context and restore it
> +  // if necessary or discard the the state backup upon destruction.
> +  class NestEggState {

Annotate with MOZ_RAII. Doesn't matter much right now, since it's single use.

Comment 39

[Matthew Gregan [:kinetik]]

Comment on attachment 8744558 [details]
MozReview Request: Bug 1261900: Update in-tree libnestegg.  r?kinetik

https://reviewboard.mozilla.org/r/48621/#review45721

Needs to be submitted upstream and pulled back in via media/libnestegg/update.sh.

::: media/libnestegg/src/nestegg.c:179
(Diff revision 2)
>      char * s;
>      struct ebml_binary b;
>    } v;
>    enum ebml_type_enum type;
>    int read;
> +  int64_t position;

Call it offset

::: media/libnestegg/src/nestegg.c:864
(Diff revision 2)
>    ctx->ancestor = item->previous;
>    free(item);
>  }
>  
>  static int
> -ne_ctx_save(nestegg * ctx, struct saved_state * s)
> +ne_ctx_save(nestegg * ctx, struct nestegg_state * s)

Drop "struct" here and in ne_ctx_restore now that nestegg_state is a typedef.

::: media/libnestegg/src/nestegg.c:998
(Diff revision 2)
>    if (storage->read) {
> -    ctx->log(ctx, NESTEGG_LOG_DEBUG, "element %llx (%s) already read, skipping",
> +    ctx->log(ctx, NESTEGG_LOG_DEBUG, "element %llx (%s) already read",
>               desc->id, desc->name);
> +    /* We do not need to re-read the element, however we do need to move the IO
> +       position back to the original offset */
> +    if (storage->position >= 0) {

I'm not sure this is the right place to handle this; it feels a bit awkward to hide the IO stream offset save/restore down in here.  I think I'd prefer it hoisted up to the main parser loop, but I don't think it's easy to do that in a nice way right now... so maybe this is okay for now.

::: media/libnestegg/src/nestegg.c:1721
(Diff revision 2)
>  {
>    int r;
>    struct ebml_list_node * node = ctx->segment.cues.cue_point.head;
>    struct seek * found;
>    uint64_t seek_pos, id;
> -  struct saved_state state;
> +  struct nestegg_state state;

No "struct" here too

::: media/libnestegg/src/nestegg.c:1997
(Diff revision 2)
>    free(ctx->io);
>    free(ctx);
>  }
>  
>  int
> +nestegg_save_state(nestegg * ctx, struct nestegg_state ** state)

Ditto

::: media/libnestegg/src/nestegg.c:2026
(Diff revision 2)
> +  *state = s;
> +  return 0;
> +}
> +
> +void
> +nestegg_restore_state(nestegg * ctx, struct nestegg_state * s)

Ditto

Comment 40

[Jean-Yves Avenard [:jya]]

Comment on attachment 8738023 [details]
MozReview Request: Bug 1261900: [webm] P1. Use block duration if known. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44255/diff/4-5/

Comment 41

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744553 [details]
MozReview Request: Bug 1261900: [MSE] P2. Prevent assertion if first media segment contains no usable frames. r=gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48611/diff/2-3/

Comment 42

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744554 [details]
MozReview Request: Bug 1261900: P3. Re-add MediaDataDemuxer::GetEvictionOffset() API. r=gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48613/diff/2-3/

Comment 43

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744555 [details]
MozReview Request: Bug 1261900: [MSE] P4. Only evict no longer used data from resource. r=gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48615/diff/2-3/

Comment 44

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744556 [details]
MozReview Request: Bug 1261900: [MSE/webm] P5. Re-add WebMTrackDemuxer::GetEvictionOffset. r=gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48617/diff/2-3/

Comment 45

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744557 [details]
MozReview Request: Bug 1261900: [MSE/webm] P6. Don't unnecessarily calculate the next keyframe time. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48619/diff/2-3/

Comment 46

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744558 [details]
MozReview Request: Bug 1261900: Update in-tree libnestegg.  r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48621/diff/2-3/

Comment 47

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744559 [details]
MozReview Request: Bug 1261900: [MSE/webm] P8. Allow WebMDemuxer to resume demuxing even after encountering EOS. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48623/diff/2-3/

Comment 48

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/48621/#review45721

> I'm not sure this is the right place to handle this; it feels a bit awkward to hide the IO stream offset save/restore down in here.  I think I'd prefer it hoisted up to the main parser loop, but I don't think it's easy to do that in a nice way right now... so maybe this is okay for now.

Yes.

I believe the best method would be to drop this part of the code, and instead, in the restore_state parse the tree of objects and clear the read flag of all objects located after the stored context stream_offset.
I have to identify however, why is it following the restore, we enter ne_read_simple with IO::offset 1 byte past the earlier position.

Comment 49

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1261900: [webm] P9. Prevent null deref when webm logs are turned on. r?kinetik

Review commit: https://reviewboard.mozilla.org/r/49111/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49111/

Comment 50

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1261900: [nestegg] P10. Improve handling of resume. r?kinetik

Following IRC discussion, also remove the temporary leak of already read ebml_binary or string if any.

Review commit: https://reviewboard.mozilla.org/r/49113/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49113/

Comment 51

[Jean-Yves Avenard [:jya]]

Comment on attachment 8738023 [details]
MozReview Request: Bug 1261900: [webm] P1. Use block duration if known. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44255/diff/5-6/

Comment 52

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744553 [details]
MozReview Request: Bug 1261900: [MSE] P2. Prevent assertion if first media segment contains no usable frames. r=gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48611/diff/3-4/

Comment 53

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744554 [details]
MozReview Request: Bug 1261900: P3. Re-add MediaDataDemuxer::GetEvictionOffset() API. r=gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48613/diff/3-4/

Comment 54

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744555 [details]
MozReview Request: Bug 1261900: [MSE] P4. Only evict no longer used data from resource. r=gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48615/diff/3-4/

Comment 55

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744556 [details]
MozReview Request: Bug 1261900: [MSE/webm] P5. Re-add WebMTrackDemuxer::GetEvictionOffset. r=gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48617/diff/3-4/

Comment 56

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744557 [details]
MozReview Request: Bug 1261900: [MSE/webm] P6. Don't unnecessarily calculate the next keyframe time. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48619/diff/3-4/

Comment 57

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744558 [details]
MozReview Request: Bug 1261900: Update in-tree libnestegg.  r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48621/diff/3-4/

Comment 58

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744559 [details]
MozReview Request: Bug 1261900: [MSE/webm] P8. Allow WebMDemuxer to resume demuxing even after encountering EOS. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48623/diff/3-4/

Comment 59

[Matthew Gregan [:kinetik]]

Comment on attachment 8745819 [details]
MozReview Request: Bug 1261900: [webm] P9. Prevent null deref when webm logs are turned on. r?kinetik

https://reviewboard.mozilla.org/r/49111/#review45919

::: dom/media/webm/WebMDemuxer.cpp:1048
(Diff revision 1)
>  WebMTrackDemuxer::SkipToNextRandomAccessPoint(media::TimeUnit aTimeThreshold)
>  {
>    uint32_t parsed = 0;
>    bool found = false;
>    RefPtr<MediaRawData> sample;
> +  int64_t sampleTime;

Initialize sampleTime to avoid using an uninitialized value in the log in the case where sample used to be null.

Comment 60

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/49111/#review45919

> Initialize sampleTime to avoid using an uninitialized value in the log in the case where sample used to be null.

The value is only used if found is true. if so, the value is set  but will initialise for clarity

Comment 61

[Matthew Gregan [:kinetik]]

Comment on attachment 8745820 [details]
MozReview Request: Bug 1261900: [nestegg] P10. Improve handling of resume. r?kinetik

https://reviewboard.mozilla.org/r/49113/#review45933

r+ with the h_free abstraction issue fixed

::: media/libnestegg/src/nestegg.c:1801
(Diff revision 1)
> +  case TYPE_BINARY:
> +    storage = (struct ebml_type *) (data + desc->offset);
> +    if (storage->offset >= stream_offset) {
> +      storage->read = 0;
> +      if (desc->type == TYPE_BINARY) {
> +        h_free(storage->v.b.data);

Replace the h_frees with an ne_pool_free that takes the pointer and the pool pointer (which, with halloc, it won't use but some other impl might).

Comment 62

[Matthew Gregan [:kinetik]]

https://reviewboard.mozilla.org/r/49111/#review45919

> The value is only used if found is true. if so, the value is set  but will initialise for clarity

Oh right, missed that.  Doesn't matter either way, then.  Happy to leave it uninitialized.

Comment 63

[Jean-Yves Avenard [:jya]]

[Tracking Requested - why for this release]: Change from bug 1257699 will cause regressions (typically stalls) with MSE/WEBM sites such as youtube.

Need to ensure this goes into 48.

Comment 64

[Jean-Yves Avenard [:jya]]

As discussed on IRC, :kinetik will rework P7 and P8, P10 to be dropped

Comment 66

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 48 since the media team wants to uplift this to 48 once it lands. 

It would be best to land it (and test) on m-c first, and also, best to get this into aurora as soon as possible, since we usually don't want to do big multi-part uplifts right at the end of the release cycle.

Comment 67

[Matthew Gregan [:kinetik]]

Attachment: replacement p7

Please see description in the PR.

Comment 68

[Matthew Gregan [:kinetik]]

Attachment: replacement p8

Use nestegg_read_reset to resume parsing at last valid point.

Comment 69

[Matthew Gregan [:kinetik]]

Just libnestegg changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ddfc8eb3f17

Complete patchset:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4febe17bf5d6

Comment 70

[Jean-Yves Avenard [:jya]]

Comment on attachment 8753690 [details] [diff] [review]
replacement p8

Review of attachment 8753690 [details] [diff] [review]:
-----------------------------------------------------------------

I'm guessing you will change the commit log.

Comment 73

[Matthew Gregan [:kinetik]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7df96ed6d0ec
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/842f55f012e6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bc61f391d06a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bf9030f86271
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/57a920361b81
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/57b4fd3e96d4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e1f81f912407
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8855ffc347b6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a5ae64c13f6f

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7df96ed6d0ec
https://hg.mozilla.org/mozilla-central/rev/842f55f012e6
https://hg.mozilla.org/mozilla-central/rev/bc61f391d06a
https://hg.mozilla.org/mozilla-central/rev/bf9030f86271
https://hg.mozilla.org/mozilla-central/rev/57a920361b81
https://hg.mozilla.org/mozilla-central/rev/57b4fd3e96d4
https://hg.mozilla.org/mozilla-central/rev/e1f81f912407
https://hg.mozilla.org/mozilla-central/rev/8855ffc347b6
https://hg.mozilla.org/mozilla-central/rev/a5ae64c13f6f

Comment 75

[Sylvestre Ledru [:Sylvestre]]

Matthew, Jean-Yves, what are your plans wrt 48? The merge to beta will arrive soon.

Comment 76

[Matthew Gregan [:kinetik]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #75)
> Matthew, Jean-Yves, what are your plans wrt 48? The merge to beta will
> arrive soon.

I reworked the fix from bug 1257699 in bug 1271866 to avoid the regression and landed that on aurora.  That, at least, avoids making the problem worse than it was.  I don't know if we'll want to uplift the complete fix from this bug as it's fairly large and thus carries some risk.

Comment 77

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc7692916d6
Remove obsolete patch file.  r=me

Comment 78

[Jean-Yves Avenard [:jya]]

I think letting it ride the train is fine; if we really wanted to, uplifting to 48 will do if we really wanted to let shaka/google player work.

As :kinetik mentioned, now that bug 1257699 got reverted there's no regression occurring.

Comment 79

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7fc7692916d6

Comment 80

[Gerry Chang [:gchang]]

Based on comment 78, let it ride the train and won't fix in 48.

Comment 82

[Ciprian Georgiu, Desktop QA]

I've managed to reproduce this issue on an older Nightly build 48.0a1 from 2016-04-04 using Windows 10 x64. 
This issue is verified fixed on Beta 49.0-build2 (2016-09-07)on the following OS's:
- Windows 10 x64
- Ubuntu 16.04 LTS x64
- Mac OS X 10.10.5(Yosemite)