Summary: Stall in MSE when using webm

RESOLVED FIXED in Firefox 42

Status

()

defect
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks 1 bug)

Trunk
mozilla43
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed, firefox43 fixed)

Details

Attachments

(10 attachments, 2 obsolete attachments)

6.75 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
4.51 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
1.32 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
6.94 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
10.60 KB, patch
gerald
: review+
Details | Diff | Splinter Review
6.61 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
10.26 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
4.87 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
5.38 KB, patch
karlt
: review+
Details | Diff | Splinter Review
8.13 KB, patch
karlt
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
STR:

1: Enable webm mediasource (media.mediasource.webm.enabled=true)
2: Go to https://www.youtube.com/watch?v=XqLTe8h0-jo
3: Seek after 1:00 and wait until seek completes and playback resume
4: Change resolution to 720p
5: Wait until playback restart
5: Seek to exactly 0:00 

Spinner will show, playback won't restart.

If you couldn't reproduce the problem at first go. I find that restarting the steps from 3 will work (changing the resolution, even to the same one, clear the sourcebuffer)
(Assignee)

Comment 1

4 years ago
Got the feeling it's due to bug 1192517 ; When seeking at 0:00 YouTube loads a partial media segment with 1s worth of video (well, they do for MP4 at least)
See Also: → 1192517
(Assignee)

Comment 2

4 years ago
hit an assert attempting to reproduce it with bug 1192517

hit an assertion in constructing a TimeInterval
mozilla::MediaData	mozilla::MediaData		
mRefCnt	mozilla::ThreadSafeAutoRefCnt		
mType	const mozilla::MediaData::Type	RAW_DATA	RAW_DATA
mOffset	int64_t	57794379	57794379
mTime	int64_t	107960000	107960000
mTimecode	int64_t	107960000	107960000
mDuration	int64_t	-107960000	-107960000
mFrames	uint32_t	0	0
mKeyframe	bool	false	false
mDiscontinuity	bool	false	false

this is due to the duration being negative
(Assignee)

Comment 3

4 years ago
This is definitely a demuxing issue ; most likely in the WebMContainerParser.

After seeking to 0; we see a 10s video segment being appended ; and then audio:

2097021696[100501df0]: TrackBuffersManager(12ca28200:audio/webm)::AppendData: Appending 11231 bytes

However:
970760192[13b5173e0]: TrackBuffersManager(12ca28200:audio/webm)::SegmentParserLoop: Found invalid or incomplete data.

It is seen as neither an init segment nor media segment. No data is further appended until we start seeking. However there's no audio data to seek to -> stall.

I'll extract the 11231 bytes tomorrow (they are easy to get as this is the segment you get when seeking to 0)
(Assignee)

Comment 4

4 years ago
Here is a narrowed case:

http://people.mozilla.org/~jyavenard/tests/mse_webm/youtube-stall.html

using chrome, we end up with a buffered range of {{ 0,6.041 }

however, using nightly the data is skipped, and we have an empty buffered range

I built a webm file using those two segments:
http://people.mozilla.org/~jyavenard/tests/mediatest/webm/segment.webm

it plays fine on itself.
Flags: needinfo?(j)
(Assignee)

Comment 5

4 years ago
ok.. the above works now with patch 1192517 applied.

But I can still reproduce the stall with that video. So will try to narrow another case.
(Assignee)

Comment 6

4 years ago
I had the extended case I had found earlier reproducing the problem:

updating html shows a buffered range of:
Chrome:
video.buffered ={{ 0,6.041 }{ 80.001,169.021 }}

vs: Nightly (with 1192517)
video.buffered ={{ 80.001,150.001 }}
(Assignee)

Comment 7

4 years ago
Update comment to describe more accurately what's being done.
Attachment #8649038 - Flags: review?(kinetik)
Can you explain why you think this change is necessary?  The WebM spec requires that timestamps are monotonically increasing, so why are we seeing a situation where that is not the case?
Flags: needinfo?(jyavenard)
(Assignee)

Comment 9

4 years ago
Due to how youtube uses it.

They append buffers in any order.

In this particular test case above this is the appendBuffer you get when you seek from 85s to 0s.

they append an init segment, then append from 80s and suddenly append a partial segment from 0-6 and only then seek to 0s.

There's no other way to tell that there's a discontinuity as this is only done *after* demuxing the samples.

Without this change, as the next sample is found earlier, we end up with negative durations which trigger an assert in the TimeRanges.
Flags: needinfo?(jyavenard)
(Assignee)

Updated

4 years ago
See Also: → 1195600
(Assignee)

Comment 10

4 years ago
The core reason for the stall is bug 1192517
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1192517
(Assignee)

Comment 11

4 years ago
Comment on attachment 8649038 [details] [diff] [review]
[webm] Do not always rely on next sample's start time to determine sample's end time.

As discussed per IRC.

The TrackBufferManagers will be modified to always provide monotonic data.
Flags: needinfo?(j)
Attachment #8649038 - Flags: review?(kinetik) → review-
(Assignee)

Comment 12

4 years ago
Actually no.

Let's make this bug purely about the TrackBuffersManager and creating new demuxers when a discontinuity in the data is identified.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Assignee)

Updated

4 years ago
Assignee: nobody → jyavenard
(Assignee)

Updated

4 years ago
Summary: Stall in Youtube when using webm → Ensure TrackBuffersManager only feed monotonic data to its demuxers
(Assignee)

Comment 13

4 years ago
WebMContainerParser was incorrectly reporting webm blocks rather than clusters, causing the webm demuxer to later fail to parse the remaining data.
Attachment #8649677 - Flags: review?(kinetik)
(Assignee)

Comment 14

4 years ago
MSE may input partial media segment, which could cause the WebMDemuxer and libnestegg to error upon encountering an incomplete block which can't be recovered from.
this will allow to limit read to known complete blocks.
Attachment #8649679 - Flags: review?(kinetik)
(Assignee)

Comment 15

4 years ago
Attachment #8649680 - Flags: review?(kinetik)
(Assignee)

Comment 16

4 years ago
This prevent entering into an unrecoverable error state when parsing incomplete data as often seen with MSE.
Attachment #8649681 - Flags: review?(kinetik)
(Assignee)

Comment 17

4 years ago
The webm demuxer will only handle data where frames's a monotonically increasing.
Attachment #8649682 - Flags: review?(gsquelart)
(Assignee)

Updated

4 years ago
See Also: 1192517
Summary: Ensure TrackBuffersManager only feed monotonic data to its demuxers → Summary: Stall in MSE when using webm
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1192517
Comment on attachment 8649682 [details] [diff] [review]
[MSE] P5. Detect out of order appends and recreate demuxer.

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

r+ with concern addressed, and comment nits.

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +704,5 @@
> +      // If so, recreate a new demuxer to ensure that the demuxer is only fed
> +      // monotonically increasing data.
> +      if (newData) {
> +        if (mNewSegmentStarted && mLastParsedEndTime.isSome() &&
> +            start < mLastParsedEndTime.ref().ToMicroseconds()) {

This seems to only test for going back in time. Could there be cases with jumps forward?
E.g. (start < mLast || start > mLast + MAX_GAP)

@@ +1126,5 @@
>      // The mediaRange is offset by the init segment position previously added.
>      uint32_t length =
>        mediaRange.mEnd - (mProcessedInput - mInputBuffer->Length());
> +    if (!length) {
> +      // We've completed our earlier media segment and no new data is to processed.

'is to' -> 'is to be' (or 'has been'?).

@@ +1127,5 @@
>      uint32_t length =
>        mediaRange.mEnd - (mProcessedInput - mInputBuffer->Length());
> +    if (!length) {
> +      // We've completed our earlier media segment and no new data is to processed.
> +      // This happens with webm container which can't detect that a media

Make this comment future-proof, e.g.: "This happens with containers (e.g., webm) that can't detect..."

@@ +1128,5 @@
>        mediaRange.mEnd - (mProcessedInput - mInputBuffer->Length());
> +    if (!length) {
> +      // We've completed our earlier media segment and no new data is to processed.
> +      // This happens with webm container which can't detect that a media
> +      // segment is ending until a new starts.

'a new one starts'
Attachment #8649682 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 20

4 years ago
(In reply to Gerald Squelart [:gerald] from comment #19)
> Comment on attachment 8649682 [details] [diff] [review]
> [MSE] P5. Detect out of order appends and recreate demuxer.
> 
> Review of attachment 8649682 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with concern addressed, and comment nits.
> 
> ::: dom/media/mediasource/TrackBuffersManager.cpp
> @@ +704,5 @@
> > +      // If so, recreate a new demuxer to ensure that the demuxer is only fed
> > +      // monotonically increasing data.
> > +      if (newData) {
> > +        if (mNewSegmentStarted && mLastParsedEndTime.isSome() &&
> > +            start < mLastParsedEndTime.ref().ToMicroseconds()) {
> 
> This seems to only test for going back in time. Could there be cases with
> jumps forward?
> E.g. (start < mLast || start > mLast + MAX_GAP)

Going back in time is the only thing we care about. A gap in the future still fulfill the requirement that timestamp are monotonically increasing. This test isn't about detecting any discontinuity in the stream ; it's just to make the demuxers happy.

Also, small gaps between media segment is happening rather often, it would cause additional unecessary delay due to extra initialization.

More accurate Discontinuity detection is done is in ProcessFrames.

> Make this comment future-proof, e.g.: "This happens with containers (e.g.,
> webm) that can't detect..."

the list of supported containers is fixed and limited. but okay.
(In reply to Jean-Yves Avenard [:jya] from comment #20)
> (In reply to Gerald Squelart [:gerald] from comment #19)
> > Comment on attachment 8649682 [details] [diff] [review]
> > [MSE] P5. Detect out of order appends and recreate demuxer.
> > 
> > Review of attachment 8649682 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r+ with concern addressed, and comment nits.
> > 
> > ::: dom/media/mediasource/TrackBuffersManager.cpp
> > @@ +704,5 @@
> > > +      // If so, recreate a new demuxer to ensure that the demuxer is only fed
> > > +      // monotonically increasing data.
> > > +      if (newData) {
> > > +        if (mNewSegmentStarted && mLastParsedEndTime.isSome() &&
> > > +            start < mLastParsedEndTime.ref().ToMicroseconds()) {
> > 
> > This seems to only test for going back in time. Could there be cases with
> > jumps forward?
> > E.g. (start < mLast || start > mLast + MAX_GAP)
> 
> Going back in time is the only thing we care about. A gap in the future
> still fulfill the requirement that timestamp are monotonically increasing.
> This test isn't about detecting any discontinuity in the stream ; it's just
> to make the demuxers happy. [...]

In this case, please rephrase the comment line just above ("Check if we have a discontinuity from the previous segment parsed.") to clarify this point.
Priority: -- → P1
Attachment #8649038 - Attachment is obsolete: true
Comment on attachment 8649677 [details] [diff] [review]
[MSE/webm] P1. Detect individual webm clusters.

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

r+ with nits

::: dom/media/mediasource/ContainerParser.cpp
@@ +188,5 @@
> +      if (mLastMapping) {
> +        // The last data contained a complete cluster but we can only detect it
> +        // now that a new one is starting.
> +        // We use mOffset as end position to ensure that any blocks not reported
> +        // by WebMBufferParser is properly skipped.

"are properly skipped"

@@ +273,5 @@
> +                mapping[completeIdx].mEndOffset - mOffset);
> +      completeIdx -= 1;
> +    }
> +
> +    // Save parsed blocks for which we do not have all datas yet.

"data"

@@ +289,5 @@
> +    }
> +    mLastMapping = Some(mapping[completeIdx]);
> +
> +    if (foundNewCluster && mOffset >= mapping[endIdx].mEndOffset) {
> +      // We now have all informations required to delimit a complete cluster.

"information"

@@ +321,5 @@
>  private:
>    WebMBufferedParser mParser;
>    nsTArray<WebMTimeDataOffset> mOverlappedMapping;
>    int64_t mOffset;
> +  // Last mapping seen.

Probably don't need this comment, it's almost identical to the member variable name.
Attachment #8649677 - Flags: review?(kinetik) → review+
Comment on attachment 8649679 [details] [diff] [review]
[webm] P2. Add WebMBufferedState::GetLastBlockOffset method.

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

::: dom/media/webm/WebMBufferedParser.h
@@ +268,5 @@
>    // on the main thread as data is received and parsed by WebMBufferedParsers.
>    nsTArray<WebMTimeDataOffset> mTimeMapping;
> +  // The last complete block parsed. -1 if not set.
> +  int64_t mLastBlockOffset;
> +  // The last seen data's end offset. -1 if not set.

"data"
Attachment #8649679 - Flags: review?(kinetik) → review+
Attachment #8649680 - Flags: review?(kinetik) → review+
Comment on attachment 8649681 [details] [diff] [review]
[MSE/webm] P4. Limit nestegg reads to the last block's boundaries.

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

This is a bit unfortunate, but libnestegg doesn't offer a nice API to deal with this use case.
Attachment #8649681 - Flags: review?(kinetik) → review+
(Assignee)

Updated

4 years ago
Blocks: 1197083
Backed out as https://hg.mozilla.org/integration/mozilla-inbound/rev/ea34d725c140 for M2 and W5 on OSX and Linux and R(R2, Ru2) bustage on Linux.
Backed out changeset aed2e9e74697 (bug 1195073) because it will now pass unexpected due to the previous backout. r=backout
Commit from previous backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad2ec509f370

Backed out changeset 6beb23f39237 (bug 1195073) because other parts of the bug have been backed out. r=backout
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bcc202f45e9
(Assignee)

Comment 31

4 years ago
The reftest that fails:
layout/reftests/webm-video/bug686957.html

parse the file layout/reftests/webm-video/frames.webm

The file has a length of 247057 bytes, but the WebMBufferedParser returns the last block offset to be at position 246716.

This causes the reads to be shorten to that last offset and we probably don't demux as much frames as before leading to the image comparison to fail.

kinetik, could you have a look at this file to see what's going on? I don't know enough about webm to understand what's going on.

Probably can't rely on the last mapping's offset found but instead have a new member in the ContainerParser with the last block's offset as I've seen in some cases where ContainerParser didn't report some blocks.
However here it causes some frames to be missed so something is not quite right, because those frames should always be available in a cluster somewhere.
Flags: needinfo?(kinetik)
(Assignee)

Comment 32

4 years ago
MSE may input partial media segment, which could cause the WebMDemuxer and libnestegg to error upon encountering an incomplete block which can't be recovered from.
this will allow to limit read to known complete blocks.

Changed to not rely on the mapping's end offset but instead the last block actually parsed.
Attachment #8650978 - Flags: review?(kinetik)
(Assignee)

Comment 33

4 years ago
ok.. after learning about webm:

so the element ends with a Cues that is 329 bytes long. This isn't reported by the WebMBufferedParser as it doesn't contain media. Why that caused a difference with playback I don't know. Especially as I see the name number of frames being drawn under both cases
Flags: needinfo?(kinetik)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1186261
(Assignee)

Comment 35

4 years ago
This allows to detect the end of a webm media segment without having to wait for the start of a new one.
Also record where an init segment (EBML) starts as this will be required by the WebM ContainerParser.
Attachment #8651501 - Flags: review?(kinetik)
(Assignee)

Comment 36

4 years ago
Most cluster contains information about their size. When known, we don't need to wait until the next media segment is received to determine its size.
Attachment #8651502 - Flags: review?(kinetik)
(Assignee)

Comment 37

4 years ago
Attachment #8651503 - Flags: review?(karlt)
(Assignee)

Comment 38

4 years ago
Remove unecessary files.
Attachment #8651504 - Flags: review?(karlt)
https://hg.mozilla.org/mozilla-central/rev/507a508aea70
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Attachment #8651503 - Flags: review?(karlt) → review+
Attachment #8651504 - Flags: review?(karlt) → review+
(Assignee)

Comment 40

4 years ago
Obviously the backout was incomplete (despite the three commits).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 41

4 years ago
Remove unecessary files.
Attachment #8651565 - Flags: review?(karlt)
(Assignee)

Comment 42

4 years ago
Comment on attachment 8651565 [details] [diff] [review]
[MSE] P9. Update webref meta files.

sorry, wrong git command.
Attachment #8651565 - Attachment is obsolete: true
Attachment #8651565 - Flags: review?(karlt)
Attachment #8650978 - Flags: review?(kinetik) → review+
Attachment #8651502 - Flags: review?(kinetik) → review+
Attachment #8651501 - Flags: review?(kinetik) → review+
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1188315

Updated

4 years ago
Depends on: 1199573
Backed out for a youtube playback regression. See Bug 1199573.
https://hg.mozilla.org/releases/mozilla-aurora/rev/5bb661db5c6c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Blocks: 1199879
You need to log in before you can comment on or make changes to this bug.