650988 - Do image scaling on the GPU

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: 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)]

lets make sure we don't bump memory usage for mobile.

Comment 2

[Matt Woodrow (:mattwoodrow)]

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]

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)]

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]

Nah, here is fine.  On second thought, it's probably better to track roc's idea in a separate bug.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Urgh.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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)]

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]

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)]

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?)]

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]]

Attachment: rebased: give images their own layer

Comment 14

[Florian Hänel [:heeen]]

Attachment: rebased - actually compiles

last one didn't compile

Comment 15

[Florian Hänel [:heeen]]

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)]

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]

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]

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]

Attachment: WIP

Comment 20

[Andreas Gal :gal]

Attachment: WIP, compiling

Comment 21

[Andreas Gal :gal]

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]

Attachment: 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]

Background image scaling on the GPU cloned as bug 750172.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Yes, those layers should turn into color layers.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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)]

(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)]

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

++1x1 image -> color layer trick

Comment 30

[Andreas Gal :gal]

Attachment: patch

Comment 31

[Andreas Gal :gal]

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)]

Do you have any performance numbers off hand?

Comment 33

[Andreas Gal :gal]

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)]

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]

Attachment: patch

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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]

Attachment: patch

Comment 38

[Andreas Gal :gal]

Attachment: patch with pref to disable it by default

Comment 39

[Andreas Gal :gal]

Attachment: patch with pref to disable it by default

Comment 40

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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]

Attachment: Screenshot cnn.com, GPU image scaling disabled

Comment 42

[Andreas Gal :gal]

Attachment: Screenshot cnn.com, GPU image scaling enabled

Comment 43

[Andreas Gal :gal]

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)]

Attachment: Screenshot cnn.com, GPU image scaling disabled

TIFF?! for shame. :)

Comment 45

[Joe Drew (not getting mail)]

Attachment: Screenshot cnn.com, GPU image scaling enabled

Comment 46

[Andreas Gal :gal]

Mac OS X Grab :) It was tiff or jpeg ;)

Comment 47

[Andreas Gal :gal]

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

Comment 48

[Ed Morley [:emorley]]

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