[Azure] Let nsCanvasRenderingContext2DAzure use backends other than D2D

RESOLVED FIXED in mozilla10

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

unspecified
mozilla10
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 561627 [details] [diff] [review]
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.
Attachment #561627 - Flags: review?(bas.schouten)
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 3

6 years ago
Created attachment 570583 [details] [diff] [review]
Add CreateOffscreenDrawTarget
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.
(Assignee)

Comment 6

6 years ago
Created attachment 570899 [details] [diff] [review]
Make nsCanvasRenderingContext2D support Azure backends other than Direct2D
Attachment #570899 - Flags: review?(bas.schouten)
(Assignee)

Updated

6 years ago
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.
(Assignee)

Comment 8

6 years ago
(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?
(Assignee)

Comment 9

6 years ago
> ::: 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.
(Assignee)

Comment 10

6 years ago
Created attachment 570927 [details] [diff] [review]
Make nsCanvasRenderingContext2D support Azure backends other than Direct2D
(Assignee)

Updated

6 years ago
Attachment #570899 - Attachment is obsolete: true
Attachment #570899 - Flags: review?(bas.schouten)
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 12

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Whiteboard: [inbound]

Updated

6 years ago
Depends on: 730885
You need to log in before you can comment on or make changes to this bug.