1226931 - Shaka Player warning: appendBuffer() should not modify the MediaSource's duration: before 59.994 after 60 delta 0.006000000000000227

Status: VERIFIED FIXED

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

Description

[Chris Peterson [:cpeterson]]

STR:
0. Enable webm MSE: media.mediasource.webm.enabled = true
1. Load http://shaka-player-demo.appspot.com/
2. Select Test manifest `"Angel One" (TNG clip) - multilingual, subtitles, VP8`
3. Press the "Load stream" button.
4. Watch the video.
5. When the end of the video is reached, look at the log messages in the console log.

RESULT:
The Shaka Player as logged warnings about appendBuffer() modify the MediaSource duration:

SBM video/webm: appendBuffer() should not modify the MediaSource's duration: before 59.994 after 60 delta 0.006000000000000227

SBM audio/webm: appendBuffer() should not modify the MediaSource's duration: before 60 after 60.016 delta 0.015999999999998238

The Shaka Player code that logs this warning has a large comment describing the apparent problem and asking whether this is a browser bug or spec compliant behavior:

  // Sanity check: appendBuffer() should not modify the MediaSource's duration
  // because an appropriate append window should have been set.
  //
  // On some browsers, even with an append window, inserting a segment that
  // ends past the end of the append window can increase the MediaSource's
  // duration (slightly). However, when this occurs it appears that no content
  // is actually buffered past the end of the append window, and subsequently
  // calling endOfStream() resets the MediaSource's duration to the correct
  // value (i.e., the end of the append window).
  //
  // TODO: Determine if this is a browser bug or is actually compliant with the
  // MSE spec.

https://github.com/google/shaka-player/blob/6fe239150a8939728876e93e1a4269032530d9f1/lib/media/source_buffer_manager.js#L281

Comment 1

[Jean-Yves Avenard [:jya]]

We have a fuzz of +/- half frame duration to the append window interval in order to allow frames that have a negative start time close to zero (in particular YouTube) to not be evicted.

If we were to be exact per spec and evict this first frame slightly outside the append window (default is [0, +infinity) all frames from the first frame to the next keyframe would be evicted; and it would cause a gap of 2s at the start (if they keyframes are every 2s)

I guess we could only add that fuzz if the append window start is the default of 0 to cater only for this particular (valid) case.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] Only apply fuzz on append window should it not be set.

Additionally, only apply the fuzz on the interval' start.

Comment 3

[Jean-Yves Avenard [:jya]]

this is also what causes some parts of bug 1222851, which I described in bug 1222851 comment 1

Comment 4

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

Comment on attachment 8690515 [details] [diff] [review]
[MSE] Only apply fuzz on append window should it not be set.

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

In the check-in comment:
- Do you mean "Only apply fuzz to append window if it is not set"? (The precautionary "should" does not make sense there.)
- What does 'it' refer to, the fuzz or the append window? Please clarify.
- There's a stray apostrophe after "interval".

Finally I don't understand the connection between the check-in comment (however I can interpret it) and your patch.
I'm afraid I cannot complete this review -- Please change patch and/or reviewer!

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +1387,5 @@
>    // 5. Let track buffer equal the track buffer that the coded frame will be added to.
>    auto& trackBuffer = aTrackData;
>  
>    // We apply a fuzz search +- mLongestFrameDuration to get around videos where
>    // the start time is negative but close to 0.

Please update the comment to explain why the fuzz is only applied when the append window starts at 0? (How does that relate to your "should it not be set"?)

Comment 5

[Jean-Yves Avenard [:jya]]

Not sure what's hard to understand 

If append window has default value (0 per spec): then use fuzz. If not use append window as is. 

The definition of the append window and what it does  is in the MSE spec.

Also we were using the fuzz for the append window end. Now it's only done on its start

Comment 6

[Jean-Yves Avenard [:jya]]

Also, the problem is fully explained in the bug description and the comment in the shakka player (link is in the bug description)

Comment 7

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

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> Not sure what's hard to understand 
> 
> If append window has default value (0 per spec):
> then use fuzz. If not use append window as is. 
> 
> The definition of the append window and what it does is in the MSE spec.

The specs specify the default append window value as [0,+inf), i.e. a *pair* of values.
So I do not understand "If append window has default value (0 per spec)", which talks about a single value.

Also your check-in comment says (I think) "... if [append window] is not set", but I do not understand how you can know that the append window has been set by just comparing the start value with 0. What if the js player has manually set the start value to 0? Or has only set the end value?

The MSE specs don't mention fuzz, which makes it harder to understand.

> Also we were using the fuzz for the append window end. Now it's only done on
> its start.

I'm happy with not having a fuzz for the append window end, it seems like it's the main issue from comment #0.

I'm guessing we still need the non-spec'd fuzz at the start because of Youtube with its negative start times, right?


I'm just uncomfortable r+'ing the 'start != 0' part, because it is not documented in the code, and because to me it does not match what the check-in comment says -- even if that code may be the exact fix that's needed.

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] Restrict leeway to append window start.

We only allow a leeway if the append window start is set to its default value (0)

Comment 9

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

Comment on attachment 8690575 [details] [diff] [review]
[MSE] Restrict leeway to append window start.

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

Thank you for the more informative comment.

Comment 10

[Pulsebot]

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d61fe2e3bf05

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8690575 [details] [diff] [review]
[MSE] Restrict leeway to append window start.

Approval Request Comment
[Feature/regressing bug #]: 1226931
[User impact if declined]: Invalid data may be appended; not per W3C spec
[Describe test coverage new/current, TreeHerder]: In central, lots of mochitest
[Risks and why]: Low. We had added a very large workaround for some invalid videos that were not playing. We've narrowed that change in a way that make us more spec compliant
[String/UUID change made/needed]: none

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Where is this player used? is this crucial to have on beta? It is a bit late to uplift (beta 7). Please let me know if it's something specifically aimed at 43. 

We could probably take it on 44 aurora though.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8690575 [details] [diff] [review]
[MSE] Restrict leeway to append window start.

Getting late in the beta cycle to take new fixes that aren't critical for new features or stability.

Comment 15

[Jean-Yves Avenard [:jya]]

I had replied to this on IRC since you pinged me, but it seems that you never got it.

42 made MSE available on all platforms. I see 43 as making MSE even more robust and spec compliant.
Which is what this bug does.

This is one area where we weren't compliant with the spec.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8690575 [details] [diff] [review]
[MSE] Restrict leeway to append window start.

This has been on Nightly for a week, let's uplift to Aurora44.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Chris, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 18

[Chris Peterson [:cpeterson]]

Verified fixed in Nightly 45.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dda1900a7523

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/dda1900a7523