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

RESOLVED FIXED in mozilla34

Status

()

defect
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: strobe, Assigned: kinetik)

Tracking

(Blocks 1 bug)

34 Branch
mozilla34
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 4 obsolete attachments)

5.28 KB, patch
cajbir
: review+
kinetik
: checkin+
Details | Diff | Splinter Review
8.42 KB, patch
cajbir
: review+
kinetik
: checkin+
Details | Diff | Splinter Review
17.47 KB, patch
cajbir
: review+
kinetik
: checkin+
Details | Diff | Splinter Review
6.69 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
12.67 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
1.04 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
5.36 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
1.87 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
2.14 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
Reporter

Description

5 years ago
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).
Assignee

Updated

5 years ago
Blocks: MSE
Assignee

Updated

5 years ago
Blocks: MSE
Assignee

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee

Comment 1

5 years ago
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.
Assignee

Comment 2

5 years ago
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.
Attachment #8474983 - Attachment is obsolete: true
Attachment #8476564 - Flags: review?(cajbir.bugzilla)
Assignee

Updated

5 years ago
Blocks: 1000686

Comment 3

5 years ago
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

5 years ago
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.
Attachment #8476564 - Flags: review?(cajbir.bugzilla)
Assignee

Comment 5

5 years ago
Fix assertion by fetching the timecode scale at the correct time.
Assignee

Updated

5 years ago
Attachment #8476564 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8477284 - Attachment description: Handle overlapped SourceBuffer appends by spinning up a new decoder. Only implemented for WebM so far. v2 → Handle overlapped SourceBuffer appends by spinning up a new decoder. Only implemented for WebM so far. v3
Attachment #8477284 - Flags: review?(cajbir.bugzilla)
Assignee

Comment 7

5 years ago
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.
Attachment #8477286 - Flags: review?(cajbir.bugzilla)
Assignee

Comment 8

5 years ago
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.
Attachment #8477291 - Flags: review?(cajbir.bugzilla)
Assignee

Updated

5 years ago
Attachment #8477284 - Attachment description: Handle overlapped SourceBuffer appends by spinning up a new decoder. Only implemented for WebM so far. v3 → p1: Handle overlapped SourceBuffer appends by spinning up a new decoder. Only implemented for WebM so far. v3
Assignee

Updated

5 years ago
Attachment #8477291 - Attachment description: Extend WebMBufferedParser to handle timecode scale elements in locations other than the first subelement of the segment info element. → p2: Extend WebMBufferedParser to handle timecode scale elements in locations other than the first subelement of the segment info element.
Assignee

Updated

5 years ago
Attachment #8477285 - Attachment description: WebMBufferedReader used mBlockOffset in some places where it was intended to use mBlockTimecode. → p3: WebMBufferedReader used mBlockOffset in some places where it was intended to use mBlockTimecode.
Assignee

Updated

5 years ago
Attachment #8477286 - Attachment description: Improve accuracy of WebM buffered parser. → p4: Improve accuracy of WebM buffered parser.

Comment 9

5 years ago
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/
Attachment #8477284 - Flags: review?(cajbir.bugzilla)

Comment 10

5 years ago
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
Attachment #8477285 - Flags: review?(cajbir.bugzilla) → review+

Updated

5 years ago
Attachment #8477286 - Flags: review?(cajbir.bugzilla) → review+

Updated

5 years ago
Attachment #8477291 - Flags: review?(cajbir.bugzilla) → review+
Assignee

Comment 12

5 years ago
Landed p2, p3, and p4.  leave-open for p1.
Keywords: leave-open
Assignee

Updated

5 years ago
Keywords: leave-open
See Also: → 1058377
Assignee

Updated

5 years ago
Depends on: 1058377
Assignee

Updated

5 years ago
Duplicate of this bug: 1058377
Assignee

Comment 16

5 years ago
Rebased against inbound.
Attachment #8477284 - Attachment is obsolete: true
Attachment #8478832 - Flags: review?(cajbir.bugzilla)
Assignee

Comment 17

5 years ago
GetOffsetForTime was never correct.

Both callers to GetOffsetForTime were passing the time in the wrong units.
Attachment #8478846 - Flags: review?(cajbir.bugzilla)
Assignee

Updated

5 years ago
No longer depends on: 1058377

Comment 18

5 years ago
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

5 years ago
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?
Attachment #8478846 - Flags: review?(cajbir.bugzilla)

Updated

5 years ago
Attachment #8478830 - Flags: review?(cajbir.bugzilla) → review+

Comment 20

5 years ago
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.
Attachment #8478832 - Flags: review?(cajbir.bugzilla) → review+
Assignee

Comment 21

5 years ago
(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

5 years ago
(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.

Updated

5 years ago
Blocks: 1041374
Assignee

Comment 23

5 years ago
(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.
Assignee

Comment 25

5 years ago
Attachment #8478846 - Attachment is obsolete: true
Attachment #8479490 - Flags: review?(cajbir.bugzilla)
Assignee

Updated

5 years ago
Attachment #8479489 - Attachment description: Return a null decoder when CreateSubDecoder is called in Shutdown state. → p6: Return a null decoder when CreateSubDecoder is called in Shutdown state.

Updated

5 years ago
Attachment #8479490 - Flags: review?(cajbir.bugzilla) → review+

Updated

5 years ago
Attachment #8479489 - Flags: review?(cajbir.bugzilla) → review+

Comment 26

5 years ago
(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

5 years ago
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

5 years ago
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
Assignee

Comment 29

5 years ago
This is much better.  We know where the cluster starts from mSyncOffset, so anything before that is the init segment.
Attachment #8479594 - Flags: review?(cajbir.bugzilla)

Comment 30

5 years ago
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.
Attachment #8479594 - Flags: review?(cajbir.bugzilla) → review+
Assignee

Comment 31

5 years ago
(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.
Assignee

Updated

5 years ago
Attachment #8477286 - Flags: checkin+
Assignee

Updated

5 years ago
Attachment #8477285 - Flags: checkin+
Assignee

Updated

5 years ago
Attachment #8477291 - Flags: checkin+

Comment 32

5 years ago
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?
Assignee

Comment 33

5 years ago
(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.
Assignee

Comment 34

5 years ago
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.
Attachment #8480326 - Flags: review?(cajbir.bugzilla)

Updated

5 years ago
Attachment #8480326 - Flags: review?(cajbir.bugzilla) → review+
Assignee

Updated

5 years ago
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Assignee

Updated

5 years ago
Blocks: 1031026
Assignee

Updated

5 years ago
Depends on: 1072483
You need to log in before you can comment on or make changes to this bug.