1717171 - 30.75 - 6.01% basic_compositor_video / basic_compositor_video + 9 more (Linux, OSX, Windows) regression on Tue June 15 2021

Status: RESOLVED FIXED

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

Description

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Perfherder has detected a talos performance regression from push 97fb4c742d684da863aa27a32ae9d1daa8d67928. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
31%	basic_compositor_video		windows10-64-shippable-qr	e10s stylo webrender-sw	3.37 -> 4.40
25%	basic_compositor_video		windows10-64-shippable-qr	e10s stylo webrender	3.38 -> 4.23
23%	basic_compositor_video		windows10-64-qr	e10s stylo webrender-sw	3.38 -> 4.15
22%	basic_compositor_video		macosx1014-64-shippable-qr	e10s stylo webrender	2.71 -> 3.31
8%	basic_compositor_video		macosx1015-64-shippable-qr	e10s stylo webrender-sw	2.69 -> 2.92
8%	basic_compositor_video		macosx1015-64-shippable-qr	e10s stylo webrender	2.69 -> 2.92
8%	basic_compositor_video		macosx1015-64-shippable-qr	e10s stylo webrender	2.69 -> 2.92
7%	basic_compositor_video		linux1804-64-shippable-qr	e10s stylo webrender	2.69 -> 2.86
6%	basic_compositor_video		linux1804-64-shippable-qr	e10s stylo webrender-sw	2.69 -> 2.86
6%	basic_compositor_video		linux1804-64-qr	e10s stylo webrender-sw	2.69 -> 2.85
6%	basic_compositor_video		linux1804-64-qr	e10s stylo webrender	2.69 -> 2.85

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the offending patch(es) will be backed out in accordance with our regression policy.

For more information on performance sheriffing please see our FAQ.

Comment 1

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

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

Comment 2

[Alastor Wu [:alwu]]

The reason of increased time of rendering video frames might be caused by storing too many video frames in the advance, so the compositor needs more test to check which frame we would need to render? I'm going to restrict the mechanism of storing too many video frames on non-4k videos in bug1717119 which might help on this.

This is the comparison of performance, after applying patches in bug1717119.

Comment 3

[Alastor Wu [:alwu]]

The result above didn't show much improvement, but it might be just because of running the test once. I'm pretty sure the patch in bug1717119 will help because it would make storing more video frames only happening on 4K+ video.

Comment 4

[Alastor Wu [:alwu]]

Matt, may I ask your opinion on this bug?
What I did in bug 1692881 was to increase buffered video frames, would that be a reason to cause a performance degrade of rendering frames?
Thank you.

Comment 5

[Matt Woodrow (:mattwoodrow)]

I don't have any great guesses sorry. I'd recommend trying to use the gecko profiler mode on treeherder to get profiles before and after your change, and see how many frames are being presented and at what times.

Comment 6

[Pascal Chevrel:pascalc]

Alastor, any news on this bug? Thanks

Comment 7

[Alastor Wu [:alwu]]

So far I've done some improvement on bug 1717119, bug 1718309 and bug 1718709, but they didn't seem to help on the performance test directly. So I started to check the test itself today again, and found out that the new mechanism that is used to skip to next keyframe if playback is late is the root cause for performance degrade while playing testsrc.1080p.60fps.mp4 in video_playback.html.

For that video file, I found that we acutally pretty easy to hit the skip-to-next-keyframe, which means the decoded video is later than the audio clock. Now we use the audio clock to determine whether the video is late, which is similar with what we did in the VideoSink. However, we should only check the media time in the MDSM, and only let VideoSink to use the audio clock. Because if we seek to the next frame too aggresively, which means once there is one dropped frame, the video would jump to the next key frame directly which might cause some video freeze if the key frame has longer interval.

Comment 8

[Alastor Wu [:alwu]]

Attachment: Bug 1717171 - only skip to next key frame when video decoding is too slow and later than the media time.

We should only seek to the next keyframe in a really bad situation where the video is way too slow and we probably already drop a lot of frames.

Using the audio clock causes seeking to next key frame too aggresively, which would happen even if only one video is late. So we should use the media time to perform such checking.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c9241d58d52
only skip to next key frame when video decoding is too slow and later than the media time. r=bryce

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/9c9241d58d52

Comment 11

[Alastor Wu [:alwu]]

Comment on attachment 9232269 [details]
Bug 1717171 - only skip to next key frame when video decoding is too slow and later than the media time.

Beta/Release Uplift Approval Request

• User impact if declined: They might experience serve frame dropping while watching some video, which are not able to be decoded fast enough due to users' device ability. Based on the performace test result [1], it did help on the performance.

[1] https://bit.ly/3hTclFn

• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This change didn't introduce any new feature, structural change and it would improve the performance of rendering video for some videos. The worst situation after applying this patch is being the same (serious frame dropping) as before, and it won't make thing worst.
• String changes made/needed: no

Comment 12

[Pascal Chevrel:pascalc]

Comment on attachment 9232269 [details]
Bug 1717171 - only skip to next key frame when video decoding is too slow and later than the media time.

Approved for 91 beta 7, thanks.

Comment 13

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/3bae272896c3

Comment 14

[Florin Strugariu [:Bebe]]

== Change summary for alert #30659 (as of Mon, 26 Jul 2021 04:30:17 GMT) ==

Improvements:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
19%	basic_compositor_video		windows10-64-shippable-qr	e10s stylo webrender-sw	4.22 -> 3.40
19%	basic_compositor_video		windows10-64-qr	e10s stylo webrender-sw	4.15 -> 3.38
18%	basic_compositor_video		windows10-64-shippable-qr	e10s stylo webrender	4.17 -> 3.40
18%	basic_compositor_video		windows10-64-qr	e10s stylo webrender-sw	4.16 -> 3.43
18%	basic_compositor_video		macosx1014-64-shippable-qr	e10s stylo webrender	3.36 -> 2.77
...	...	...	...	...	...
6%	basic_compositor_video		linux1804-64-shippable-qr	e10s stylo webrender-sw	2.86 -> 2.69

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=30659