Open Bug 1543059 Opened 6 years ago Updated 1 year ago

Loading spinner displays at end of some non-looping videos hosted on reddit.com

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
57 Branch
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox-esr60 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix

People

(Reporter: ke5trel, Unassigned)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, Whiteboard: [webcompat])

Attachments

(2 files, 1 obsolete file)

Orange loading spinner at end of video
210.62 KB, video/webm
Details
Testcase.zip
2.31 KB, application/x-zip-compressed
Details
Rank: 15
Priority: -- → P2

Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.

Webcompat Priority: --- → ?

See bug 1547409. Migrating whiteboard priority tags to program flags.

This has been broken for a while (since 57) and it's a fairly common action on a popular site. Any chance we can try to fix this for 69 or 70?

Flags: needinfo?(drno)

This is something I've been interested in looking into but have not found time to do so. Not sure if I'll have time in the immediate future, but throwing NI on myself as a reminder.

Flags: needinfo?(bvandyk)

Looking at the videos we get the spinner on by adding listeners:

$('video').addEventListener('waiting', e => {console.log("waiting!");}); $('video').addEventListener('seeking', e => {console.log("seeking!");});

We're firing waiting at the end of the video which is triggering the spinner. You can see this on videos that don't trigger the spinner also if you force dispatch of the waiting event:

$('video').dispatchEvent(new Event('waiting'));

Will look into why we're doing this with specific videos. Holding NI for now.

Assignee: nobody → bvandyk
Webcompat Priority: ? → ---

It looks like this is happening during changing the ready state and not detecting the media as ended[0]. This happens because we have mDecoder and that decoder is not reporting that it's ended[1, 2].

It looks like the decoder moves to the ended state following us firing the notification rather than before, which would avoid this bug.

Regression range (edit): derp, missed it in comment 0 after coming back to this.

[0] https://searchfox.org/mozilla-central/rev/0c682c4f01442c3de0fa6cd286e9cadc8276b45f/dom/html/HTMLMediaElement.cpp#6074
[1] https://searchfox.org/mozilla-central/rev/0c682c4f01442c3de0fa6cd286e9cadc8276b45f/dom/html/HTMLMediaElement.cpp#2184
[2] https://searchfox.org/mozilla-central/rev/0c682c4f01442c3de0fa6cd286e9cadc8276b45f/dom/media/MediaDecoder.cpp#797

https://searchfox.org/mozilla-central/rev/0c682c4f01442c3de0fa6cd286e9cadc8276b45f/dom/media/MediaDecoderStateMachine.cpp#2032-2034 appears to be the regressing code.

It looks like the MDSM reliably notifies playback has ended[0] prior to us firing the waiting. The issue is that we always fire waiting before the MediaDecoder receives that event[1] and is shutdown. MediaElement is updating its ready state in between these happening, at which point it queries the decoder which says it has not yet ended, and doesn't have the next frame, so we set our state to have current data[2], triggering the code mentioned in comment 6. It seems likely that his is a result of the code above which intentionally triggers the ready state update prior to ending.

https://bugzilla.mozilla.org/show_bug.cgi?id=1390443#c0 discusses the need to do the ended + no more frames event in one. I'm not sure why this wasn't done. It seems like what is needed here. I'm going to gaze at the spec and some of the historical bugs to get a better idea on what our limitations are and make sure I don't retread any prior work.

[0] https://searchfox.org/mozilla-central/rev/0c682c4f01442c3de0fa6cd286e9cadc8276b45f/dom/media/MediaDecoderStateMachine.cpp#2036
[1] https://searchfox.org/mozilla-central/rev/84922363f4014eae684aabc4f1d06380066494c5/dom/media/MediaDecoder.cpp#412
[2] https://searchfox.org/mozilla-central/rev/84922363f4014eae684aabc4f1d06380066494c5/dom/html/HTMLMediaElement.cpp#5996

I've investigated why this only happens with some videos:

  • We always trigger the update ready state before ending, but we only fire waiting for certain videos like the ones linked here. This is because of logic in HTMLMediaElement::UpdateReadyStateInternal[0].
    • I've checked a few different cases and we normally avoid the issue by transitioning to the same state, which avoids firing the waiting[1]. It looks like in this case we stay in HAVE_ENOUGH_DATA.
    • We can end up in the same state because we think we still have the next frame in our buffered ranges[2] and then we hit a HAVE_ENOUGH_DATA case. This seems to avoid the issue on many sites using MSE.
    • In other cases we hit the media cache case so report HAVE_ENOUGH_DATA[3]. This happens in my testing when playing non-MSE files.
    • So the cases we're hitting here are MSE and nextFrameStatus = mDecoder->NextFrameBufferedStatus();[2] is saying we don't have a next frame.

It seems like there are a few things happening here and some of them are not entirely clear to me. The MSE next frame logic reporting that we have next frames on videos where we've reached the end is odd. However, I think there's an underlying issue that can be fixed without yak shaving that. Since playback ending implies we don't have a next frame we can combine that logic on the decoder when we deal with playback ended, rather than doing it in 2 steps via notifying unavailable next frame then playback ended.

[0] https://searchfox.org/mozilla-central/rev/84922363f4014eae684aabc4f1d06380066494c5/dom/html/HTMLMediaElement.cpp#5832
[1] https://searchfox.org/mozilla-central/rev/84922363f4014eae684aabc4f1d06380066494c5/dom/html/HTMLMediaElement.cpp#6034
[2] https://searchfox.org/mozilla-central/rev/84922363f4014eae684aabc4f1d06380066494c5/dom/html/HTMLMediaElement.cpp#5903
[3] https://searchfox.org/mozilla-central/rev/84922363f4014eae684aabc4f1d06380066494c5/dom/html/HTMLMediaElement.cpp#5903

Looking at the videos on reddit where the issue takes place compared to other videos we end up with a currentTime past the end of our buffered ranges. E.g. for https://www.reddit.com/r/Awww/comments/a8praa/when_the_laptop_closes_to_bed_i_goeses/

$('video').buffered.end(0)
23.289625
$('video').currentTime
23.295999

I figure this is the difference between these videos and those that won't fire an additional waiting as we end. In this case our currentTime and the currentPosition used internally[0] to determine if we have a next frame are outside the buffered range, so we end up thinking we're waiting on data before we end.

:jya, do you have any thoughts about what MSE is doing here and if there's a preferred way to address this? My thought at this stage is to not handle the next frame unavailable and ended separately. I.e. remove https://searchfox.org/mozilla-central/rev/0c682c4f01442c3de0fa6cd286e9cadc8276b45f/dom/media/MediaDecoderStateMachine.cpp#2032-2034 and build similar logic into handling the playback ended event.

[0] https://searchfox.org/mozilla-central/rev/b2716c233e9b4398fc5923cbe150e7f83c7c6c5b/dom/media/MediaDecoder.cpp#1333

Flags: needinfo?(bvandyk) → needinfo?(jyavenard)

buffered range is based on what is found in the container (as per spec).
However, currentTime is based on the decoded value and this will depend on the platform you're on as not all decoders behave the same.

If the container information is wrong, then it could happen. However, I doubt this is what is causing the problem.

Having said that an event such as "waiting" isn't based on the container's information, and there's a 500ms leeway for discrepancy, it only occurs when we changed the readyState..

I don't fully recall, however I believe the change you're proposing would also break the spec, if you are to move readyState to HAVE_CURRENT_DATA, from HAVE_FUTURE_DATA , then waiting should be fired.

With MSE, you also don't fire the ended event and must fire the waiting event until the MediaSource element itself is ended via a call to endOfStream(). So firing waiting here is also the proper thing.

I'm also concerned of regression with YouTube with the change you are proposing, as this is why that code was added in the first place.

I believe the right approach would be for Reddit to stop their spinner once they get the ended event. That is the right answer to this problem
I don't believe there's anything broken on our end, it's all per spec.

Flags: needinfo?(jyavenard)

My reading of the HTML spec is we don't have to fire the waiting for a readystate change if the element has ended.

https://html.spec.whatwg.org/multipage/media.html#dom-media-readystate-dev

If the media element was potentially playing before its readyState attribute changed to a value lower than HAVE_FUTURE_DATA, and the element has not ended playback, and playback has not stopped due to errors, paused for user interaction, or paused for in-band content, the user agent must queue a media element task given the media element to fire an event named timeupdate at the element, and queue a media element task given the media element to fire an event named waiting at the element.

We already don't do this in most cases. My reading of the spec is we always need to transition to HAVE_CURRENT_DATA as we end.

HAVE_CURRENT_DATA (numeric value 2)
Data for the immediate current playback position is available, but either not enough data is available that the user agent could successfully advance the current playback position in the direction of playback at all without immediately reverting to the HAVE_METADATA state, or there is no more data to obtain in the direction of playback. For example, in video this corresponds to the user agent having data from the current frame, but not the next frame, when the current playback position is at the end of the current frame; and to when playback has ended.

and

HAVE_FUTURE_DATA (numeric value 3)
Data for the immediate current playback position is available, as well as enough data for the user agent to advance the current playback position in the direction of playback at least a little without immediately reverting to the HAVE_METADATA state, and the text tracks are ready. For example, in video this corresponds to the user agent having data for at least the current frame and the next frame when the current playback position is at the instant in time between the two frames, or to the user agent having the video data for the current frame and audio data to keep playing at least a little when the current playback position is in the middle of a frame. The user agent cannot be in this state if playback has ended, as the current playback position can never advance in this case.

I take the above to mean we always need to transition state to HAVE_CURRENT_DATA as we end, and if we always fire waiting on state transition we would do it every time we end. However we do not currently.

I'm not sure how to read the MSE spec here. The endOfStream algo specifically has this

Notify the media element that it now has all of the media data.

What does this mean? Much of the other readyState behaviour seems better specified. Do we take this to mean that once endOfStream has been called and we reach the end of our duration that we can consider playback ended? Otherwise we assume more data is coming and cannot consider the element ended?

It looks like our code operates consistently with the above. Tracing from our endOfStream code[0] we propagate the end state such that it looks like our demuxer will report end of stream. This means that we should only hit the ended case in the MDSM we're looking at here once a the MediaSource is in an ended state.

This code was added with a number of other changes to fix the original YouTube bug, it's not clear to me why this was needed. https://bugzilla.mozilla.org/show_bug.cgi?id=1390443#c0 suggests the correct fix would be to combine the tasks, but then bug 1405025 just restores the yanked code without changing it. P1 in that bug seems to be the important part that alters how 'seeking' and 'waiting' are being fired. It's not clear to me how P2 was needed there. P2's commit message even references the comment suggesting the correct approach would be to combine the ready state and ended handling, though it doesn't do so in the patch.

[0] https://searchfox.org/mozilla-central/rev/b2716c233e9b4398fc5923cbe150e7f83c7c6c5b/dom/media/mediasource/MediaSource.cpp#345

I'm tracing further to try and create a minimal testcase. At least for https://www.reddit.com/r/Awww/comments/a8praa/when_the_laptop_closes_to_bed_i_goeses/ it's the audio causing the bug. If I create my own MSE test case I don't repro with the video on it's own, but if I have a source buffer with the audio on its own, or with the video I can repro locally.

Attached file TestCase.zip (obsolete) —

Adding a test case produced from our test files.

It looks like the problem with the reddit audio (for the first linked video) is an incorrect sample duration on their last audio sample. I'm fairly sure it should be 1024 because of being AAC, but they specify 718.

Attached file Testcase.zip —

Adding updated testcase that also uses omit_tfhd_offset flag so that the created file plays nice with Chrome (easier parity comparison).

STR:

  • Download testcase and extract.
  • Serve locally via python http server or similar.
  • Open page and play file.
  • Observe waiting notifications in console -- we fire a waiting as we end.

Revisiting this.

There are two (potential) problems here, I think fixing either is enough to address this.

  1. As detailed above, we change the next frame available state before the ended state. This causes two separate update events, and the issue is the first fire waiting and the second fires ended. I remain unconvinced we need to do these things separately, but appreciate there's a lot of history and machinery here which makes me hesitant to mess with it.
  2. MediaSource bases its buffered ranges off container information, so that if the container is wrong we can end up with samples that have intervals outside of the buffered range. E.g. the container says the media has a range of [0ms, 1000ms], and we then get a sample at [1100ms, 1120ms]. This confuses our checks[0][1] that prevent waiting being fired when we have correct container information.

My previous efforts have been around addressing the first case, but I'm going to take a look at if we can do anything on the MSE side.

[0] https://searchfox.org/mozilla-central/rev/9ae77e4ce3378bd683ac9a86b729ea6b6bd22cb8/dom/html/HTMLMediaElement.cpp#5693
[1] https://searchfox.org/mozilla-central/rev/9ae77e4ce3378bd683ac9a86b729ea6b6bd22cb8/dom/media/mediasource/MediaSourceDecoder.cpp#243

Has Regression Range: --- → yes
Depends on: media-reddit

Declaring NI bankruptcy.

Flags: needinfo?(drno)

Jim, this has been P2 for three years, should it be retriaged? I can still reproduce on Reddit's old design (old.reddit.com) but not www.reddit.com (On linux, don't know about other platforms)

Flags: needinfo?(jmathies)
Assignee: brycebugemail → nobody
Severity: normal → S4
Flags: needinfo?(jmathies)
Priority: P2 → P3
Blocks: media-reddit
No longer depends on: media-reddit

Is this an issue expected to be only present to be on Reddit as that may be the only mainstream media server that doesn't sanitizes the content, or are the other sites just not as annoying when it comes to this problem?

Yet another year later the issue is still present on Reddit as described by others, but I thought of this issue after seeing the spinner at the end of a YouTube video.
Now while I can't reproduce exactly that, I checked out multiple relatively short (<2 min) videos so the details aren't lost on the progress bar, and surprisingly I noticed that it doesn't actually get filled up, it just merely gets updated to be full once there's a forced update like changing to full screen.
While this itself could be an isolated issue with YouTube, due to the spinner at the end issue I started suspecting that it may be the same issue, maybe just not noticed as often because:

  • YouTube seems to work around the issue mostly, considering the video to be stopped even if the progress bar isn't at the end which likely masks the problem most of the time
  • As YouTube tends to have bloated videos with absolutely unnecessary outros, they are likely watched less often to the end
  • The silly overlay YouTube shows on the top of the videos near the end obstructs the spinner. I remember seeing a spinner earlier under the overlay, and now I likely only paid mind to it as I'm using debloating extensions which let me see the end of the videos
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: