950556 - Get rid of GetThebesSurfaceForDrawTarget

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

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

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 2

[Bas Schouten (:bas.schouten)]

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

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

I am taking this; Matt or Bas, anything that you know about what needs to be done here, is useful information to me.

Comment 4

[Matt Woodrow (:mattwoodrow)]

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

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Filed bug 966451 for not using old layers on Windows anymore. This should take care of the CanvasLayerD3D* cases here.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

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

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 10

[Jonathan Watt [:jwatt]]

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

Comment 11

[Bas Schouten (:bas.schouten)]

This is done I believe.