The default bug view has changed. See this FAQ.

Do image scaling on the GPU

RESOLVED FIXED in mozilla15

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: mattwoodrow, Assigned: gal)

Tracking

(Blocks: 1 bug, {mobile, perf})

unspecified
mozilla15
mobile, perf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec+)

Details

Attachments

(4 attachments, 11 obsolete attachments)

27.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
29.36 KB, patch
cjones
: review+
Details | Diff | Splinter Review
1.21 MB, image/png
Details
1.19 MB, image/png
Details
(Reporter)

Description

6 years ago
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.
Attachment #526876 - Flags: feedback?(jones.chris.g)

Comment 1

6 years ago
lets make sure we don't bump memory usage for mobile.
(Reporter)

Comment 2

6 years ago
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.
Is this the same as bug 598874?  Doesn't particularly matter except that roc posted a clever idea there for JPEG images.
(Reporter)

Comment 4

6 years ago
This is indeed identical to bug 598874. Want me to close this and move the attachment over?
Nah, here is fine.  On second thought, it's probably better to track roc's idea in a separate bug.
Blocks: 598736
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 598874
Urgh.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Duplicate of this bug: 598874
Blocks: 651060
tracking-fennec: --- → ?
Keywords: mobile, perf
OS: Mac OS X → All
Hardware: x86 → All
tracking-fennec: ? → 2.0-
Whiteboard: [fennec-6]
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.)
Attachment #526876 - Flags: feedback?(jones.chris.g)
(Reporter)

Comment 9

6 years ago
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?
tracking-fennec: - → 6+
Whiteboard: [fennec-6]

Comment 10

6 years ago
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.
(Reporter)

Comment 11

6 years ago
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.
tracking-fennec: 6+ → 7+
Looks like Matt is doing the work, so I am assigning the bug to him. "nobody" wasn't right.
Assignee: nobody → matt.woodrow
tracking-fennec: 7+ → ?
tracking-fennec: ? → 8+
tracking-fennec: 8+ → +
Created attachment 554403 [details] [diff] [review]
rebased: give images their own layer
Attachment #526876 - Attachment is obsolete: true
Created attachment 554412 [details] [diff] [review]
rebased - actually compiles

last one didn't compile
Attachment #554403 - Attachment is obsolete: true
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.
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.
(Assignee)

Comment 17

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: matt.woodrow → gal
(Assignee)

Comment 18

5 years ago
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?
(Assignee)

Comment 19

5 years ago
Created attachment 619497 [details] [diff] [review]
WIP
Attachment #554412 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Created attachment 619498 [details] [diff] [review]
WIP, compiling
Attachment #619497 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
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!
(Assignee)

Comment 22

5 years ago
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.
Attachment #619498 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #619499 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Blocks: 750172
(Assignee)

Updated

5 years ago
No longer blocks: 750172
Depends on: 750172
(Assignee)

Updated

5 years ago
Summary: Do image scaling on the GPU on mobile → Do image scaling on the GPU
(Assignee)

Comment 23

5 years ago
Background image scaling on the GPU cloned as bug 750172.
(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...
(Assignee)

Comment 25

5 years ago
Yes, those layers should turn into color layers.
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.
Attachment #619499 - Flags: review?(roc) → review-
(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.
(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.)
++1x1 image -> color layer trick
(Assignee)

Comment 30

5 years ago
Created attachment 619801 [details] [diff] [review]
patch
Attachment #619499 - Attachment is obsolete: true
(Assignee)

Comment 31

5 years ago
Comment on attachment 619801 [details] [diff] [review]
patch

This offloads a bunch of image scaling onto the GPU on cnn.com.
Attachment #619801 - Flags: review?(roc)
Do you have any performance numbers off hand?
(Assignee)

Comment 33

5 years ago
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 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.
Attachment #619801 - Flags: review?(roc) → review-
(Assignee)

Comment 35

5 years ago
Created attachment 619980 [details] [diff] [review]
patch
Attachment #619801 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #619980 - Flags: review?(roc)
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.
Attachment #619980 - Flags: review?(roc) → review-
(Assignee)

Comment 37

5 years ago
Created attachment 620024 [details] [diff] [review]
patch
Attachment #619980 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #620024 - Flags: review?(roc)
Attachment #620024 - Flags: review?(roc) → review+
(Assignee)

Comment 38

5 years ago
Created attachment 620200 [details] [diff] [review]
patch with pref to disable it by default
(Assignee)

Comment 39

5 years ago
Created attachment 620201 [details] [diff] [review]
patch with pref to disable it by default
Attachment #620200 - Attachment is obsolete: true
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.
Attachment #620201 - Flags: review+
(Assignee)

Comment 41

5 years ago
Created attachment 620212 [details]
Screenshot cnn.com, GPU image scaling disabled
(Assignee)

Comment 42

5 years ago
Created attachment 620213 [details]
Screenshot cnn.com, GPU image scaling enabled
(Assignee)

Comment 43

5 years ago
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).
Created attachment 620330 [details]
Screenshot cnn.com, GPU image scaling disabled

TIFF?! for shame. :)
Attachment #620212 - Attachment is obsolete: true
Created attachment 620331 [details]
Screenshot cnn.com, GPU image scaling enabled
Attachment #620213 - Attachment is obsolete: true
(Assignee)

Comment 46

5 years ago
Mac OS X Grab :) It was tiff or jpeg ;)
(Assignee)

Comment 47

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed9e42293a6f
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/ed9e42293a6f
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Blocks: 1099314
You need to log in before you can comment on or make changes to this bug.