Closed Bug 1226931 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla45
Tracking Status
firefox42 --- affected
firefox43 --- affected
firefox44 --- fixed
firefox45 --- verified
b2g-v2.5 --- fixed

People

(Reporter: cpeterson, Assigned: jya)

References

Details

Attachments

(1 file, 1 obsolete file)

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
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.
Additionally, only apply the fuzz on the interval' start.
Attachment #8690515 - Flags: review?(gsquelart)
Assignee: nobody → jyavenard
this is also what causes some parts of bug 1222851, which I described in bug 1222851 comment 1
Blocks: 1222851
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"?)
Attachment #8690515 - Flags: review?(gsquelart)
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
Also, the problem is fully explained in the bug description and the comment in the shakka player (link is in the bug description)
(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.
We only allow a leeway if the append window start is set to its default value (0)
Attachment #8690575 - Flags: review?(gsquelart)
Attachment #8690515 - Attachment is obsolete: true
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.
Attachment #8690575 - Flags: review?(gsquelart) → review+
https://hg.mozilla.org/mozilla-central/rev/d61fe2e3bf05
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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
Attachment #8690575 - Flags: approval-mozilla-beta?
Attachment #8690575 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(jyavenard)
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.
Attachment #8690575 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
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.
Flags: needinfo?(jyavenard)
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.
Attachment #8690575 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Chris, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(cpeterson)
Verified fixed in Nightly 45.
Status: RESOLVED → VERIFIED
Flags: needinfo?(cpeterson)
You need to log in before you can comment on or make changes to this bug.