1255281 - Add pixman fast path for bilinear x888_8888_SRC

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f633e07ba908&selectedJob=17854822

Comment 1

[Jeff Muizelaar [:jrmuizel]]

This causes a bunch of test failures that I don't understand. I may try to rr them.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

I was just an idiot and was pushing on top of broken stuff.

This should be better:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5eb8ff47346a

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add pixman fast path for bilinear x888_8888_SRC

This avoids falling back to a slow path when doing x8888 -> 8888 scales. It is basically just cherrypicked from upstream.

Comment 4

[Lee Salzman [:lsalzman]]

Comment on attachment 8729256 [details] [diff] [review]
Add pixman fast path for bilinear x888_8888_SRC

Looks sane. It should work.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b84619dbcbd9

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b84619dbcbd9

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8729256 [details] [diff] [review]
Add pixman fast path for bilinear x888_8888_SRC

Approval Request Comment
[Feature/regressing bug #]: Not sure. I think things have been bad a long time.
[User impact if declined]: Continued bad video performance with basic layers. This patch improves video performance quite a bit.
[Describe test coverage new/current, TreeHerder]: Has been on Nightly for a little while.
[Risks and why]: Pretty low risk. Very easy to back out.

Comment 8

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

Hi Jeff, Milan: Is there a way to quantitatively confirm the perf improvements with this patch? I am trying to debate the need to uplift this to 47 versus letting it ride the trains. While back outs are easy to do, we should be cautious about uplifting. This time I am also trying to minimize code churn in Aurora cycle to see if it helps improve release quality. Is the perf improvement worth the risk here?

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to Ritu Kothari (:ritu) from comment #8)
> Hi Jeff, Milan: Is there a way to quantitatively confirm the perf
> improvements with this patch? I am trying to debate the need to uplift this
> to 47 versus letting it ride the trains. While back outs are easy to do, we
> should be cautious about uplifting. This time I am also trying to minimize
> code churn in Aurora cycle to see if it helps improve release quality. Is
> the perf improvement worth the risk here?

I believe this at least doubles the frame rate of video with basic layers on Windows. The perf improvement is so large that I'd like us to consider uplifting it to beta. (I'll try to get more concrete numbers before asking for that)

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Sotaro, can you measure video performance with builds before and after this change so that we can get some concrete numbers?

Comment 11

[Lee Salzman [:lsalzman]]

(In reply to Ritu Kothari (:ritu) from comment #8)
> Hi Jeff, Milan: Is there a way to quantitatively confirm the perf
> improvements with this patch? I am trying to debate the need to uplift this
> to 47 versus letting it ride the trains. While back outs are easy to do, we
> should be cautious about uplifting. This time I am also trying to minimize
> code churn in Aurora cycle to see if it helps improve release quality. Is
> the perf improvement worth the risk here?

We also encountered at least one page in the wild doing some animation transform effects that repeatedly triggered pixman's slow path without this patch. Combine this with the fact that on Linux we moved away from using Xrender, so that we now rely on pixman for performance. I would wager that this patch will save us more trouble in the end -- it may result in fewer reports of performance regressions there.

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #10)
> Sotaro, can you measure video performance with builds before and after this
> change so that we can get some concrete numbers?

I tested the following video on my laptop with BasicCompositor enabled(disable hw acceleration).
   https://www.youtube.com/watch?v=e-ORhEE9VVg

  - full screen video playback with the patch: 10 fps
  - full screen video playback without the patch: 3fps

With the patch, fps improved a lot in full screen video playback case.

Comment 13

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

Thanks Jeff, Lee and Sotaro. I am convinced that this patch is worth uplifting to Aurora. Appreciate the due diligence.

Comment 14

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

Comment on attachment 8729256 [details] [diff] [review]
Add pixman fast path for bilinear x888_8888_SRC

Fix was verified to improve performance (almost 3x), has been in Nightly for a few days, taking it. Aurora47+

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/93f4293d8302

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8729256 [details] [diff] [review]
Add pixman fast path for bilinear x888_8888_SRC

Approval Request Comment
[Feature/regressing bug #]: Not sure. I think things have been bad a long time.
[User impact if declined]: Continued bad video performance with basic layers. This patch improves video performance quite a bit.
[Describe test coverage new/current, TreeHerder]: Has been on Aurora and Nightly for a quite a while.
[Risks and why]: Very low risk. Very easy to back out. I know it's late but this code is very self contained and doesn't have many ways to go wrong.

This gives a 3x performance improvement on video decoding for users with basic layers (30% of our windows users)

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8729256 [details] [diff] [review]
Add pixman fast path for bilinear x888_8888_SRC

Big perf improvement, let's try this for beta 11.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/da4c947c01e6

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Anthony, looks like we want Betabreakers on non-HWA video somewhat sooner.

Comment 20

[u279076]

(In reply to Milan Sreckovic [:milan] from comment #19)
> Anthony, looks like we want Betabreakers on non-HWA video somewhat sooner.

The absolute earliest we could start would be next Tuesday with results early the following week.

Comment 21

[u279076]

(In reply to Anthony Hughes (:ashughes) [GFX][QA][Mentor] from comment #20)
> (In reply to Milan Sreckovic [:milan] from comment #19)
> > Anthony, looks like we want Betabreakers on non-HWA video somewhat sooner.
> 
> The absolute earliest we could start would be next Tuesday with results early the following week.

Florin, since it's going to be several days before we get results from Betabreakers, could you please run the following test as part of your next Beta sign off? 

(test as many platforms, GPUs, drivers as you have time for)
1. Start Firefox with a new profile
2. Make sure "Use hardware acceleration when available" is unchecked, and restart Firefox
3. Play https://www.youtube.com/watch?v=zKkKGlMLkow and test different size/quality modes
4. Play https://www.youtube.com/watch?v=-xNN-bJQ4vI and test dragging around the video in different size/quality modes
5. Play https://vimeo.com/channels/staffpicks/162434937 and test different size/quality modes
6. Test watching an embedded video on Facebook, Duckduckgo, and Reddit
7. Test watching a streaming video on Steam, and Twitch.tv
8. Pick a News website at random and play some videos (see http://www.alexa.com/topsites/category/Top/News)
9. Pick a Sports website at random and play some videos (see http://www.alexa.com/topsites/category/Top/Sports) 

If you encounter an issue with video playback including crashes/hangs please do the following:
* verify if the issue reproduces
* verify if the issue reproduces with hardware acceleration enabled
* verify if the issue reproduces in Firefox 45.0.2
* file a new bug and attach the Graphics section of your about:support 

Thank you in advance.

Comment 22

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Adding Andrei to see how we can fit this in.

Comment 23

[Andrei Vaida [:avaida]]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #22)
> Adding Andrei to see how we can fit this in.

We've added this to our 46.0b11 sign off. We'll follow up with test results as soon as testing is completed.

Comment 24

[Andrei Vaida [:avaida]]

We finished testing this using tinderbox builds, here's a summary of what we've managed to cover.

Overview:
* Windows 7 x64 (Intel HD 2500) – x64, en-US build [1],
* Windows 10 x64 (Intel HD 4600) – x86, en-US build [2],
* Mac OS X 10.10.5 (AMD Radeon HD 6750) – x64, en-US build [3],
* Ubuntu 12.04 x86 – x86, en-US build [4].

Apart from a few already known bugs - none of them specifically related to what we were testing here - there was 1 new issue uncovered: Bug 1264969, a crash that's reproducible with HWA enabled as well.

Notable known bugs we encountered: Bug 1241875, a crash that we've only happened to reproduce once, on OS X.


[1] http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-win64/1460672624/
[2] http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-win32/1460672624/
[3] http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-macosx64/1460672624/
[4] http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-linux/1460672624/

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

and a perf win on beta for tsvgx:
https://treeherder.mozilla.org/perf.html#/alerts?id=900