Closed Bug 923341 Opened 6 years ago Closed 6 years ago

Update imagelib surface caching code to avoid creating unnecessary Thebes contexts


(Core :: ImageLib, defect)

Not set





(Reporter: seth, Assigned: mattwoodrow)


(Depends on 2 open bugs)


(Whiteboard: [qa-])


(4 files)

Once we have gfxDrawTargetDrawable we can remove the unnecessary GetThebesSurfaceForDrawTarget() call in VectorImage::CreateDrawableAndShow.
We should also update CachedSurface::Drawable. Any code that uses SurfaceCache should get a lookover once gfxDrawTargetDrawable is available.
Summary: Update VectorImage::CreateDrawableAndShow to avoid creating unnecessary Thebes contexts → Update imagelib surface caching code to avoid creating unnecessary Thebes contexts
At the moment it looks like this will be solved by adding features to Moz2D (source clipping for patterns) to support the missing use cases we need, and then not calling DrawPixelSnapped at all on Moz2D platforms. This will get hammered out in more detail at the upcoming web rendering work week.
This adds a dreaded call to GetThebesSurfaceForDrawTarget, but only to use as a source. This should work everywhere, though may involve readback.

There's no other way to avoid this other than just never having a gfxContext that returns true for IsCairo() (which we're working on!).
Attachment #831283 - Flags: review?(seth)
Blocks: 937519
Assignee: seth → matt.woodrow
Attachment #831283 - Flags: review?(seth) → review+
Comment on attachment 831284 [details] [diff] [review]
Part 2: Use SVG caches on all azure backends.

Review of attachment 831284 [details] [diff] [review]:

So glad that ugly code is gone!

::: image/src/VectorImage.cpp
@@ +836,5 @@
>    // If we couldn't create the draw target, it was probably because it would end
>    // up way too big. Generally it also wouldn't fit in the cache, but the prefs
>    // could be set such that the cache isn't the limiting factor. If we couldn't
>    // create the surface, we are on a platform that does not support
>    // GetThebesSurfaceForDrawTarget. This will be fixed in bug 923341.

Should probably update this comment since |surface| is gone now. (Can just remove the last two sentences, I think.)

Is there another reason CreateOffscreenContentDrawTarget could fail?

@@ +842,2 @@
>      return Show(svgDrawable, aParams);
> +  

Nit: Stray whitespace.
Attachment #831284 - Flags: review?(seth) → review+
Comment on attachment 831285 [details] [diff] [review]
Part 3: Use azure for CreateSamplingRestrictedDrawable

Review of attachment 831285 [details] [diff] [review]:

::: gfx/thebes/gfxUtils.cpp
@@ +271,5 @@
> +    if (target) {
> +      drawable = new gfxSurfaceDrawable(target, size, gfxMatrix().Translate(-needed.TopLeft()));
> +    } else {
> +      drawable = new gfxSurfaceDrawable(temp, size, gfxMatrix().Translate(-needed.TopLeft()));
> +    }

Maybe just fold this into the previous if/else pair?
Attachment #831285 - Flags: review?(seth) → review+
Missed this the first time.
Attachment #831812 - Flags: review?(seth)
Comment on attachment 831812 [details] [diff] [review]
Part 4: Update CachedSurface::Drawable

Review of attachment 831812 [details] [diff] [review]:

Looks good to me.
Attachment #831812 - Flags: review?(seth) → review+
Depends on: 942364
Whiteboard: [qa-]
Depends on: 985882
Depends on: 990152
Depends on: 1038309
You need to log in before you can comment on or make changes to this bug.