1195073 - Summary: Stall in MSE when using webm

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

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)

Comment 1

[Jean-Yves Avenard [:jya]]

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)

Comment 2

[Jean-Yves Avenard [:jya]]

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

Comment 3

[Jean-Yves Avenard [:jya]]

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)

Comment 4

[Jean-Yves Avenard [:jya]]

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.

Comment 5

[Jean-Yves Avenard [:jya]]

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.

Comment 6

[Jean-Yves Avenard [:jya]]

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 }}

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: [webm] Do not always rely on next sample's start time to determine sample's end time.

Update comment to describe more accurately what's being done.

Comment 8

[Matthew Gregan [: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?

Comment 9

[Jean-Yves Avenard [:jya]]

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.

Comment 10

[Jean-Yves Avenard [:jya]]

The core reason for the stall is bug 1192517

Comment 11

[Jean-Yves Avenard [:jya]]

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.

Comment 12

[Jean-Yves Avenard [:jya]]

Actually no.

Let's make this bug purely about the TrackBuffersManager and creating new demuxers when a discontinuity in the data is identified.

Comment 13

[Jean-Yves Avenard [:jya]]

Attachment: [MSE/webm] P1. Detect individual webm clusters.

WebMContainerParser was incorrectly reporting webm blocks rather than clusters, causing the webm demuxer to later fail to parse the remaining data.

Comment 14

[Jean-Yves Avenard [:jya]]

Attachment: [webm] P2. Add WebMBufferedState::GetLastBlockOffset method.

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.

Comment 15

[Jean-Yves Avenard [:jya]]

Attachment: [webm] P3. Initialize members.

Comment 16

[Jean-Yves Avenard [:jya]]

Attachment: [MSE/webm] P4. Limit nestegg reads to the last block's boundaries.

This prevent entering into an unrecoverable error state when parsing incomplete data as often seen with MSE.

Comment 17

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P5. Detect out of order appends and recreate demuxer.

The webm demuxer will only handle data where frames's a monotonically increasing.

Comment 19

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

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'

Comment 20

[Jean-Yves Avenard [:jya]]

(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.

Comment 21

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

(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.

Comment 22

[Matthew Gregan [:kinetik]]

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.

Comment 23

[Matthew Gregan [:kinetik]]

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"

Comment 24

[Matthew Gregan [:kinetik]]

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.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5afa29ea85a
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e983ccb61cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f6fc1a91d07
https://hg.mozilla.org/integration/mozilla-inbound/rev/16669eed518d
https://hg.mozilla.org/integration/mozilla-inbound/rev/507a508aea70

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6beb23f39237

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aed2e9e74697

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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.

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out changeset aed2e9e74697 (bug 1195073) because it will now pass unexpected due to the previous backout. r=backout

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or 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

Comment 31

[Jean-Yves Avenard [:jya]]

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.

Comment 32

[Jean-Yves Avenard [:jya]]

Attachment: [webm] P2. Add WebMBufferedState::GetLastBlockOffset method. v2

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.

Comment 33

[Jean-Yves Avenard [:jya]]

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

Comment 35

[Jean-Yves Avenard [:jya]]

Attachment: [webm] P6. Calculate cluster's end offset if its size is known in advance.

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.

Comment 36

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P7. Determine a WebM Media Segment end as early as available information permits.

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.

Comment 37

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P8. Update webref results.

Comment 38

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P9. Update webref meta files.

Remove unecessary files.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/507a508aea70

Comment 40

[Jean-Yves Avenard [:jya]]

Obviously the backout was incomplete (despite the three commits).

Comment 41

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P9. Update webref meta files.

Remove unecessary files.

Comment 42

[Jean-Yves Avenard [:jya]]

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

sorry, wrong git command.

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e8526f57706
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae8e5d0b0455
https://hg.mozilla.org/integration/mozilla-inbound/rev/487ede19c072
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e7405c70314
https://hg.mozilla.org/integration/mozilla-inbound/rev/90fac53dc387
https://hg.mozilla.org/integration/mozilla-inbound/rev/69619b7f188e
https://hg.mozilla.org/integration/mozilla-inbound/rev/76f12bc3d57a
https://hg.mozilla.org/integration/mozilla-inbound/rev/85295a300ea2
https://hg.mozilla.org/integration/mozilla-inbound/rev/5698786babe8

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3e8526f57706
https://hg.mozilla.org/mozilla-central/rev/ae8e5d0b0455
https://hg.mozilla.org/mozilla-central/rev/487ede19c072
https://hg.mozilla.org/mozilla-central/rev/1e7405c70314
https://hg.mozilla.org/mozilla-central/rev/90fac53dc387
https://hg.mozilla.org/mozilla-central/rev/69619b7f188e
https://hg.mozilla.org/mozilla-central/rev/76f12bc3d57a
https://hg.mozilla.org/mozilla-central/rev/85295a300ea2
https://hg.mozilla.org/mozilla-central/rev/5698786babe8

Comment 46

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/73daa8e4568d
https://hg.mozilla.org/releases/mozilla-aurora/rev/d2ed0c5abd35
https://hg.mozilla.org/releases/mozilla-aurora/rev/efb94d2f8b68
https://hg.mozilla.org/releases/mozilla-aurora/rev/23b5669479f8
https://hg.mozilla.org/releases/mozilla-aurora/rev/e24897436eaa
https://hg.mozilla.org/releases/mozilla-aurora/rev/cbbb8dbd2745
https://hg.mozilla.org/releases/mozilla-aurora/rev/1fb0a72a7fca
https://hg.mozilla.org/releases/mozilla-aurora/rev/4c993f78e9f0

Comment 47

[Ralph Giles (:rillian)]

Backed out for a youtube playback regression. See Bug 1199573.
https://hg.mozilla.org/releases/mozilla-aurora/rev/5bb661db5c6c

Comment 48

[Jean-Yves Avenard [:jya]]

pushed it as part of 
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=7437f28133fc