688342 - [Azure] Let nsCanvasRenderingContext2DAzure use backends other than D2D

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: Add BACKEND_DEFAULT and use it to automatically decide the skia backend to use

We need this so that we can enable other backends for Azure.

CC'ing Joe since he also had a patch to the effect, and we may want to use that instead.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Bas did you have any thoughts on this?

Having the decision logic inside Factory.cpp seems wrong here, we probably should be exposing something through gfxPlatform instead.

Calling this directly from nsCanvasRenderingContext, or having a BACKEND_DEFAULT option and calling it from Factory.cpp doesn't matter much to me.

Any other suggestions?

Comment 2

[Bas Schouten (:bas.schouten)]

In any normal case we should be using the layer manager and this will make the decision. The rare case where something is wrong and we can't find it, we should ass a gfxPlatform::CreateOffscreenDrawTarget like we have for surfaces.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Add CreateOffscreenDrawTarget

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 570583 [details] [diff] [review]
Add CreateOffscreenDrawTarget

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

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +4425,4 @@
>    data.mDrawTarget = mTarget;
> +  } else {
> +    surf = (gfxASurface*)mTarget->GetNativeSurface(NATIVE_SURFACE_GFXASURFACE);
> +    data.mSurface = surf;

I don't think for Non-D2D backends we want to be sending them as Cairo-surfaces.

::: gfx/thebes/gfxPlatform.cpp
@@ +499,5 @@
>  
>  already_AddRefed<gfxASurface>
>  gfxPlatform::GetThebesSurfaceForDrawTarget(DrawTarget *aTarget)
>  {
> +  nsRefPtr<gfxASurface> surf = (gfxASurface*)aTarget->GetNativeSurface(NATIVE_SURFACE_GFXASURFACE);

I'm not sure if we actually want to pollute Azure with gfxASurface stuff.

::: gfx/thebes/gfxPlatformMac.cpp
@@ +141,5 @@
> +    
> +RefPtr<DrawTarget>
> +gfxPlatformMac::CreateOffscreenDrawTarget(const IntSize& aSize, SurfaceFormat aFormat)
> +{
> +  return NULL;

Don't need this if it does the same as the superclass implementation :).

Comment 5

[Bas Schouten (:bas.schouten)]

(In reply to Bas Schouten (:bas) from comment #4)
> Comment on attachment 570583 [details] [diff] [review] [diff] [details] [review]
> Add CreateOffscreenDrawTarget
> 
> Review of attachment 570583 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
> @@ +4425,4 @@
> >    data.mDrawTarget = mTarget;
> > +  } else {
> > +    surf = (gfxASurface*)mTarget->GetNativeSurface(NATIVE_SURFACE_GFXASURFACE);
> > +    data.mSurface = surf;
> 
> I don't think for Non-D2D backends we want to be sending them as
> Cairo-surfaces.
> 
> ::: gfx/thebes/gfxPlatform.cpp
> @@ +499,5 @@
> >  
> >  already_AddRefed<gfxASurface>
> >  gfxPlatform::GetThebesSurfaceForDrawTarget(DrawTarget *aTarget)
> >  {
> > +  nsRefPtr<gfxASurface> surf = (gfxASurface*)aTarget->GetNativeSurface(NATIVE_SURFACE_GFXASURFACE);
> 
> I'm not sure if we actually want to pollute Azure with gfxASurface stuff.

What I mean here is the conversion should be done in our gfxPlatform methods, outside of Azure. Eventually we want to be able to toss out gfxASurface and all, might as well start early.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Make nsCanvasRenderingContext2D support Azure backends other than Direct2D

Comment 7

[Bas Schouten (:bas.schouten)]

Comment on attachment 570899 [details] [diff] [review]
Make nsCanvasRenderingContext2D support Azure backends other than Direct2D

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

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +4425,5 @@
>    data.mDrawTarget = mTarget;
> +  } else {
> +    surf = gfxPlatform::GetPlatform()->GetThebesSurfaceForDrawTarget(mTarget);
> +    data.mSurface = surf;
> +  }

What about my previous comments?

::: gfx/thebes/gfxPlatform.cpp
@@ +511,5 @@
> +  gfxASurface::gfxImageFormat format = gfxASurface::FormatFromContent(ContentForFormat(data->GetFormat()));
> +  
> +  nsRefPtr<gfxImageSurface> image =
> +    new gfxImageSurface(data->GetData(), gfxIntSize(size.width, size.height),
> +                        data->Stride(), format);

Looks much better to me! This surface will not 'live update' but be a snapshot though, unlike the D2D version, we'll probably want to make some kind of decision there soon.

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to Bas Schouten (:bas) from comment #7)
> Comment on attachment 570899 [details] [diff] [review] [diff] [details] [review]
> Make nsCanvasRenderingContext2D support Azure backends other than Direct2D
> 
> Review of attachment 570899 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
> @@ +4425,5 @@
> >    data.mDrawTarget = mTarget;
> > +  } else {
> > +    surf = gfxPlatform::GetPlatform()->GetThebesSurfaceForDrawTarget(mTarget);
> > +    data.mSurface = surf;
> > +  }
> 
> What about my previous comments?

Oh right, I wasn't sure what to do here.

It seemed easier to just do this here, rather than pass the DrawTarget and have multiple code paths in all of the layer managers. Wouldn't it be better to wait until we can have just the DrawTarget code there, and switch at that point?

Comment 9

[Matt Woodrow (:mattwoodrow)]

> ::: gfx/thebes/gfxPlatformMac.cpp
> @@ +141,5 @@
> > +    
> > +RefPtr<DrawTarget>
> > +gfxPlatformMac::CreateOffscreenDrawTarget(const IntSize& aSize, SurfaceFormat aFormat)
> > +{
> > +  return NULL;
> 
> Don't need this if it does the same as the superclass implementation :).

And yes, I know, but the following patch (add Skia backend) implements this properly, so I just left it in.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Make nsCanvasRenderingContext2D support Azure backends other than Direct2D

Comment 11

[Bas Schouten (:bas.schouten)]

Comment on attachment 570927 [details] [diff] [review]
Make nsCanvasRenderingContext2D support Azure backends other than Direct2D

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

::: gfx/layers/opengl/CanvasLayerOGL.cpp
@@ +188,5 @@
>      }
>    } else {
>      nsRefPtr<gfxASurface> updatedAreaSurface;
> +    if (mDrawTarget) {
> +      updatedAreaSurface = gfxPlatform::GetPlatform()->GetThebesSurfaceForDrawTarget(mDrawTarget);

As per IRC discussion, I'd add in a comment this is a 'suboptimal' but easy way of doing it, but that it should be improved.

Comment 12

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf97283ed45f

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/cf97283ed45f