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

Status: RESOLVED FIXED

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

Description

[Jeff Muizelaar [:jrmuizel]]

Basic compositor is currently very slow and we can get far behind the audio.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

I saw this when playing fullscreen video (h264) on youtube on XP with basic compositor

Comment 2

[Jean-Yves Avenard [:jya]]

i'm fairly certain we have another bug for this where A/V sync gets terrible when decoding/rendering video is way too slow

Comment 3

[Avi Halachmi (:avih)]

(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.

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Mass change P2 -> P3

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c033eafa82eb

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

(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.

Comment 9

[Matt Woodrow (:mattwoodrow)]

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.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00fbf0e3304c

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1258870 - Don't push late video frames to the compositor, drop them.

We can get out of A/V sync if the decode is struggling to keep up. This is
because of the loop in VideoSink::UpdateRenderedVideoFrames(). 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 results in us entering VideoSink::RenderVideoFrames() with only 1 late
frame, which the compositor dutifully draws. Resulting in a late frame being
drawn, and thus broken A/V sync.

This change makes VideoSink::UpdateRenderedVideoFrames() drop all late
frames, even the last one in the video queue. We now keep A/V sync when the
decode is struggling to keep up. However, if I do this, we end up dropping
(and reporting that we drop) a lot more frames, and thus rendering a lot fewer.
But since we when we drop the frames we report them as dropped, a well written
MSE player can detect that we've failing miserably to keep up, and and lower
their bitrate.

Review commit: https://reviewboard.mozilla.org/r/71758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71758/

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f39f2e1324b
Don't push late video frames to the compositor, drop them. r=jwwang

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4f39f2e1324b

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 18

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

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+

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/21fd3ea62d8c

Comment 20

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

JW - can you come up with a patch for beta that affects frame drop reporting but doesn't otherwise change playback behaviour?

Comment 21

[JW Wang [:jwwang] [:jw_wang]]

Bug 1299018 will do that.