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 User image 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 User image 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 User image 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 User image Matt Woodrow (:mattwoodrow) 2011-10-30 17:30:01 PDT
Created attachment 570583 [details] [diff] [review]
Add CreateOffscreenDrawTarget
Comment 4 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Matt Woodrow (:mattwoodrow) 2011-11-02 12:56:01 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf97283ed45f
Comment 13 User image 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.