Last Comment Bug 650988 - Do image scaling on the GPU
: Do image scaling on the GPU
Status: RESOLVED FIXED
: mobile, perf
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: mozilla15
Assigned To: Andreas Gal :gal
:
Mentors:
: 598874 (view as bug list)
Depends on: 750172
Blocks: 651060 598736 1099314
  Show dependency treegraph
 
Reported: 2011-04-18 16:57 PDT by Matt Woodrow (:mattwoodrow)
Modified: 2014-11-14 15:04 PST (History)
27 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Give images their own layer (3.97 KB, patch)
2011-04-18 16:57 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
rebased: give images their own layer (4.02 KB, patch)
2011-08-19 07:05 PDT, Florian Hänel [:heeen]
no flags Details | Diff | Splinter Review
rebased - actually compiles (4.25 KB, patch)
2011-08-19 07:35 PDT, Florian Hänel [:heeen]
no flags Details | Diff | Splinter Review
WIP (4.02 KB, patch)
2012-04-29 23:24 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
WIP, compiling (4.48 KB, patch)
2012-04-29 23:55 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (4.48 KB, patch)
2012-04-30 00:08 PDT, Andreas Gal :gal
roc: review-
Details | Diff | Splinter Review
patch (27.30 KB, patch)
2012-04-30 18:11 PDT, Andreas Gal :gal
roc: review-
Details | Diff | Splinter Review
patch (27.44 KB, patch)
2012-05-01 10:46 PDT, Andreas Gal :gal
roc: review-
Details | Diff | Splinter Review
patch (27.32 KB, patch)
2012-05-01 12:32 PDT, Andreas Gal :gal
roc: review+
Details | Diff | Splinter Review
patch with pref to disable it by default (36.50 KB, patch)
2012-05-01 22:54 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch with pref to disable it by default (29.36 KB, patch)
2012-05-01 22:55 PDT, Andreas Gal :gal
cjones.bugs: review+
Details | Diff | Splinter Review
Screenshot cnn.com, GPU image scaling disabled (1.52 MB, image/tiff)
2012-05-01 23:52 PDT, Andreas Gal :gal
no flags Details
Screenshot cnn.com, GPU image scaling enabled (1.46 MB, image/tiff)
2012-05-01 23:53 PDT, Andreas Gal :gal
no flags Details
Screenshot cnn.com, GPU image scaling disabled (1.21 MB, image/png)
2012-05-02 08:34 PDT, Joe Drew (not getting mail)
no flags Details
Screenshot cnn.com, GPU image scaling enabled (1.19 MB, image/png)
2012-05-02 08:35 PDT, Joe Drew (not getting mail)
no flags Details

Description Matt Woodrow (:mattwoodrow) 2011-04-18 16:57:11 PDT
Created attachment 526876 [details] [diff] [review]
Give images their own layer

We should look into creating separate layers for images so that scaling happens on the GPU.

We probably need to profile this to determine heuristics of when to do this, and when to stick with scaling in the content process.

Attached is my efforts while stuck on a plane, seems to mainly work except the images flicker sometimes. Only with GL layers though, BasicLayers appears to work 100% correct.
Comment 1 Doug Turner (:dougt) 2011-04-18 17:02:17 PDT
lets make sure we don't bump memory usage for mobile.
Comment 2 Matt Woodrow (:mattwoodrow) 2011-04-18 17:20:54 PDT
This will almost always increase total memory usage, but hopefully with a substantial performance win.

Hopefully we can tune this so we can get the best of both.

One thought I had (and this is substantially more complex), is we can delete the decoded image's backing store once it has been uploaded to a texture. Not sure how this would work exactly, since the GL texture objects lifetime would be tied to a layout (content process) object, instead of a layers object. It would fix the extra memory usage problem though.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-18 19:41:27 PDT
Is this the same as bug 598874?  Doesn't particularly matter except that roc posted a clever idea there for JPEG images.
Comment 4 Matt Woodrow (:mattwoodrow) 2011-04-18 22:40:45 PDT
This is indeed identical to bug 598874. Want me to close this and move the attachment over?
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-19 02:21:44 PDT
Nah, here is fine.  On second thought, it's probably better to track roc's idea in a separate bug.

*** This bug has been marked as a duplicate of bug 598874 ***
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-19 02:22:00 PDT
Urgh.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-19 02:22:24 PDT
*** Bug 598874 has been marked as a duplicate of this bug. ***
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-20 14:49:35 PDT
Comment on attachment 526876 [details] [diff] [review]
Give images their own layer

As a proof-of-concept this looks OK, though there are bugs in it ;).

The biggest omission from the heuristics here is the pres-shell resolution, which is the common case we care about on fennec.  I'm not sure if GetDestRect() here takes CSS transforms into account, but that's another input that would be nice to have.

Another big omission is that we only want to do this if we have "free" resampling in the chrome process.  We don't want to spin CPU resampling while the UI remains blocked unless we get some secondary benefit like reduced memory traffic.  Probably another IsCompositingCheap() check somewhere near here would be OK initially.

I think it's going to be fairly difficult to tune the heuristics.  Are there pages anyone knows of where we have high repaint lag from resampling large images?  (vs. say, decoding them on draw.)  We might need to construct those benchmarks.  I'm not comfortable landing a successor to this patch until we have a good suite to tune on.  It would be even better to get these kinds of tests into talos or another perf-test suite that's being ramped up right now.

(Clearing feedback request because I'm not sure whether + or - makes sense.)
Comment 9 Matt Woodrow (:mattwoodrow) 2011-04-24 16:54:10 PDT
How does the pres shell resolution code work for other layer types? Like video and canvas?

The IsCompositingCheap() check is on my todo list, will fix this.

Stuart: Any ideas regarding testing this?
Comment 10 Stuart Parmenter 2011-05-13 07:32:17 PDT
Given that we already retain images, I'm not sure why this approach should use more memory.  We probably need logic to create/destroy layers based on images coming/going from the image cache.  I suppose we'll want most images to live on the GPU from the start and use those as the official retained copy rather than the software one, so probably imagelib based hacking is also required here as well.  Probably a decent chunk of work, but if all done I don't see why we'd be worse off in any respect here.
Comment 11 Matt Woodrow (:mattwoodrow) 2011-05-13 15:00:01 PDT
The simple implementation (as I currently have it), won't affect the memory usage of ThebesLayers much (if at all) but it will cause the images to be stored a second time in texture memory.

But yes, we can definitely work around this with imagelib changes. We can also decode directly to YUV textures and speed up decoding.

As Chris said, the main blocker on getting this finished is deciding how to measure the performance changes of it.
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-21 07:55:15 PDT
Looks like Matt is doing the work, so I am assigning the bug to him. "nobody" wasn't right.
Comment 13 Florian Hänel [:heeen] 2011-08-19 07:05:58 PDT
Created attachment 554403 [details] [diff] [review]
rebased: give images their own layer
Comment 14 Florian Hänel [:heeen] 2011-08-19 07:35:03 PDT
Created attachment 554412 [details] [diff] [review]
rebased - actually compiles

last one didn't compile
Comment 15 Florian Hänel [:heeen] 2011-08-19 08:12:33 PDT
A few observations: does not create an image layer on google.de, but on other pages I get lots of white (empty?) images. This is on Maemo6.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-29 14:51:29 PDT
To make this work on background-image, nsDisplayBackground just needs similar treatment to nsDisplayImage here, hopefully with some refactoring to share code.

GetLayerState should be modified to pass in scale parameters so to make the heuristics here work properly.

I would do it without support for tiled images first, then add support for tiled images to ImageLayer and and tiled background-image support on top of that.
Comment 17 Andreas Gal :gal 2012-04-29 15:29:36 PDT
Matt, I am totally not qualified to work on this but I will poke at it. Feel free to steal back at any time if you have a few cycles to work on this.
Comment 18 Andreas Gal :gal 2012-04-29 23:21:50 PDT
Working on an updated patch for nsImageFrame and looking at how to do it for background images. I think I want a layer for the background image only, but right now ProcessDisplayItems assumes that we only get one layer back for a frame, so I would let frames with scaled backgrounds return a ContainerLayer that contains all the layers it needs. roc, does that sound right?
Comment 19 Andreas Gal :gal 2012-04-29 23:24:07 PDT
Created attachment 619497 [details] [diff] [review]
WIP
Comment 20 Andreas Gal :gal 2012-04-29 23:55:34 PDT
Created attachment 619498 [details] [diff] [review]
WIP, compiling
Comment 21 Andreas Gal :gal 2012-04-30 00:01:27 PDT
Patch works. What cnn.com does to be so dog slow on mobile is scaling up 1px images to really large sizes, by the way. Go cnn.com!
Comment 22 Andreas Gal :gal 2012-04-30 00:08:27 PDT
Created attachment 619499 [details] [diff] [review]
patch

We should do this everywhere, not just mobile. I will file a separate bug to do this for background images too.
Comment 23 Andreas Gal :gal 2012-04-30 00:13:10 PDT
Background image scaling on the GPU cloned as bug 750172.
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-30 00:39:00 PDT
(In reply to Andreas Gal :gal from comment #21)
> Patch works. What cnn.com does to be so dog slow on mobile is scaling up 1px
> images to really large sizes, by the way. Go cnn.com!

Those images are fully transparent, aren't they? We can optimize that particularly well...
Comment 25 Andreas Gal :gal 2012-04-30 00:46:42 PDT
Yes, those layers should turn into color layers.
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-30 02:25:15 PDT
Comment on attachment 619499 [details] [diff] [review]
patch

Review of attachment 619499 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsImageFrame.cpp
@@ +1268,5 @@
> +
> +  // Adjust it by the zoom factor.
> +  nsIPresShell* presShell = mFrame->PresContext()->GetPresShell();
> +  scale.width *= presShell->GetXResolution();
> +  scale.height *= presShell->GetYResolution();

This isn't right for images that are inside a transform. We need to thread a FrameLayerBuilder::ContainerParameters parameter through all calls to GetLayerState and use its resolution here.

@@ +1279,5 @@
> +  // If we are downscaling by more than 2x, avoid using a layer because of the
> +  // reduced memory usage of redrawing at the lower resolution.
> +  if (scale.width * scale.height < 0.5 * 0.5) {
> +    return LAYER_INACTIVE;
> +  }

Maybe instead of this check, we should simply check the overall size of the scaled image. If it's less than some threshold, don't use a layer. So small images scaled up (or down) would not need layers, and very large images scaled down could still use a layer.
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-30 02:28:05 PDT
(In reply to Andreas Gal :gal from comment #25)
> Yes, those layers should turn into color layers.

Fully transparent images can turn into nothing at all. We can actually eliminate them at display-list-construction time.

If we have important cases where single-color images (not fully transparent) are being used, we just need to make sure that drawing such images turns into a simple color fill that doesn't require scaling.

In general I would rather do those things than layer-ize general images, because those are clear wins that don't require tradeoffs.
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-30 02:29:32 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> In general I would rather do those things than layer-ize general images,
> because those are clear wins that don't require tradeoffs.

(We can still layer-ize general images if we have data showing it's a good win after we've done those other optimizations.)
Comment 29 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-30 15:25:38 PDT
++1x1 image -> color layer trick
Comment 30 Andreas Gal :gal 2012-04-30 18:11:03 PDT
Created attachment 619801 [details] [diff] [review]
patch
Comment 31 Andreas Gal :gal 2012-04-30 18:12:35 PDT
Comment on attachment 619801 [details] [diff] [review]
patch

This offloads a bunch of image scaling onto the GPU on cnn.com.
Comment 32 Benoit Girard (:BenWa) 2012-04-30 18:50:22 PDT
Do you have any performance numbers off hand?
Comment 33 Andreas Gal :gal 2012-04-30 19:08:07 PDT
I will try to get to it tonight, otherwise jeffm will do it in the morning. This doesn't address background images yet, so it won't handle the biggest chunk of cycles we spend on scaling.
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-30 20:02:47 PDT
Comment on attachment 619801 [details] [diff] [review]
patch

Review of attachment 619801 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsImageFrame.cpp
@@ +1278,5 @@
> +
> +  // If the target size is pretty small, no point in using a layer.
> +  if (destRect.width * destRect.height < 64 * 64) {
> +    return LAYER_INACTIVE;
> +  }

This calculation needs to take aParameters.mXScale/mYScale into account. Probably want to scale destRect width/height by mX/YScale before doing the rest.

Also ... We should do all this only if (aManager->IsCompositingCheap()), i.e., we're GPU accelerated.

It might also turn out that this is bad because it forces us to create more transparent layers on top of the images, or something like that. But we shall see.
Comment 35 Andreas Gal :gal 2012-05-01 10:46:32 PDT
Created attachment 619980 [details] [diff] [review]
patch
Comment 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-01 12:00:35 PDT
Comment on attachment 619980 [details] [diff] [review]
patch

Review of attachment 619980 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsImageFrame.cpp
@@ +1278,5 @@
> +    return LAYER_INACTIVE;
> +  }
> +
> +  destRect.width *= aParameters.mXScale;
> +  destRect.height *= aParameters.mYScale;

Move this up to right after GetDestRect(), then you don't need to multiple scale.width and scale.height by aParameters.mX/YScale separately.
Comment 37 Andreas Gal :gal 2012-05-01 12:32:48 PDT
Created attachment 620024 [details] [diff] [review]
patch
Comment 38 Andreas Gal :gal 2012-05-01 22:54:18 PDT
Created attachment 620200 [details] [diff] [review]
patch with pref to disable it by default
Comment 39 Andreas Gal :gal 2012-05-01 22:55:47 PDT
Created attachment 620201 [details] [diff] [review]
patch with pref to disable it by default
Comment 40 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-01 23:08:20 PDT
Comment on attachment 620201 [details] [diff] [review]
patch with pref to disable it by default

You need to learn the joys of mq ;).  Easier to review as separate patch.

>+bool
>+nsLayoutUtils::UseGPUToScaleImages()

>+    sUseGPUToScaleImagesEnabled = mozilla::Preferences::GetBool("layout.gpu-scaling.enabled", true);

sGPUImageScalingEnabled, pref name "layout.gpu-image-scaling.enabled"

This should default to false, and you need to enable it explicitly for
fennec-android, mobile/android/app/mobile.js.  Right?

>diff --git a/layout/base/nsLayoutUtils.h b/layout/base/nsLayoutUtils.h

>   /**
>+   * Checks whether we want to use the GPU to scale images when
>+   * possible.
>+   */
>+  static bool UseGPUToScaleImages();
>+

Call this |GPUImageScalingEnabled()|.  "Use..()" and the comment here suggests it's doing
an interesting logic check, which it's not.

r=me on pref check code only, with those fixes.
Comment 41 Andreas Gal :gal 2012-05-01 23:52:46 PDT
Created attachment 620212 [details]
Screenshot cnn.com, GPU image scaling disabled
Comment 42 Andreas Gal :gal 2012-05-01 23:53:17 PDT
Created attachment 620213 [details]
Screenshot cnn.com, GPU image scaling enabled
Comment 43 Andreas Gal :gal 2012-05-01 23:54:59 PDT
As an added benefit, this patch substantially improves image scaling quality. Checkout the "Click to play" text in the image for example. The screenshots were taken on a MacOSX desktop (and zoomed in).
Comment 44 Joe Drew (not getting mail) 2012-05-02 08:34:42 PDT
Created attachment 620330 [details]
Screenshot cnn.com, GPU image scaling disabled

TIFF?! for shame. :)
Comment 45 Joe Drew (not getting mail) 2012-05-02 08:35:14 PDT
Created attachment 620331 [details]
Screenshot cnn.com, GPU image scaling enabled
Comment 46 Andreas Gal :gal 2012-05-02 08:38:21 PDT
Mac OS X Grab :) It was tiff or jpeg ;)
Comment 48 Ed Morley [:emorley] 2012-05-04 09:27:23 PDT
https://hg.mozilla.org/mozilla-central/rev/ed9e42293a6f

Note You need to log in before you can comment on or make changes to this bug.