Closed Bug 1255281 Opened 9 years ago Closed 9 years ago

Add pixman fast path for bilinear x888_8888_SRC

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

(Blocks 1 open bug)

Details

(Whiteboard: gfx-noted)

Attachments

(1 file)

Add pixman fast path for bilinear x888_8888_SRC
4.68 KB, patch
lsalzman
: review+
ritu
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta+
Details | Diff | Splinter Review
Blocks: unaccel-video
This causes a bunch of test failures that I don't understand. I may try to rr them.
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
This avoids falling back to a slow path when doing x8888 -> 8888 scales. It is basically just cherrypicked from upstream.
Attachment #8729256 - Flags: review?(lsalzman)
Comment on attachment 8729256 [details] [diff] [review] Add pixman fast path for bilinear x888_8888_SRC Looks sane. It should work.
Attachment #8729256 - Flags: review?(lsalzman) → review+
https://hg.mozilla.org/mozilla-central/rev/b84619dbcbd9
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Blocks: 1255834
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.
Attachment #8729256 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(milan)
Flags: needinfo?(jmuizelaar)
(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)
Flags: needinfo?(jmuizelaar)
Sotaro, can you measure video performance with builds before and after this change so that we can get some concrete numbers?
Flags: needinfo?(milan) → needinfo?(sotaro.ikeda.g)
(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.
(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.
Flags: needinfo?(sotaro.ikeda.g)
Thanks Jeff, Lee and Sotaro. I am convinced that this patch is worth uplifting to Aurora. Appreciate the due diligence.
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+
Attachment #8729256 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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)
Attachment #8729256 - Flags: approval-mozilla-beta?
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.
Attachment #8729256 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Anthony, looks like we want Betabreakers on non-HWA video somewhat sooner.
Assignee: nobody → jmuizelaar
Flags: needinfo?(anthony.s.hughes)
(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.
Flags: needinfo?(anthony.s.hughes)
(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.
Flags: needinfo?(florin.mezei)
Adding Andrei to see how we can fit this in.
Flags: needinfo?(florin.mezei) → needinfo?(andrei.vaida)
(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.
Flags: qe-verify+
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/
Flags: needinfo?(andrei.vaida)
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: