Closed Bug 688342 Opened 9 years ago Closed 9 years ago

[Azure] Let nsCanvasRenderingContext2DAzure use backends other than D2D

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Attachment #561627 - Flags: review?(bas.schouten)
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?
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.
Attached patch Add CreateOffscreenDrawTarget (obsolete) — Splinter Review
Attachment #561627 - Attachment is obsolete: true
Attachment #561627 - Flags: review?(bas.schouten)
Attachment #570583 - Flags: review?(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 :).
(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.
Attachment #570583 - Attachment is obsolete: true
Attachment #570583 - Flags: review?(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.
(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?
> ::: 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.
Attachment #570899 - Attachment is obsolete: true
Attachment #570899 - Flags: review?(bas.schouten)
Attachment #570927 - Flags: review?(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.
Attachment #570927 - Flags: review?(bas.schouten) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf97283ed45f
Assignee: nobody → matt.woodrow
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/cf97283ed45f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Whiteboard: [inbound]
Depends on: 730885
You need to log in before you can comment on or make changes to this bug.