SkiaGL constantly reuploads texture during drawImage in this testcase

RESOLVED FIXED in Firefox 45, Firefox OS v2.5

Status

()

Core
Canvas: 2D
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mstange, Assigned: mattwoodrow)

Tracking

Trunk
mozilla45
All
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox44 affected, firefox45 fixed, b2g-v2.5 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
Steps to reproduce:
 1. Go to https://medium.com/art-science/a-warning-for-our-next-great-screenwriters-4af580c2e0b7
 2. Scroll up and down
 3. Notice content process jank.

Profile: http://people.mozilla.org/~bgirard/cleopatra/#report=22df8ae384601acf114cb51e9849fc96fba0238f

The profile shows a surprisingly large percentage of time spent in this callstack:

> _platform_memmove$VARIANT$Nehalem
> glrWriteTextureData
> glTexImage2D_Exec
> glTexImage2D
> glTexImage2D_mozilla
> GrGpuGL::uploadTexData(GrGLTexture::Desc const&, bool, int, int, int, int, GrPixelConfig, void const*, unsigned long)
> GrGpuGL::onCreateTexture(GrTextureDesc const&, void const*, unsigned long)
> GrGpu::createTexture(GrTextureDesc const&, void const*, unsigned long)
> GrContext::createTexture(GrTextureParams const*, GrTextureDesc const&, GrCacheID const&, void const*, unsigned long, GrResourceKey*)
> GrLockAndRefCachedBitmapTexture(GrContext*, SkBitmap const&, GrTextureParams const*)
> SkGpuDevice::internalDrawBitmap(SkBitmap const&, SkRect const&, GrTextureParams const&, SkPaint const&, SkCanvas::DrawBitmapRectFlags, bool, bool)
> SkGpuDevice::drawBitmapCommon(SkDraw const&, SkBitmap const&, SkRect const*, SkSize const*, SkPaint const&, SkCanvas::DrawBitmapRectFlags)
> SkGpuDevice::drawBitmapRect(SkDraw const&, SkBitmap const&, SkRect const*, SkRect const&, SkPaint const&, SkCanvas::DrawBitmapRectFlags)
> SkCanvas::internalDrawBitmapRect(SkBitmap const&, SkRect const*, SkRect const&, SkPaint const*, SkCanvas::DrawBitmapRectFlags)
> mozilla::gfx::DrawTargetSkia::DrawSurface(mozilla::gfx::SourceSurface*, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::DrawSurfaceOptions const&, mozilla::gfx::DrawOptions const&)
> mozilla::dom::CanvasRenderingContext2D::DrawImage(mozilla::dom::HTMLImageElementOrHTMLCanvasElementOrHTMLVideoElementOrImageBitmap const&, double, double, double, double, double, double, double, double, unsigned char, mozilla::ErrorResult&)
> mozilla::dom::CanvasRenderingContext2DBinding::drawImage(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, JSJitMethodCallArgs const&)

Are we painting the same SourceSurface each time? If so, why is Skia evicting its texture? Or if that's not what's happening, what is?
(Assignee)

Comment 1

2 years ago
The first time we paint the big image, the destination is a canvas that *doesn't* have skia-gl enabled. We call OptimizeSourceSurface (which does nothing and returns a SourceSurfaceRawData), and then stick that 'optimized' surface into the canvas image cache.

All following draws of the image are to the real canvas (which does have skia-gl) and we reuse the image from our cache, and have to upload it each time.

If you stop scrolling a wait for a while then the image cache will expire, and when we start scrolling again everything works correctly.

Our CanvasImageCache code is designed expecting that we only have one canvas backend, which is true at the Moz2D API level but not functionally true for skia vs skia-gl.

We have a few options to fix this:

1) Modify CanvasImageCache (and the 'simple' cache) to take a boolean 'isAccelerated' flag into account for the hash keys and store separate images for skia and skia-gl canvases. Adds a bit of annoying code just to deal with skia-gl weirdness.

2) Make DrawTargetSkia::OptimizeSourceSurface always make a copy of the source data into a new SourceSurfaceSkia (with a retained SkBitmap) so that there is no longer a functional difference between surfaces optimized for skia vs skia-gl. This will make the software case slower though, since we're copying unnecessarily.

2b) Make DrawTargetSkia::OptimizeSourceSurface never make a copy, but instead create a wrapper SourceSurfaceSkia that holds a strong reference to the original surface. Actually improves performance over baseline (by skipping the copy), but is a bit weird to have dependent SourceSurfaces.

I think I like 2b the most, so I'll start on a patch for that.
Assignee: nobody → matt.woodrow
(Assignee)

Comment 2

2 years ago
Actually, that will require us to keep the original surface Mapped for the entire lifetime of the optimized surface.

That's probably abusing the API a bit much, not sure we can get away with it.
(Assignee)

Comment 3

2 years ago
Created attachment 8682305 [details] [diff] [review]
Take SkiaGL status into account for the canvas image cache
Attachment #8682305 - Flags: review?(mstange)
(Assignee)

Comment 4

2 years ago
It would be nice if we could do 2b at some point in the future, since it would remove a copy when drawing an image for the first time for skia-gl.

We'd need Moz2D API support to determine when the data is permanently available (like SourceSurfaceRawData) so we can retain skia's SkBitmap in some form.
(Reporter)

Comment 5

2 years ago
Comment on attachment 8682305 [details] [diff] [review]
Take SkiaGL status into account for the canvas image cache

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

::: dom/canvas/CanvasImageCache.cpp
@@ +266,2 @@
>  {
> +  printf("Inserting image into cache: %d\n", aIsAccelerated);

please remove
Attachment #8682305 - Flags: review?(mstange) → review+
(Reporter)

Comment 6

2 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> The first time we paint the big image, the destination is a canvas that
> *doesn't* have skia-gl enabled.

Do you know why? Is it a different canvas, one that has been demoted because of pixel accesses?
(Assignee)

Comment 7

2 years ago
(In reply to Markus Stange [:mstange] from comment #6)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> > The first time we paint the big image, the destination is a canvas that
> > *doesn't* have skia-gl enabled.
> 
> Do you know why? Is it a different canvas, one that has been demoted because
> of pixel accesses?

Too small. We require at least 128x128 for skia-gl. It's a different canvas, yeah.

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/92a73b885c87
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.