Closed Bug 1044498 Opened 6 years ago Closed 6 years ago

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

Categories

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

34 Branch
x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: strobe, Assigned: kinetik)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 4 obsolete files)

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
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).
Blocks: MSE
Blocks: MSE
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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)
Blocks: 1000686
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 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)
Fix assertion by fetching the timecode scale at the correct time.
Attachment #8476564 - Attachment is obsolete: true
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)
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)
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)
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
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.
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.
Attachment #8477286 - Attachment description: Improve accuracy of WebM buffered parser. → p4: Improve accuracy of WebM buffered parser.
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 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+
Attachment #8477286 - Flags: review?(cajbir.bugzilla) → review+
Attachment #8477291 - Flags: review?(cajbir.bugzilla) → review+
Landed p2, p3, and p4.  leave-open for p1.
Keywords: leave-open
Keywords: leave-open
See Also: → 1058377
Depends on: 1058377
Duplicate of this bug: 1058377
Rebased against inbound.
Attachment #8477284 - Attachment is obsolete: true
Attachment #8478832 - Flags: review?(cajbir.bugzilla)
GetOffsetForTime was never correct.

Both callers to GetOffsetForTime were passing the time in the wrong units.
Attachment #8478846 - Flags: review?(cajbir.bugzilla)
No longer depends on: 1058377
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 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)
Attachment #8478830 - Flags: review?(cajbir.bugzilla) → review+
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+
(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.
(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.
Blocks: 1041374
(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.
Attachment #8478846 - Attachment is obsolete: true
Attachment #8479490 - Flags: review?(cajbir.bugzilla)
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.
Attachment #8479490 - Flags: review?(cajbir.bugzilla) → review+
Attachment #8479489 - Flags: review?(cajbir.bugzilla) → review+
(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.
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.
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
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 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+
(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.
Attachment #8477286 - Flags: checkin+
Attachment #8477285 - Flags: checkin+
Attachment #8477291 - Flags: checkin+
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?
(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.
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)
Attachment #8480326 - Flags: review?(cajbir.bugzilla) → review+
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Blocks: 1031026
Depends on: 1072483
You need to log in before you can comment on or make changes to this bug.