1780150 - Twitter video clock flashes "-1:0-1:0-1" before looping

Status: RESOLVED FIXED

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

Description

[Chris Peterson [:cpeterson]]

Attachment: screenshot.png

@ Paul, this bug is a regression in 102 from muted audio stream bug 1743834. I bisected this regression to this pushlog:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6323b526b0ac784521845c58f4035da85572c4be&tochange=3774e6ab386979c9553482e1bd2ed5d0a2eee075

Steps to reproduce

1. Open https://twitter.com/grumpygamer/status/1549048421708836864
2. Scroll up to the first tweet. (I linked to a reply tweet because directly linking to the first tweet sometimes causes the video to play without showing a clock.)
3. Play the video.
4. Watch the video loop.

Expected result

The video's clock should seamlessly roll over from 0:00 to 0:09.

Actual result

Before rolling over to 0:09, the clock flashes -1:0-1:0-1. See the attached screenshot.

Comment 1

[Chris Peterson [:cpeterson]]

I can reproduce this bug on both Windows and macOS.

Comment 2

[Alastor Wu [:alwu]]

By doing a quick check, from the point of the MDSM, I didn't see any problem yet. When the video reaches to the end, its current time and the media time in MDSM are both correct. That video would be seeked from zero as well, not from a negative timestamp. If you hover cursor over the video, Twitter would show another video control interface, which time is correct when looping back to the head.

Maybe we incorrectly dispatch some events or change media element's attribute causing Twitter think the video's duration in unknown?

Comment 3

[Paul Adenot (:padenot)]

I've observed that the currentTime can be greater than the duration on this page, by doing this in the console:

setInterval(function() { if ($$("video")[0].currentTime > $$("video")[0].duration) { console.log($$("video")[0].currentTime)}})

it has printed: 9.673986, the duration being 9.636281.

I'll get us a fix.

Comment 4

[Paul Adenot (:padenot)]

Attachment: Bug 1780150 - Cap currentTime to the end time of the last packet when a video with an audio track is muted. r?alwu

Comment 5

[Paul Adenot (:padenot)]

Attachment: Bug 1780150 - Test that a muted media element with an audio track never has currentTime exceeding its duration. r?alwu

Depends on D152405

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1743834

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c2c6af6a7cf
Cap currentTime to the end time of the last packet when a video with an audio track is muted. r=alwu
https://hg.mozilla.org/integration/autoland/rev/d060eced6ecf
Test that a muted media element with an audio track never has currentTime exceeding its duration. r=alwu

Comment 8

[Cristian Tuns]

Backed out for causing build bustages on AudioSinkWrapper.h

• Backout link
• Push with failures
• Failure Log
• Failure line: /builds/worker/checkouts/gecko/dom/media/mediasink/AudioSinkWrapper.h:144:9: error: unknown type name 'TimeUnit'; did you mean 'media::TimeUnit'?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d85601edc0c4
Cap currentTime to the end time of the last packet when a video with an audio track is muted. r=alwu
https://hg.mozilla.org/integration/autoland/rev/c09aaf7f5146
Test that a muted media element with an audio track never has currentTime exceeding its duration. r=alwu

Comment 10

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/d85601edc0c4
https://hg.mozilla.org/mozilla-central/rev/c09aaf7f5146

Comment 11

[Alin Ilea, Desktop Test Engineering]

Reproduced the issue in Nightly 105.0a1 (build id: 20220727214405)
Verified - Fixed in Beta 105.0b4 and the latest Nightly 106.0a1 (2022-08-30) using Windows 10 and macOS 12.

Comment 12

[Paul Adenot (:padenot)]

We're backing this out in bug 1789103, and we'll reland a different fix.

Comment 13

[Paul Adenot (:padenot)]

Reopening, this has been backed out.

Comment 14

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/mozilla-central/rev/ecdb4d9d26dc might plausibly address this but I wasn't able to reproduce as originally reported, so cannot check.

Comment 15

[Paul Adenot (:padenot)]

I wasn't able to repro either, and I'm absolutely sure I could reproduce 100% of the time before. But considering my fixes about time and duration reporting all over the place in the recent week, Alastor fixes about seamless looping and audio, and your fixes from yesterday, it would be fairly plausible that this cannot happen anymore. All of those are somewhat related, but it's hard to know exactly which one fixed this particular issue (and all patches were needed on their own anyways), see below for the explanation why.

setInterval(function() { if ($$("video")[0].currentTime > $$("video")[0].duration) { console.log($$("video")[0].currentTime)}})

is what I used to characterize the problem, running in the console, previously. Over a couple loops of the video, this would emit a log statement.

But Twitter has seemingly broken their mp4 muxer and the video sometimes has a duration of 1:36 instead of 0:09. Depending on the resolution (apparently) they have incorrect value for BaseMediaDecodeTime in traf boxes for the audio track. But I've tried a number of loops with a current nightly and was lucky to get the non-broken video, and it didn't seem to reproduce. This broken duration happens in the nightly used to report this bug, and also in current Chrome release version.

I'm closing this.

Comment 16

[Paul Adenot (:padenot)]

Attachment: twitter video, with broken BaseMediaDecodeTime in traf boxes

Here's the broken video for posterity, I used https://gpac.github.io/mp4box.js/test/filereader.html to understand the structure easily and compare to what our demuxer is doing. I was afraid that I had broken something subtly because of all my changes in the mp4 demuxer recently.