Last Comment Bug 688342 - [Azure] Let nsCanvasRenderingContext2DAzure use backends other than D2D
: [Azure] Let nsCanvasRenderingContext2DAzure use backends other than D2D
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: Matt Woodrow (:mattwoodrow)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 730885
Blocks: 687187
  Show dependency treegraph
 
Reported: 2011-09-21 17:32 PDT by Matt Woodrow (:mattwoodrow)
Modified: 2012-02-27 22:30 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add BACKEND_DEFAULT and use it to automatically decide the skia backend to use (8.03 KB, patch)
2011-09-21 17:32 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Add CreateOffscreenDrawTarget (11.44 KB, patch)
2011-10-30 17:30 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Make nsCanvasRenderingContext2D support Azure backends other than Direct2D (11.97 KB, patch)
2011-10-31 18:36 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Make nsCanvasRenderingContext2D support Azure backends other than Direct2D (14.66 KB, patch)
2011-10-31 21:35 PDT, Matt Woodrow (:mattwoodrow)
bas: review+
Details | Diff | Splinter Review

Description Matt Woodrow (:mattwoodrow) 2011-09-21 17:32:13 PDT
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.
Comment 1 Matt Woodrow (:mattwoodrow) 2011-10-30 13:58:17 PDT
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) 2011-10-30 14:05:16 PDT
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) 2011-10-30 17:30:01 PDT
Created attachment 570583 [details] [diff] [review]
Add CreateOffscreenDrawTarget
Comment 4 Bas Schouten (:bas.schouten) 2011-10-30 18:22:41 PDT
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) 2011-10-30 18:23:44 PDT
(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) 2011-10-31 18:36:53 PDT
Created attachment 570899 [details] [diff] [review]
Make nsCanvasRenderingContext2D support Azure backends other than Direct2D
Comment 7 Bas Schouten (:bas.schouten) 2011-10-31 20:25:18 PDT
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) 2011-10-31 20:41:51 PDT
(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) 2011-10-31 20:43:27 PDT
> ::: 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) 2011-10-31 21:35:07 PDT
Created attachment 570927 [details] [diff] [review]
Make nsCanvasRenderingContext2D support Azure backends other than Direct2D
Comment 11 Bas Schouten (:bas.schouten) 2011-10-31 23:30:19 PDT
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) 2011-11-02 12:56:01 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf97283ed45f
Comment 13 Marco Bonardo [::mak] 2011-11-03 08:44:07 PDT
https://hg.mozilla.org/mozilla-central/rev/cf97283ed45f

Note You need to log in before you can comment on or make changes to this bug.