Get rid of GetThebesSurfaceForDrawTarget

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mattwoodrow, Assigned: bjacob)

Tracking

(Depends on 2 bugs)

unspecified
mozilla31
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

We can't support this properly with some backends (skia-gl, direct2d 1.1), so we need to get rid of it and use moz2d instead.

We can implement it in a way that does readback, for callers that don't want to write, but this will be slow.

We need to get rid of all the callers that write to the returned surface, as well as all the performance critical reading ones before we can enable direct 1.1
Bas, is this in the "stop asking Moz2D for Thebes" bucket?  If so, what other things are in that bucket, and do we have bugs for them?
Flags: needinfo?(bas)
(In reply to Milan Sreckovic [:milan] from comment #1)
> Bas, is this in the "stop asking Moz2D for Thebes" bucket?  If so, what
> other things are in that bucket, and do we have bugs for them?

This is more or less in that bucket. It's also definitely the most important thing in that bucket. I'm not sure if there are any other things which are in that specific bucket, I don't think so.
Flags: needinfo?(bas)
Assignee: nobody → bjacob
I am taking this; Matt or Bas, anything that you know about what needs to be done here, is useful information to me.
http://mxr.mozilla.org/mozilla-central/ident?i=GetThebesSurfaceForDrawTarget&tree=mozilla-central&filter=

I assume you're taking this as a step towards getting bug 902952 fixed.

For Direct2D 1.1, GetThebesSurfaceForDrawTarget will return a valid surface for reading from (using readback), but writing to this surface won't work. It's also obviously going to be slow for the former.

With that in mind we can probably ignore all the callers that use the result for debugging (since we don't overly care about speed there), and callers that won't ever be hit by direct2d (d3d9, osx, etc).

I think that takes us down to the ClippedImage/OrientedImage callers (needs bug 950372), CanvasLayerD3D10. gfxDrawable (though it's inside an if branch that I'd hope we never actually take), and the callers of CanvasRenderingContext2D::GetThebesSurface (I think there's only one).
Thanks Matt for the answers.

I also found CreateThebesSurfaceAliasForDrawTarget_hack.

Conversely, it seems that CanvasLayerD3D10 is not important to fix at this point, as the ongoing work to get Windows on new-layers will automatically remove CanvasLayerD3D10 and CanvasLayerD3D9.

This leaves us with this list of relevant callers to prioritize cleaning up:
  CreateThebesSurfaceAliasForDrawTarget_hack
  CanvasRenderingContext2D::GetThebesSurface
  gfxSurfaceDrawable::Draw
  ClippedImageSurfaceCache::Matches
  ClippedImage::GetFrameInternal
  OrientedImage::GetFrame

I'll file bugs about these individually.
Filed bug 966451 for not using old layers on Windows anymore. This should take care of the CanvasLayerD3D* cases here.
Depends on: 966451
Depends on: 966455
Depends on: 966479
I was looking at gfxSurfaceDrawable::Draw, and that is one huge piece; but I am told that Bas is already taking care of this, as well as taking care of the ImageLib bits.

Bas, can you elaborate, and block the present bug on all the bugs pertaining to gfxSurfaceDrawable::Draw and ImageLib that block it? In the current blockers list, I don't see anything that obviously covers gfxSurfaceDrawable::Draw, but I could just be failing to interprete things.
No longer depends on: 966455, 966479
Flags: needinfo?(bas)
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> gfxDrawable (though it's inside an if branch
> that I'd hope we never actually take)

It would be very interesting to know whether we take it :-) Bas, any idea about that?
Depends on: 966455, 966479
Had a phone call with Bas, from which the main take-away was that essentially the only part that matters here is the ImageLib part.

In particular, Bas confirmed that gfxSurfaceDrawable is only using GetThebesSurfaceForDrawTarget in a non-performance-sensitive code path.

Work is moving over to bug 950372 where Bas is attaching his work-in-progress patch.
(In reply to Benoit Jacob [:bjacob] from comment #9)
> Had a phone call with Bas, from which the main take-away was that
> essentially the only part that matters here is the ImageLib part.

Maybe this shouldn't block bug 902952 then? I just marked a direct dependency on bug 950372 (and landed the fix on m-i).
This is done I believe.
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(bas)
Resolution: --- → FIXED
No longer blocks: 902952
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.