Video and audio lose sync on XP when we can't present frames quickly enough

RESOLVED FIXED in Firefox 50

Status

()

defect
P3
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: jrmuizel, Assigned: cpearce)

Tracking

(Depends on 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, firefox50 fixed, firefox51 fixed)

Details

Attachments

(1 attachment)

Basic compositor is currently very slow and we can get far behind the audio.
I saw this when playing fullscreen video (h264) on youtube on XP with basic compositor
Component: Audio/Video → Audio/Video: Playback
Priority: -- → P2
i'm fairly certain we have another bug for this where A/V sync gets terrible when decoding/rendering video is way too slow
(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> I saw this when playing fullscreen video (h264) on youtube on XP with basic
> compositor

Since basic compositor currently mean also sw decoding (bug 1254874), it's possible that the compositor and decoder end up competing for cpu resources, which might make the decoder slower than it should.

If that is indeed the case, maybe we should make sure that the decoder gets as many resources as we can give it, and the compositor should come much further down the priorities.
Mass change P2 -> P3
Priority: P2 → P3
Assignee: nobody → cpearce
I can repro the A/V sync issues on my Win10 desktops when I play a 4K video of DOOM4 [1] at 2X playbackRate using WebM/VP9 and with HW acceleration disabled. All the gunshots get out of sync.

The issue is not in the compositor. I added logging in ImageHost::ChooseImageIndex(), and that shows that the compositor is ending up receiving only late frames, and it's painting the frame with the greatest timestamp that it gets. Since that frame is already very late, we see A/V sync issues.

Extra logging in VideoSink::RenderVideoFrames() shows that we're calling VideoSink::RenderVideoFrames() when the video queue only has 1 frame in it, and the frame is late.

The caller is VideoSink::UpdateRenderedVideoFrames(). It has a loop which pops late frames. However, if this function runs while there's only one frame in the video queue, we won't drop that one frame if it's late. If we're struggling to keep up, it's increasingly likely that we'll end up running this function with only one frame in the video queue. That's how we're getting into VideoSink::RenderVideoFrames() with only 1 late frame; we're not dropping it, even though it's late, because it's the only frame we have when VideoSink::UpdateRenderedVideoFrames() runs.

If I change VideoSink::UpdateRenderedVideoFrames() to drop when there's only 1 frame in the video queue, we'll not render late frames, and when we do manage to render a frame, it'll be in sync with the audio. However, if I do this, we end up dropping a lot more frames, and thus rendering a lot fewer. However, this allows us to *report* that we dropped the frames, so a well written MSE player can detect that we've failing miserably to keep up, and and lower their bitrate.

Amazon reported that we're not reporting dropped frames when A/V sync is bad, so I think we should drop the frames here and report it, so that players can adapt. I think it's better to report we're failing, rather than have broken A/V sync.


[1] https://www.youtube.com/watch?v=737Gl1sm0rE
Oh, so that explains why our A/V sync is busted when the decode can't keep up. But it doesn't explain why the decode doesn't keep up, or whether we can do anything to help it keep up.

FWIW, my CPU fans were going crazy when I repro'd the A/V sync issue, so I suspect we're hitting the limits of the hardware here.
(In reply to Chris Pearce (:cpearce) from comment #6)
> If I change VideoSink::UpdateRenderedVideoFrames() to drop when there's only
> 1 frame in the video queue, we'll not render late frames, 

Once upon a time, we have a B2G platform where no duration info is attached to the video frames. So there is no way to tell if this frame is obsolete or not until next frame is present. For ex:

current time = 100s
video frame time = 90s

It would be appropriate to render this frame if next frame time turns out to be 120s.
That's really interesting Chris, thanks for the full write up.

We improved 4k performance with WMF/h264 considerably in bug 1138967, I suspect we could get similar improvements with WebM/VP9. Full frame copies of 4k video really tax the memory bus, removing unnecessary ones makes a huge difference.
(In reply to JW Wang [:jwwang] from comment #8)
> Once upon a time, we have a B2G platform where no duration info is attached
> to the video frames. So there is no way to tell if this frame is obsolete or
> not until next frame is present.

I'd assume this isn't an issue when we're using the MediaFormatReader on B2G. The other B2G (non-MediaFormatReader) readers can now be removed.
Comment on attachment 8781345 [details]
Bug 1258870 - Don't push late video frames to the compositor, drop them.

https://reviewboard.mozilla.org/r/71758/#review69310
Attachment #8781345 - Flags: review?(jwwang) → review+
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4f39f2e1324b
Don't push late video frames to the compositor, drop them. r=jwwang
https://hg.mozilla.org/mozilla-central/rev/4f39f2e1324b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Duplicate of this bug: 1279624
Comment on attachment 8781345 [details]
Bug 1258870 - Don't push late video frames to the compositor, drop them.

Approval Request Comment
[Feature/regressing bug #]: Amazon Prime Video. This is a fix for a long-standing A/V sync issue which we've only just figured out.

[User impact if declined]: Amazon Prime Video users on slower machines will experience broken audio-video sync, and Amazon's player won't be able to detect this condition and adapt to a lower bitrate that's more likely to be able to be decoded fast enough to keep A/V in sync.

[Describe test coverage new/current, TreeHerder]: None. It's hard to test this via automation (though this patch was noticed by a talos test that relied on the previous broken behaviour starting to fail in Bug 1295630, so we'll need to uplift Bug 1295630 too if we take this).

[Risks and why]: Moderate. This was a tricky patch to write, and hard to test manually. The code which does A/V sync is finickity, as it deals with timing. So I'm refraining from requesting Beta uplift. This is the sort of patch that just needs eyeballs and ear drums on the output (video playback) for any regressions to be detected.

Amazon's customer service are getting complaints that Firefox's audio is not in sync with video. Amazon have confirmed that this patch fixes that. So I think we should take this patch on Aurora, despite the risk.

[String/UUID change made/needed]: None.
Attachment #8781345 - Flags: approval-mozilla-aurora?
Comment on attachment 8781345 [details]
Bug 1258870 - Don't push late video frames to the compositor, drop them.

Even though the patch is medium risk, I am taking it given that this is a problem with Amazon video playback on slower machines and verified by Amazon, Aurora50+
Attachment #8781345 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1298594
JW - can you come up with a patch for beta that affects frame drop reporting but doesn't otherwise change playback behaviour?
Flags: needinfo?(jwwang)
Bug 1299018 will do that.
Flags: needinfo?(jwwang)
Depends on: 1299019
Depends on: 1299021
Depends on: 1299230
Depends on: 1307546
Depends on: 1316571
See Also: → 1349679
You need to log in before you can comment on or make changes to this bug.