1044498 - Media Source: discontiguous appends are not processed without calling abort()

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P2)

Description

[strobe]

It is legal under the Media Source spec (or at least my reading of it) to perform the following sequence of appends to a Media Source source buffer, where all segments are from the same media and share the same initialization segment data:

- Append an initialization segment.
- Append a complete media segment covering the time [0,5].
- Append a complete media segment covering the time [5,10].
- Play until t=8. 
- Append a complete media segment covering the time [10,15]. 
  (In Firefox, this will result in the segment from [0,5] being evicted.)
- Seek to t=3.
- Append a complete media segment covering the time [0,5].

In Firefox, the last step does not result in media data being added to the timeline; buffered ranges at the end of this sequence are [5,15], not [0,15] as would be expected. Inserting an abort before the final append causes the final buffered ranges to be [0,5], which is more correct.

This currently causes the YouTube player to stall on backward seeks in some conditions, since it only calls abort() as often as necessary in the spec (in particular, when changing seek locations while the segment parser loop is in the PARSING_MEDIA_SEGMENT state).

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: WIP: Handle overlapped SourceBuffer appends. (WebM only)

Use the existing WebM buffered range parser to extract start and end timecodes when data is appended to a SourceBuffer.  Use the timecodes to detect when data overlaps the existing timeline and generate an internal decoder reset.

Comment 2

[Matthew Gregan [:kinetik]]

Attachment: Handle overlapped SourceBuffer appends by spinning up a new decoder. Only implemented for WebM so far. v2

Introduce timestamp parsing to ContainerBuffer with only a WebM implementation for now.  The MP4 implementation returns false and retains the existing behaviour.

Use the timestamps parsed from a segment appended to a SourceBuffer to detect non-contiguous appends.  The demuxer can handle non-contiguous appends with increasing timestamps, so detect only the case where timestamps decrease and spin up a new decoder to handle the segment.

Extend the existing WebM BufferedParser code to find SegmentInfo elements and parse the TimecodeScale element.  Modify the rest of the code to require a TimecodeScale and pre-scale the timecodes rather than forcing the end user to scale them.  Since only the first BufferedParser can be expected to see a SegmentInfo, initialize secondary BufferedParsers with the scale from the first parser.

Comment 3

[cajbir (:cajbir)]

With this patch applied on Mozilla Central I get the following assertion as soon as I go to http://cd.pn/mse/ytdemo/dash-player.html?url=http://cd.pn/mse/ytdemo/feelings2.mpd:

Assertion failure: mGotTimecodeScale, at content/media/webm/WebMBufferedParser.cpp:193

Comment 4

[cajbir (:cajbir)]

Comment on attachment 8476564 [details] [diff] [review]
Handle overlapped SourceBuffer appends by spinning up a new decoder.  Only implemented for WebM so far.  v2

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

Code looks good. Just needs assertion fixed.

Comment 5

[Matthew Gregan [:kinetik]]

Attachment: p1: Handle overlapped SourceBuffer appends by spinning up a new decoder. Only implemented for WebM so far. v3

Fix assertion by fetching the timecode scale at the correct time.

Comment 6

[Matthew Gregan [:kinetik]]

Attachment: p3: WebMBufferedReader used mBlockOffset in some places where it was intended to use mBlockTimecode.

Comment 7

[Matthew Gregan [:kinetik]]

Attachment: p4: Improve accuracy of WebM buffered parser.

Store the end offset of the block, rather than the start offset, since a
frame is only usable if we have all of it.  Also compute the start of the
buffered range based on the offset of the block's resync point, since we
can't decode frames unless we have their cluster to resync at and compute
the absolute timecode from.

Comment 8

[Matthew Gregan [:kinetik]]

Attachment: p2: Extend WebMBufferedParser to handle timecode scale elements in locations other than the first subelement of the segment info element.

The initial patch for this bug required TimecodeScale to be the first element inside SegmentInfo, but that's not required by the spec.  This extends the parser (and deletes a bunch of code, yay!) to remove that assumption from the parser.

This applies before the "improve accuracy" patch, I attached them in the wrong order sorry.

Comment 9

[cajbir (:cajbir)]

Comment on attachment 8477284 [details] [diff] [review]
p1: Handle overlapped SourceBuffer appends by spinning up a new decoder.  Only implemented for WebM so far.  v3

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

This patch breaks seeking. Steps to reproduce breakage:

1) Visit http://cd.pn/mse/ytdemo/dash-player.html?url=http://cd.pn/mse/ytdemo/feelings2.mpd.
2) Once playback starts, seek to a time point.
3) Without this patch, seek works.
4) With this patch, Seek never continues - it is stuck buffering.

Other than that issue changes look ok but seeking breakage prevents me from r+ing.

::: content/media/mediasource/SourceBuffer.cpp
@@ +510,5 @@
> +  double start, end;
> +  if (mParser->ParseStartAndEndTimestamps(aData, aLength, start, end)) {
> +    if (start <= mLastParsedTimestamp) {
> +      // This data is earlier in the timeline than data we have already
> +      // processed, so we must create a new decode to handle the decoding.

s/decode/decoder/

Comment 10

[cajbir (:cajbir)]

Comment on attachment 8477285 [details] [diff] [review]
p3: WebMBufferedReader used mBlockOffset in some places where it was intended to use mBlockTimecode.

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

This patch doesn't build standalone - it requires changes to WebMReader.cpp in part 4 to build as this patch still uses CLUSTER_START in that file. It might be worth rolling that into this patch to allow it to build if you plan to land these separately.

::: content/media/webm/WebMBufferedParser.cpp
@@ +225,5 @@
>    *aEndTime += mTimeMapping[end].mTimecode - mTimeMapping[end - 1].mTimecode;
>    return true;
>  }
>  
> +bool WebMBufferedState::GetOffsetForTime(uint64_t aTime, int64_t* aOffset);

Spurious semicolon at end of line

Comment 11

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e1f72905179
https://hg.mozilla.org/integration/mozilla-inbound/rev/c40f86ae9b17
https://hg.mozilla.org/integration/mozilla-inbound/rev/f277da7fa9fa

Comment 12

[Matthew Gregan [:kinetik]]

Landed p2, p3, and p4.  leave-open for p1.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e1f72905179
https://hg.mozilla.org/mozilla-central/rev/c40f86ae9b17
https://hg.mozilla.org/mozilla-central/rev/f277da7fa9fa

Comment 15

[Matthew Gregan [:kinetik]]

Attachment: p5: Save init segment for reuse during overlapped appends.

Comment 16

[Matthew Gregan [:kinetik]]

Attachment: p1: Handle overlapped SourceBuffer appends by spinning up a new decoder. Only implemented for WebM so far. v4

Rebased against inbound.

Comment 17

[Matthew Gregan [:kinetik]]

Attachment: p1.5: Fix a bunch of WebM buffered range bugs.

GetOffsetForTime was never correct.

Both callers to GetOffsetForTime were passing the time in the wrong units.

Comment 18

[cajbir (:cajbir)]

With these patches applied I get a crash running http://cd.pn/mse/ytdemo/dash-player.html?url=http://cd.pn/mse/ytdemo/feelings_vp9-20130806-manifest.mpd and attempting to seek:

#8  mozilla::MediaSourceReader::CreateSubDecoder (this=0x7f2565e27800, aType=...) at content/media/mediasource/MediaSourceReader.cpp:348
#9  0x00007f2591e8406c in mozilla::MediaSourceDecoder::CreateSubDecoder (this=<optimized out>, aType=...) at content/media/mediasource/MediaSourceDecoder.cpp:128
#10 0x00007f2591e87a98 in mozilla::dom::SourceBuffer::InitNewDecoder (this=this@entry=0x7f2565db7f80) at content/media/mediasource/SourceBuffer.cpp:444
#11 0x00007f2591e88711 in mozilla::dom::SourceBuffer::AppendData (this=this@entry=0x7f2565db7f80, 
    aData=0x7f256c3ae000 "\032Eߣ\237B\206\201\001B\367\201\001B\362\201\004B\363\201\bB\202\204webmB\207\201\002B\205\201\002\030S\200g\001", aLength=4452, aRv=...)
    at content/media/mediasource/SourceBuffer.cpp:521

Assertion printed in console is:

WARNING: Decoder=7f2565261090 Decode error, changed state to SHUTDOWN due to error: file /home/chris/src/gecko-dev/content/media/MediaDecoderStateMachine.cpp, line 1839
Assertion failure: GetTaskQueue(), at /home/chris/src/gecko-dev/content/media/mediasource/MediaSourceReader.cpp:337

Comment 19

[cajbir (:cajbir)]

Comment on attachment 8478846 [details] [diff] [review]
p1.5: Fix a bunch of WebM buffered range bugs.

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

::: content/media/webm/WebMBufferedParser.cpp
@@ +251,5 @@
> +  uint64_t time = aTime;
> +  if (time > 0) {
> +    time = time - 1;
> +  }
> +  uint32_t idx = mTimeMapping.IndexOfFirstElementGt(time, TimeComparator());

mTimeMapping is sorted by offset according to the .h file. Wouldn't it need to be sorted by time for this to work?

Comment 20

[cajbir (:cajbir)]

Comment on attachment 8478832 [details] [diff] [review]
p1: Handle overlapped SourceBuffer appends by spinning up a new decoder.  Only implemented for WebM so far. v4

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

::: content/media/webm/WebMReader.cpp
@@ +1059,3 @@
>          // If this range extends to the end of the file, the true end time
>          // is the file's duration.
> +        if (resource->IsDataCachedToEndOfResource(ranges[index].mStart) && mContext) {

Check mContext first to save the call to IsDataCachedToEndOfResource if it is null.

Comment 21

[Matthew Gregan [:kinetik]]

(In reply to cajbir (:cajbir) from comment #19)
> mTimeMapping is sorted by offset according to the .h file. Wouldn't it need
> to be sorted by time for this to work?

Good question.  Same question applies to the place(s) where I'm using SyncComparator.

It works anyway because timecodes must be monotonically increasing within the file, so any file sorted by offset is also effectively sorted by timecode (and sync offset).  I can make this clearer with a comment, or rework the mapping to handle this in a cleaner way.

Comment 22

[cajbir (:cajbir)]

(In reply to Matthew Gregan [:kinetik] from comment #21)
> It works anyway because timecodes must be monotonically increasing within
> the file, so any file sorted by offset is also effectively sorted by
> timecode (and sync offset).  I can make this clearer with a comment, or
> rework the mapping to handle this in a cleaner way.

Comment is fine, thanks.

Comment 23

[Matthew Gregan [:kinetik]]

(In reply to cajbir (:cajbir) from comment #18)
> WARNING: Decoder=7f2565261090 Decode error, changed state to SHUTDOWN due to
> error: file
> /home/chris/src/gecko-dev/content/media/MediaDecoderStateMachine.cpp, line
> 1839
> Assertion failure: GetTaskQueue(), at
> /home/chris/src/gecko-dev/content/media/mediasource/MediaSourceReader.cpp:337

This is caused by bug 1058428.  We seek successfully in the audio stream but the video stream seek fails, so the MDSM is set to SHUTDOWN state.

A new init segment is then appended to the video SourceBuffer (to service the seek) and we assert trying to dispatch a task to the MSDN task queue.  We should error rather than assert, obviously, so I'll add an IsShutdown test to CreateSubDecoder.

Comment 24

[Matthew Gregan [:kinetik]]

Attachment: p6: Return a null decoder when CreateSubDecoder is called in Shutdown state.

Comment 25

[Matthew Gregan [:kinetik]]

Attachment: p1.5: Fix a bunch of WebM buffered range bugs. v2

Comment 26

[cajbir (:cajbir)]

(In reply to Matthew Gregan [:kinetik] from comment #23)
> This is caused by bug 1058428.  We seek successfully in the audio stream but
> the video stream seek fails, so the MDSM is set to SHUTDOWN state.

With this fix and the fix from bug 1058428 the seek in feelings_vp9 doesn't crash but now fails with a "video is corrupt" message.

Comment 27

[cajbir (:cajbir)]

I also got the following crash seeking in youtube:

Assertion failure: mInitData.Length() > 0, at /home/chris/src/gecko-dev/content/media/mediasource/SourceBuffer.cpp:70

The video was http://www.youtube.com/watch?v=3V7wWemZ_cs.

Comment 28

[cajbir (:cajbir)]

Stack from crash:

#5  0x00007fc2f9c88e67 in mozilla::ContainerParser::InitData (this=<optimized out>) at content/media/mediasource/SourceBuffer.cpp:70
#6  0x00007fc2f9c889ed in mozilla::dom::SourceBuffer::AppendData (this=this@entry=0x7fc2ddbb5030, 
    aData=0x7fc2c03b1000 "\243\330x=1\251\255\220\276S\255T\031<\253\233a\026\245\231\360\354\f\354\274\021Nw\026lk83\240\023\034\256\361\243\253\317\017\236\314\020q)\310\022`\n\212\006\273yj\246\273\004\242BUeL\356\023\vcKjhʳ\346\360f\352\340KN*\364C\373K\376O\001\203\027ME\342\005=iN\271\315\331\020", <incomplete sequence \311>, aLength=228021, aRv=...)
    at content/media/mediasource/SourceBuffer.cpp:553
#7  0x00007fc2f9c88cc6 in mozilla::dom::SourceBuffer::AppendBuffer (this=this@entry=0x7fc2ddbb5030, aData=..., aRv=...) at content/media/mediasource/SourceBuffer.cpp:314

Comment 29

[Matthew Gregan [:kinetik]]

Attachment: p5.5: Bug 1044498 - Improve calculation of init segment region.

This is much better.  We know where the cluster starts from mSyncOffset, so anything before that is the init segment.

Comment 30

[cajbir (:cajbir)]

Comment on attachment 8479594 [details] [diff] [review]
p5.5: Bug 1044498 - Improve calculation of init segment region.

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

::: content/media/mediasource/SourceBuffer.cpp
@@ +133,5 @@
> +      }
> +      MSE_DEBUG("WebMContainerParser(%p)::ParseStartAndEndTimestamps: Stashed init of %u bytes.",
> +                this, length);
> +
> +      mInitData.ReplaceElementsAt(0, mInitData.Length(), aData, length);

I think we need to assert or handle the case if length > aLength, given that mSyncOffset is from an external source.

Comment 31

[Matthew Gregan [:kinetik]]

(In reply to cajbir (:cajbir) from comment #30)
> I think we need to assert or handle the case if length > aLength, given that
> mSyncOffset is from an external source.

It's not really from an external source; it's calculated by WebMBufferedParser based on the offset within aData that a cluster is parsed.  There's no external data involved in that deriving the offset beyond "were these bytes detected within aData[0:aLength]".  But an assert is free.

Comment 32

[cajbir (:cajbir)]

Comment on attachment 8477291 [details] [diff] [review]
p2: Extend WebMBufferedParser to handle timecode scale elements in locations other than the first subelement of the segment info element.

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

::: content/media/webm/WebMBufferedParser.cpp
@@ +129,2 @@
>      case READ_TIMECODESCALE:
> +      mTimecodeScale = mVInt.mValue;

Do we need to set mGotTimecodeScale here?

Comment 33

[Matthew Gregan [:kinetik]]

(In reply to cajbir (:cajbir) from comment #32)
> Do we need to set mGotTimecodeScale here?

mTimecodeScale is initialized with a default value from the WebM spec.  We need to know that we have either read the timecode scale from the data or decided to use the default.  Once we reach the segment info (which timecode scale is a subelement of), we know we are either going to read the timecode scale from the segment info or (if it's not present) use the default value.  So mGotTimecodeScale is set when the segment info is parsed.

Comment 34

[Matthew Gregan [:kinetik]]

Attachment: p5.6 Bug 1044498 - Pass saved init segment to decoder's NotifyDataAvailable.

This fixes the mGotTimecodeScale assert you hit.  I forgot to pass the saved init segment to NotifyDataAvailable, so the decoder's mBufferedState would not be able to compute the timecode scale and would then assert when a non-init segment was appended.

Comment 35

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03978f8ca2b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/676e7fc0984e
https://hg.mozilla.org/integration/mozilla-inbound/rev/adcb3f7a437b
https://hg.mozilla.org/integration/mozilla-inbound/rev/e50fd1470da5
https://hg.mozilla.org/integration/mozilla-inbound/rev/35457d37cebf
https://hg.mozilla.org/integration/mozilla-inbound/rev/4eb947a21953
https://hg.mozilla.org/integration/mozilla-inbound/rev/756ce4591f67

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/03978f8ca2b5
https://hg.mozilla.org/mozilla-central/rev/676e7fc0984e
https://hg.mozilla.org/mozilla-central/rev/adcb3f7a437b
https://hg.mozilla.org/mozilla-central/rev/e50fd1470da5
https://hg.mozilla.org/mozilla-central/rev/35457d37cebf
https://hg.mozilla.org/mozilla-central/rev/4eb947a21953
https://hg.mozilla.org/mozilla-central/rev/756ce4591f67