Last Comment Bug 764125 - [Azure] Get Azure/Cairo canvas passing tests
: [Azure] Get Azure/Cairo canvas passing tests
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: 15 Branch
: All All
: -- normal with 1 vote (vote)
: mozilla17
Assigned To: Nick Cameron [:nrc]
:
Mentors:
Depends on: 981391 761895 778195 780392 921132
Blocks: 622842 694351 775063 775145 773460 776685 776802
  Show dependency treegraph
 
Reported: 2012-06-12 13:14 PDT by Nick Cameron [:nrc]
Modified: 2014-03-10 09:34 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fixes for 2d.toDataURL.* (1.03 KB, patch)
2012-06-12 13:25 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
no flags Details | Diff | Review
Implemented DrawTargetCairo::CopySurface to fix 2d.toDataURL.* (2.09 KB, patch)
2012-06-12 13:34 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
no flags Details | Diff | Review
Implemented DrawTargetCairo::CopySurface to fix 2d.toDataURL.* (1.93 KB, patch)
2012-06-13 22:09 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
no flags Details | Diff | Review
Fixed surface edges on Linux to make 2d.drawImage.5arg pass (969 bytes, patch)
2012-06-13 22:10 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
no flags Details | Diff | Review
Possible logic error affecting Cairo win32 (950 bytes, patch)
2012-06-13 22:14 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
bas: review+
Details | Diff | Review
patch 1: Add DrawTarget arg to GetScaledFontForFont (12.70 KB, patch)
2012-06-24 19:48 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
patch 2: small fix (1.24 KB, patch)
2012-06-24 19:49 PDT, Nick Cameron [:nrc]
bas: review+
Details | Diff | Review
patch 1: Add DrawTarget arg to GetScaledFontForFont (11.67 KB, patch)
2012-06-24 19:50 PDT, Nick Cameron [:nrc]
bas: review+
Details | Diff | Review
patch 3: pass around the size of a gfxASurface when creating a DrawTarget (7.10 KB, patch)
2012-06-24 19:51 PDT, Nick Cameron [:nrc]
bas: review+
Details | Diff | Review
patch 4: store a reference to the backing surface in DrawTargetCairo (2.23 KB, patch)
2012-06-24 19:52 PDT, Nick Cameron [:nrc]
bas: review+
Details | Diff | Review
patch 5: drawing changes to DrawTargetCairo (12.21 KB, patch)
2012-06-24 19:53 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
patch 6: Fix a bug where a path with a transform is re-used (2.87 KB, patch)
2012-06-24 19:54 PDT, Nick Cameron [:nrc]
bas: review+
Details | Diff | Review
patch 7: changes to nsCanvasRenderingContext2DAzure (12.02 KB, patch)
2012-06-24 19:57 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
patch 1.5: implement DrawTargetCairo::CopySurface (1.99 KB, patch)
2012-06-26 23:06 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
bas: review-
Details | Diff | Review
patch 8: Fix canvas 2d.drawImage.5arg and 2d.drawImage.9arg surface scaling tests (2.41 KB, patch)
2012-06-26 23:12 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
karlt: review+
Details | Diff | Review
patch 9: fixed crash test fail on gtk caused by null (1011 bytes, patch)
2012-06-26 23:21 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
bas: review+
Details | Diff | Review
patch 10: Fixed build break for qt (2.81 KB, patch)
2012-06-26 23:28 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
no flags Details | Diff | Review
patch 1.5: implement DrawTargetCairo::CopySurface [v2] (2.01 KB, patch)
2012-07-03 14:20 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
jmuizelaar: review-
Details | Diff | Review
patch 9.5 test tweaks (2.95 KB, patch)
2012-07-04 16:30 PDT, Nick Cameron [:nrc]
jmuizelaar: review+
Details | Diff | Review
patch 5: drawing changes to DrawTargetCairo (12.71 KB, patch)
2012-07-04 16:33 PDT, Nick Cameron [:nrc]
jmuizelaar: review+
Details | Diff | Review
patch 7: changes to nsCanvasRenderingContext2DAzure (13.73 KB, patch)
2012-07-08 14:20 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
patch 1.5: implement DrawTargetCairo::CopySurface [v3] (1.98 KB, patch)
2012-07-10 16:19 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
jmuizelaar: review+
Details | Diff | Review
patch 9.5 test tweaks (3.45 KB, patch)
2012-07-16 03:59 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Review
patch 9.5 test tweaks (4.32 KB, patch)
2012-07-16 04:30 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Review
patch 9.5 test tweaks (4.33 KB, patch)
2012-07-18 05:29 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Review
patch 5: drawing changes to DrawTargetCairo (13.58 KB, patch)
2012-07-18 05:52 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Review
patch 7: changes to nsCanvasRenderingContext2DAzure (8.78 KB, patch)
2012-07-18 06:27 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
patch 7: changes to nsCanvasRenderingContext2DAzure (6.18 KB, patch)
2012-07-22 14:44 PDT, Nick Cameron [:nrc]
bas: review+
Details | Diff | Review
patch 10: mochitest for isPointInPath and multiple transforms (2.29 KB, patch)
2012-07-22 14:46 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
patch 10: mochitest for isPointInPath and multiple transforms (2.17 KB, patch)
2012-07-22 21:45 PDT, Nick Cameron [:nrc]
bas: review+
Details | Diff | Review
patch 11: enabling Azure Canvas pt 1 (3.22 KB, patch)
2012-07-22 21:46 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
patch 12: enable Azure canvas pt 2 (18.81 KB, patch)
2012-07-22 21:48 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
patch 13: enable Azure canvas pt 3 (9.87 KB, patch)
2012-07-22 21:49 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
patch 11: enabling Azure Canvas pt 1 (3.22 KB, patch)
2012-07-22 21:53 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
patch 12: enable Azure canvas pt 2 (18.81 KB, patch)
2012-07-22 21:54 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
patch 11: enabling Azure Canvas pt 1 (3.21 KB, patch)
2012-07-23 22:18 PDT, Nick Cameron [:nrc]
bas: review+
Details | Diff | Review
patch 12: enable Azure canvas pt 2 (19.91 KB, patch)
2012-07-23 22:18 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
patch 13: enable Azure canvas pt 3 (9.82 KB, patch)
2012-07-23 22:19 PDT, Nick Cameron [:nrc]
bas: review+
Details | Diff | Review
patch 12: enable Azure canvas pt 2 (20.97 KB, patch)
2012-07-24 20:14 PDT, Nick Cameron [:nrc]
bas: review+
Details | Diff | Review
patch 14: remove AzureEnabled from gfxInfo (14.17 KB, patch)
2012-07-25 21:01 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
patch 14: remove AzureEnabled from gfxInfo (14.14 KB, patch)
2012-07-25 21:13 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Review
patch 14: remove AzureEnabled from gfxInfo (15.63 KB, patch)
2012-07-25 22:13 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Review

Description Nick Cameron [:nrc] 2012-06-12 13:14:32 PDT

    
Comment 1 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-06-12 13:25:11 PDT
Created attachment 632382 [details] [diff] [review]
Fixes for 2d.toDataURL.*

The attached patch should fix the 2d.toDataURL.* by implementing DrawTargetCairo::CopySurface()
Comment 2 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-06-12 13:34:16 PDT
Created attachment 632389 [details] [diff] [review]
Implemented DrawTargetCairo::CopySurface to fix 2d.toDataURL.*

Two similarly named patches. Let's try for the right one this time.
Comment 3 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-06-13 22:09:31 PDT
Created attachment 633021 [details] [diff] [review]
Implemented DrawTargetCairo::CopySurface to fix 2d.toDataURL.*

Updated patch.
Comment 4 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-06-13 22:10:50 PDT
Created attachment 633022 [details] [diff] [review]
Fixed surface edges on Linux to make 2d.drawImage.5arg pass
Comment 5 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-06-13 22:14:00 PDT
Created attachment 633023 [details] [diff] [review]
Possible logic error affecting Cairo win32
Comment 6 Nick Cameron [:nrc] 2012-06-24 19:48:47 PDT
Created attachment 636212 [details] [diff] [review]
patch 1: Add DrawTarget arg to GetScaledFontForFont
Comment 7 Nick Cameron [:nrc] 2012-06-24 19:49:44 PDT
Created attachment 636213 [details] [diff] [review]
patch 2: small fix
Comment 8 Nick Cameron [:nrc] 2012-06-24 19:50:30 PDT
Created attachment 636214 [details] [diff] [review]
patch 1: Add DrawTarget arg to GetScaledFontForFont
Comment 9 Nick Cameron [:nrc] 2012-06-24 19:51:33 PDT
Created attachment 636215 [details] [diff] [review]
patch 3: pass around the size of a gfxASurface when creating a DrawTarget
Comment 10 Nick Cameron [:nrc] 2012-06-24 19:52:35 PDT
Created attachment 636216 [details] [diff] [review]
patch 4: store a reference to the backing surface in DrawTargetCairo
Comment 11 Nick Cameron [:nrc] 2012-06-24 19:53:28 PDT
Created attachment 636217 [details] [diff] [review]
patch 5: drawing changes to DrawTargetCairo
Comment 12 Nick Cameron [:nrc] 2012-06-24 19:54:30 PDT
Created attachment 636218 [details] [diff] [review]
patch 6: Fix a bug where a path with a transform is re-used
Comment 13 Nick Cameron [:nrc] 2012-06-24 19:57:20 PDT
Created attachment 636219 [details] [diff] [review]
patch 7: changes to nsCanvasRenderingContext2DAzure
Comment 14 Bas Schouten (:bas.schouten) 2012-06-25 01:40:55 PDT
Comment on attachment 636215 [details] [diff] [review]
patch 3: pass around the size of a gfxASurface when creating a DrawTarget

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

How will this work for Cairo surfaces that don't have a finite size? (i.e. PDF and such)
Comment 15 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-06-26 23:06:14 PDT
Created attachment 637004 [details] [diff] [review]
patch 1.5: implement DrawTargetCairo::CopySurface
Comment 16 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-06-26 23:12:47 PDT
Created attachment 637006 [details] [diff] [review]
patch 8: Fix canvas 2d.drawImage.5arg and 2d.drawImage.9arg surface scaling tests
Comment 17 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-06-26 23:21:24 PDT
Created attachment 637007 [details] [diff] [review]
patch 9: fixed crash test fail on gtk caused by null
Comment 18 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-06-26 23:28:12 PDT
Created attachment 637009 [details] [diff] [review]
patch 10: Fixed build break for qt
Comment 19 Karl Tomlinson (ni?:karlt) 2012-06-26 23:37:55 PDT
Comment on attachment 637006 [details] [diff] [review]
patch 8: Fix canvas 2d.drawImage.5arg and 2d.drawImage.9arg surface scaling tests

> Bug 764125; Fix canvas 2d.drawImage.5arg and 2d.drawImage.9arg surface scaling tests. r=karl

The comment should describe how the patch changes behavior.
e.g. "switch from EXTEND_NONE to EXTEND_PAD for drawing surfaces to avoid sampling beyond the surface"
Comment 20 Bas Schouten (:bas.schouten) 2012-06-29 05:49:22 PDT
Comment on attachment 637004 [details] [diff] [review]
patch 1.5: implement DrawTargetCairo::CopySurface

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

::: gfx/2d/DrawTargetCairo.cpp
@@ +473,5 @@
>    AutoPrepareForDrawing prep(this, mContext);
> +
> +  cairo_matrix_t src_mat;
> +  cairo_matrix_init_scale(&src_mat, 1, 1);
> +  cairo_matrix_translate(&src_mat, aSource.X(), aSource.Y());

nit: It's easier to use the members directly rather than the getters (i.e. aSource.x, aSource.y)

@@ +480,5 @@
> +  if (aSurface->GetType() == SURFACE_CAIRO) {
> +    surf = static_cast<SourceSurfaceCairo*>(aSurface)->GetSurface();
> +  }
> +
> +  cairo_pattern_t* pat = cairo_pattern_create_for_surface(surf);

What will this do with a NULL surface? Should we have an else { gfxWarning() above?

@@ +482,5 @@
> +  }
> +
> +  cairo_pattern_t* pat = cairo_pattern_create_for_surface(surf);
> +  cairo_pattern_set_matrix(pat, &src_mat);
> +  cairo_pattern_set_filter(pat, CAIRO_FILTER_BILINEAR);

We can use point filtering as there's never any resizing going on here. I also believe cairo has a set_source_surface that can directly set a source surface with an offset. That should make this function a lot simpler.

What you'd do is totally ignore matrices or anything (except set the identity matrix on the context), i.e. as far as I can see something like this would work (peudo-code):

cairo_save(mContext);

cairo_identity_matrix(mContext);

cairo_set_source_surface(mContext, surf, aDest - aSource);
cairo_set_operator(mContext, SOURCE);

cairo_reset_clip(mContext);
cairo_new_path(mContext);
cairo_rectangle(aDest.x, aDest.y, aSource.width, aSource.height);
cairo_clip(mContext);

cairo_paint(mContext);

cairo_restore(mContext);

@@ +498,5 @@
> +  cairo_clip(mContext);
> +  cairo_set_source(mContext, pat);
> +
> +  cairo_set_operator(mContext, CAIRO_OPERATOR_SOURCE);
> +  cairo_paint_with_alpha(mContext, 1.0F);

We should just be able to use cairo_paint();
Comment 21 Bas Schouten (:bas.schouten) 2012-06-29 05:50:12 PDT
Comment on attachment 637007 [details] [diff] [review]
patch 9: fixed crash test fail on gtk caused by null

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

::: gfx/thebes/gfxPlatform.cpp
@@ +633,5 @@
>    if (backend == BACKEND_CAIRO) {
>      nsRefPtr<gfxASurface> surf = CreateOffscreenSurface(ThebesIntSize(aSize),
>                                                          ContentForFormat(aFormat));
> +    if (!surf)
> +      return NULL;

nit: { } also for single line ifs.
Comment 22 Bas Schouten (:bas.schouten) 2012-06-29 05:55:17 PDT
Comment on attachment 636216 [details] [diff] [review]
patch 4: store a reference to the backing surface in DrawTargetCairo

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

::: gfx/2d/SourceSurfaceCairo.cpp
@@ +105,5 @@
>      cairo_destroy(ctx);
>  
>      // Swap in this new surface.
>      cairo_surface_destroy(mSurface);
> +    cairo_pattern_destroy(pat);

This is actually fixing a leak as far as I can see. It should technically be in a separate patch.
Comment 23 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-07-03 14:20:16 PDT
Created attachment 638865 [details] [diff] [review]
patch 1.5: implement DrawTargetCairo::CopySurface [v2]

I've made the suggested changes in comment 20 and now the patch looks suspiciously like the given pseudo-code.
Comment 24 Bas Schouten (:bas.schouten) 2012-07-03 14:34:10 PDT
(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #23)
> Created attachment 638865 [details] [diff] [review]
> patch 1.5: implement DrawTargetCairo::CopySurface [v2]
> 
> I've made the suggested changes in comment 20 and now the patch looks
> suspiciously like the given pseudo-code.

I assume you checked if it worked? ;)
Comment 25 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-07-03 15:40:31 PDT
(In reply to Bas Schouten (:bas) from comment #24)
> I assume you checked if it worked? ;)

Your assumption is correct.
Comment 26 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-07-03 18:39:43 PDT
Comment on attachment 633023 [details] [diff] [review]
Possible logic error affecting Cairo win32

I haven't been able to figure out the problem fixed by this patch (possible logic error...). It seems it will not fail gracefully if it hits a later error condition. Anyway the logic appears inverted.
Comment 27 Nick Cameron [:nrc] 2012-07-04 16:30:28 PDT
Created attachment 639196 [details] [diff] [review]
patch 9.5 test tweaks
Comment 28 Nick Cameron [:nrc] 2012-07-04 16:33:42 PDT
Created attachment 639197 [details] [diff] [review]
patch 5: drawing changes to DrawTargetCairo
Comment 29 Nick Cameron [:nrc] 2012-07-04 17:58:40 PDT
Comment on attachment 639196 [details] [diff] [review]
patch 9.5 test tweaks

Looks like Joe is away, so I've asked Bas for reviews on these two patches too, not sure if there is a better person for the Canvas stuff, but at least Bas has seen the other patches.
Comment 30 Bas Schouten (:bas.schouten) 2012-07-05 04:27:53 PDT
Comment on attachment 636219 [details] [diff] [review]
patch 7: changes to nsCanvasRenderingContext2DAzure

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

Please explain why we need the mNewPath stuff, it's not clear to me.
Comment 31 Nick Cameron [:nrc] 2012-07-08 14:17:05 PDT
(In reply to Bas Schouten (:bas) from comment #30)
> Comment on attachment 636219 [details] [diff] [review]
> patch 7: changes to nsCanvasRenderingContext2DAzure
> 
> Review of attachment 636219 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please explain why we need the mNewPath stuff, it's not clear to me.

It was to meet the spec for quadratic curves (tests: http://philip.html5.org/tests/canvas/suite/tests/index.2d.path.quadraticCurveTo.ensuresubpath.html) - if there is no existing path then the first control point is used as the start of the path.
Comment 32 Nick Cameron [:nrc] 2012-07-08 14:20:26 PDT
Created attachment 640100 [details] [diff] [review]
patch 7: changes to nsCanvasRenderingContext2DAzure

rebased and fixed a bug
Comment 33 George Wright (:gw280) (:gwright) 2012-07-09 15:38:49 PDT
Comment on attachment 637009 [details] [diff] [review]
patch 10: Fixed build break for qt

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

jfkthame is better suited to review this I think.

::: gfx/thebes/gfxQtPlatform.cpp
@@ +514,3 @@
>      return gPlatformFTLibrary;
> +#else
> +    return nsnull;

Jonathan Kew wasn't too fond of this. Maybe we can NS_ASSERTION that we're not using Pango here?

@@ +518,3 @@
>  }
>  
> +#ifndef MOZ_PANGO

Where is the matching endif for this? Why is this necessary?
Comment 34 Jeff Muizelaar [:jrmuizel] 2012-07-10 12:42:48 PDT
Comment on attachment 638865 [details] [diff] [review]
patch 1.5: implement DrawTargetCairo::CopySurface [v2]

It seems like this should use cairo_fill instead of clip and paint. Otherwise this looks ok.
Comment 35 Jeff Muizelaar [:jrmuizel] 2012-07-10 12:46:15 PDT
(In reply to Nick Cameron [:nrc] from comment #31)
> (In reply to Bas Schouten (:bas) from comment #30)
> > Comment on attachment 636219 [details] [diff] [review]
> > patch 7: changes to nsCanvasRenderingContext2DAzure
> > 
> > Review of attachment 636219 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Please explain why we need the mNewPath stuff, it's not clear to me.
> 
> It was to meet the spec for quadratic curves (tests:
> http://philip.html5.org/tests/canvas/suite/tests/index.2d.path.
> quadraticCurveTo.ensuresubpath.html) - if there is no existing path then the
> first control point is used as the start of the path.

Does this affect other backends then? If so it would probably be better in another bug.
Comment 36 Jeff Muizelaar [:jrmuizel] 2012-07-10 12:52:40 PDT
Comment on attachment 636215 [details] [diff] [review]
patch 3: pass around the size of a gfxASurface when creating a DrawTarget

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

This needs Bas's comment about cairo surfaces not having sizes addressed
Comment 37 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-07-10 16:19:12 PDT
Created attachment 640848 [details] [diff] [review]
patch 1.5: implement DrawTargetCairo::CopySurface [v3]

cairo_fill instead of cairo_clip and cairo_paint
Comment 38 Jeff Muizelaar [:jrmuizel] 2012-07-11 05:42:02 PDT
Comment on attachment 639196 [details] [diff] [review]
patch 9.5 test tweaks

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

Do we understand the cause of these regressions? i.e. why it worked before with cairo but not with azure? The comment near the test talks about cairo limitations but isn't clear about what limitations and why we hit them now and not before.
Comment 39 Nick Cameron [:nrc] 2012-07-11 15:06:23 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #35)
> (In reply to Nick Cameron [:nrc] from comment #31)
> > (In reply to Bas Schouten (:bas) from comment #30)
> > > Comment on attachment 636219 [details] [diff] [review]
> > > patch 7: changes to nsCanvasRenderingContext2DAzure
> > > 
> > > Review of attachment 636219 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Please explain why we need the mNewPath stuff, it's not clear to me.
> > 
> > It was to meet the spec for quadratic curves (tests:
> > http://philip.html5.org/tests/canvas/suite/tests/index.2d.path.
> > quadraticCurveTo.ensuresubpath.html) - if there is no existing path then the
> > first control point is used as the start of the path.
> 
> Does this affect other backends then? If so it would probably be better in
> another bug.

It does not seem to affect D2D, I do not know why, presumably because it is taken into account in the DrawTarget somehow. I don't know about other backends. It might be possible to move the logiv into the DrawTarget, but that seemed much more difficult. I believe this test fails for Thebes/Cairo canvas anyway, so might not be necessary to pass immediately.
Comment 40 Nick Cameron [:nrc] 2012-07-11 15:09:40 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #36)
> Comment on attachment 636215 [details] [diff] [review]
> patch 3: pass around the size of a gfxASurface when creating a DrawTarget
> 
> Review of attachment 636215 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This needs Bas's comment about cairo surfaces not having sizes addressed

We discussed this on IRC, and iirc, we are not able to deal with size-less surfaces at the moment and it there is no easy way to sort this, so it didn't need addressing, but Bas was going to have a think about this before confirming.
Comment 41 Nick Cameron [:nrc] 2012-07-11 15:13:48 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #38)
> Comment on attachment 639196 [details] [diff] [review]
> patch 9.5 test tweaks
> 
> Review of attachment 639196 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we understand the cause of these regressions? i.e. why it worked before
> with cairo but not with azure? The comment near the test talks about cairo
> limitations but isn't clear about what limitations and why we hit them now
> and not before.

The limitation is that Cairo draws arcs by stroking perpendicular to the arc, and at very large stroke thicknesses, this becomes a fan. Where exactly the 'blades' of the fan appear seems to depend on exactly how the arc is defined and the platform. So if the blades of the fan are where pixels are tested it passes the test, if the testing pixels fall in between the blades, then we fail. Before, we were rendering wrong, but got lucky with the test, now we are not so lucky.
Comment 42 Jeff Muizelaar [:jrmuizel] 2012-07-12 15:13:52 PDT
Comment on attachment 639197 [details] [diff] [review]
patch 5: drawing changes to DrawTargetCairo

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

::: gfx/2d/DrawTargetCairo.cpp
@@ +220,5 @@
>    return pat;
>  }
>  
>  static bool
> +OperatorAffectsUncoveredAreas(CompositionOp op)

Please copy the comment from nsCanvasRenderingContext2D.cpp as well

@@ +327,5 @@
> +      cairo_new_path(mContext);
> +      cairo_rectangle(mContext, 0, 0, aDest.Width(), aDest.Height());
> +      cairo_clip(mContext);
> +      cairo_set_source(mContext, pat);
> +      cairo_paint(mContext);

We should be able to use fill() instead of clip() and paint()

@@ +418,5 @@
> +      cairo_new_path(mContext);
> +      cairo_rectangle(mContext, 0, 0, width, height);
> +      cairo_clip(mContext);
> +      cairo_set_source(mContext, pat);
> +      cairo_paint(mContext);

Same here. fill() instead of clip() and paint()

@@ +727,5 @@
> +  unsigned char* surfData = cairo_image_surface_get_data(surf);
> +  const int32_t pixelWidth = BytesPerPixel(aFormat);
> +  for (size_t y = 0; y < aSize.height; ++y) {
> +    memcpy(surfData + y*aSize.width*pixelWidth, aData + y*aStride, aSize.width*pixelWidth);
> +  }

I think it would probably be helpful to add a helper method that does this strided copy. We could use it the CoreGraphics backend too.
Comment 43 Jeff Muizelaar [:jrmuizel] 2012-07-12 15:22:34 PDT
Comment on attachment 639196 [details] [diff] [review]
patch 9.5 test tweaks

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

It would be nice to know more about bug 764108 before pushing this, but I guess it doesn't need to block.

::: content/canvas/test/test_canvas.html
@@ +11309,5 @@
>  isPixel(ctx, 1,48, 0,255,0,255, 0);
> +// Fails on Linux with Azure/Cairo only
> +// The arc is drawn badly due to Cairo limitations, the error only becomes
> +// apparent on Linux because of anti-aliasing, probably due to X
> +// Bug 764125

Move the description you gave into this comment to make it more useful. Also we should only disable the test when using Azure/Cairo
Comment 44 Nick Cameron [:nrc] 2012-07-16 03:59:26 PDT
Created attachment 642528 [details] [diff] [review]
patch 9.5 test tweaks

Added comment, carrying r=jrmuizel
Comment 45 Nick Cameron [:nrc] 2012-07-16 04:01:28 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #43)

> Move the description you gave into this comment to make it more useful. Also
> we should only disable the test when using Azure/Cairo

I added the comment, but the test only fails on Linux, it passes on other platforms. We do not have the facility for to mark a test passing on specific platforms, the policy elsewhere in the file seems to be to comment out tests which are platform-specific pass/fail.
Comment 46 :Ms2ger 2012-07-16 04:08:57 PDT
(In reply to Nick Cameron [:nrc] from comment #45)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #43)
> 
> > Move the description you gave into this comment to make it more useful. Also
> > we should only disable the test when using Azure/Cairo
> 
> I added the comment, but the test only fails on Linux, it passes on other
> platforms. We do not have the facility for to mark a test passing on
> specific platforms, the policy elsewhere in the file seems to be to comment
> out tests which are platform-specific pass/fail.

You can, and you should. I object to commenting out any test.
Comment 47 Nick Cameron [:nrc] 2012-07-16 04:30:32 PDT
Created attachment 642533 [details] [diff] [review]
patch 9.5 test tweaks

with a conditional test, rather than commenting it out
Comment 48 Nick Cameron [:nrc] 2012-07-16 04:35:13 PDT
(In reply to :Ms2ger from comment #46)
> (In reply to Nick Cameron [:nrc] from comment #45)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #43)
> > 
> > > Move the description you gave into this comment to make it more useful. Also
> > > we should only disable the test when using Azure/Cairo
> > 
> > I added the comment, but the test only fails on Linux, it passes on other
> > platforms. We do not have the facility for to mark a test passing on
> > specific platforms, the policy elsewhere in the file seems to be to comment
> > out tests which are platform-specific pass/fail.
> 
> You can, and you should. I object to commenting out any test.

OK, we can (very small bit of extra code, it turns out), but in this case I think we probably shouldn't, because whether we pass or not is basically non-deterministic, and we have established that we don't care (at the moment) whether we pass or not. I believe that is expressed by commenting out the test (we don't delete it because we might change our mind in the future). Making the test conditional says that it passes on one platform and fails on another, and we expect it to remain that way. It is easy to imagine some change somewhere which changes this situation and then a conditional test would be confusing.

Anyway, I have put up alternative versions of the patch, I can land whichever.
Comment 49 Jonathan Kew (:jfkthame) 2012-07-17 07:19:47 PDT
Comment on attachment 637009 [details] [diff] [review]
patch 10: Fixed build break for qt

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

I'd like to know what makes this appear necessary; is there something in one of the other patches here that's requiring it?

::: gfx/thebes/gfxQtPlatform.cpp
@@ +518,1 @@
>  }

I'm not clear why we need to do this - what purpose does it serve (in the MOZ_PANGO case) if it's just going to return null?

If we do have to provide this as a stub in the MOZ_PANGO case, we should assert NS_NOTREACHED because it shouldn't ever be called (I presume).
Comment 50 Nick Cameron [:nrc] 2012-07-18 05:29:19 PDT
Created attachment 643330 [details] [diff] [review]
patch 9.5 test tweaks

made one test conditional and added an IsLinux method to do so. Carrying r=jrmuizel
Comment 51 Nick Cameron [:nrc] 2012-07-18 05:52:10 PDT
Created attachment 643342 [details] [diff] [review]
patch 5: drawing changes to DrawTargetCairo

changes as requested; carrying r=jrmuizel
Comment 52 Nick Cameron [:nrc] 2012-07-18 06:27:55 PDT
Created attachment 643351 [details] [diff] [review]
patch 7: changes to nsCanvasRenderingContext2DAzure

removed the mNewPath stuff (to Bug 775063).
Comment 53 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-07-18 15:49:10 PDT
Comment on attachment 637009 [details] [diff] [review]
patch 10: Fixed build break for qt

this patch is no longer needed because the patch causing the original build break is no longer being used.
Comment 54 :Ms2ger 2012-07-21 05:14:29 PDT
So, for exceptions, I think we want different messages for the two places where we call onFailure. The one in wrapObjectTemplate should say something about the value not being an object, and the one next to the xpc_qsUnwrapArg call something about it not implementing descriptor.interface.identifier. (The latter never deals with non-object values.)
Comment 55 Nick Cameron [:nrc] 2012-07-22 14:44:46 PDT
Created attachment 644796 [details] [diff] [review]
patch 7: changes to nsCanvasRenderingContext2DAzure

removed all the bits Bas didn't like and fixed the bits which needed fixing
Comment 56 Nick Cameron [:nrc] 2012-07-22 14:46:42 PDT
Created attachment 644797 [details] [diff] [review]
patch 10: mochitest for isPointInPath and multiple transforms
Comment 57 Nick Cameron [:nrc] 2012-07-22 21:45:42 PDT
Created attachment 644831 [details] [diff] [review]
patch 10: mochitest for isPointInPath and multiple transforms
Comment 58 Nick Cameron [:nrc] 2012-07-22 21:46:53 PDT
Created attachment 644832 [details] [diff] [review]
patch 11: enabling Azure Canvas pt 1
Comment 59 Nick Cameron [:nrc] 2012-07-22 21:48:24 PDT
Created attachment 644833 [details] [diff] [review]
patch 12: enable Azure canvas pt 2

I'm a bit unsure about this one - is using the backend type as a bitmask really nasty? Is there a better way to do this?
Comment 60 Nick Cameron [:nrc] 2012-07-22 21:49:20 PDT
Created attachment 644834 [details] [diff] [review]
patch 13: enable Azure canvas pt 3
Comment 61 Nick Cameron [:nrc] 2012-07-22 21:53:22 PDT
Created attachment 644836 [details] [diff] [review]
patch 11: enabling Azure Canvas pt 1

core -> cg
Comment 62 Nick Cameron [:nrc] 2012-07-22 21:54:07 PDT
Created attachment 644837 [details] [diff] [review]
patch 12: enable Azure canvas pt 2

core -> cg
Comment 63 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-22 22:00:55 PDT
Comment on attachment 644833 [details] [diff] [review]
patch 12: enable Azure canvas pt 2

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

::: gfx/thebes/gfxPlatform.cpp
@@ +730,5 @@
> +  if (strcmp(aName, "skia") == 0)
> +    return BACKEND_SKIA;
> +  if (strcmp(aName, "direct2d") == 0)
> +    return BACKEND_DIRECT2D;
> +  if (strcmp(aName, "core") == 0)

Better to just take const nsCString& and call EqualsLiteral here. char*s are somewhat evil and unsafe so the more we can avoid using them, the better, especially when (like here) the nsCString code is just as simple.

I think "cg" is better then "core", which is a bit ambiguous.

::: gfx/thebes/gfxPlatform.h
@@ +467,5 @@
> +
> +    // returns the first backend named in the pref gfx.canvas.azure.backends
> +    // which is a component of aMask, a bitmask of backend types
> +    static mozilla::gfx::BackendType GetBackendPref(mozilla::gfx::BackendType aMask);
> +    static mozilla::gfx::BackendType BackendTypeForName(const char* aName);

GetBackendPref/BackendTypeForName are not good names for functions which are canvas-specific. Nor are mFallbackDrawTargetBackend and mPreferredDrawTargetBackend good names for data which are canvas-specific.

Either we keep these names, and make the pref cover content as well as canvas, or we parameterize these over "content" and "canvas".
Comment 64 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-22 22:02:39 PDT
What about different kinds of cairo backends? Should we split cairo into cairo-image, cairo-xlib, cairo-gdi, etc?
Comment 65 Nick Cameron [:nrc] 2012-07-22 22:23:45 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #64)
> What about different kinds of cairo backends? Should we split cairo into
> cairo-image, cairo-xlib, cairo-gdi, etc?

I'd be inclined to use a different pref for that, so that the azure.backends pref is one-to-one with the actual Azure backends.

What do we need to be able to switch Cairo surface type for?
Comment 66 Nick Cameron [:nrc] 2012-07-22 22:28:11 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #63)
> Comment on attachment 644833 [details] [diff] [review]
> patch 12: enable Azure canvas pt 2
> 
> Review of attachment 644833 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Either we keep these names, and make the pref cover content as well as
> canvas, or we parameterize these over "content" and "canvas".

Is it worth having different backend preferences for canvas and content? I guess we don't need to fallback for content; do we? I had assumed we'd use the same pref for canvas/content, but maybe we don't want to.
Comment 67 :Ms2ger 2012-07-23 02:14:59 PDT
Comment on attachment 644834 [details] [diff] [review]
patch 13: enable Azure canvas pt 3

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

::: widget/xpwidgets/GfxInfoBase.cpp
@@ +731,5 @@
> +  static bool azureInited = false;
> +
> +  if (!azureInited) {
> +    nsresult rv = mozilla::Preferences::GetBool("gfx.canvas.azure.enabled", &azure);
> +    azure = NS_SUCCEEDED(rv) && azure;

Maybe 'azure = Preferences::GetBool("gfx.canvas.azure.enabled", false);'
Comment 68 :Ms2ger 2012-07-23 02:15:55 PDT
Comment on attachment 644836 [details] [diff] [review]
patch 11: enabling Azure Canvas pt 1

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

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +569,5 @@
>  nsresult
>  NS_NewCanvasRenderingContext2DAzure(nsIDOMCanvasRenderingContext2D** aResult)
>  {
> +  // XXX[nrc] remove this check when Thebes canvas is removed
> +  // (because we will always support Azure)

*shrug*. This entire function will be able to go by then.
Comment 69 :Ms2ger 2012-07-23 03:00:28 PDT
Comment on attachment 644831 [details] [diff] [review]
patch 10: mochitest for isPointInPath and multiple transforms

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

Now also <http://w3c-test.org/html/tests/submission/PhilipTaylor/canvas/2d.path.isPointInPath.transform.4.html>.
Comment 70 Nick Cameron [:nrc] 2012-07-23 22:18:03 PDT
Created attachment 645191 [details] [diff] [review]
patch 11: enabling Azure Canvas pt 1
Comment 71 Nick Cameron [:nrc] 2012-07-23 22:18:40 PDT
Created attachment 645193 [details] [diff] [review]
patch 12: enable Azure canvas pt 2
Comment 72 Nick Cameron [:nrc] 2012-07-23 22:19:50 PDT
Created attachment 645194 [details] [diff] [review]
patch 13: enable Azure canvas pt 3

All three patches updated for comments by roc and Ms2ger (thanks for the comments!)
Comment 73 Bas Schouten (:bas.schouten) 2012-07-24 01:21:43 PDT
Comment on attachment 645193 [details] [diff] [review]
patch 12: enable Azure canvas pt 2

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

::: gfx/layers/d3d10/LayerManagerD3D10.cpp
@@ +488,4 @@
>    if ((aFormat != FORMAT_B8G8R8A8 &&
> +       aFormat != FORMAT_B8G8R8X8) ||
> +       !gfxPlatform::GetPlatform()->SupportsAzure(backend) ||
> +       backend != BACKEND_DIRECT2D) {

This really can't happen with D3D10 layers.

::: gfx/thebes/gfxAndroidPlatform.h
@@ -32,5 @@
>      virtual already_AddRefed<gfxASurface>
>      CreateOffscreenSurface(const gfxIntSize& size,
>                             gfxASurface::gfxContentType contentType);
>      
> -    virtual bool SupportsAzure(mozilla::gfx::BackendType& aBackend) { aBackend = mozilla::gfx::BACKEND_SKIA; return true; }

Why was this removed?

::: gfx/thebes/gfxPlatform.h
@@ +469,5 @@
> +
> +    /**
> +     * initialise preferred and fallback canvas backends
> +     * both backends will be contained in aBackendBitmask, the order is
> +     * determined by the gfx.canvas.azure.backends pref

This comment isn't very clear in my opinion.
Comment 74 Bas Schouten (:bas.schouten) 2012-07-24 01:23:57 PDT
(In reply to Bas Schouten (:bas) from comment #73)
> Comment on attachment 645193 [details] [diff] [review]
> patch 12: enable Azure canvas pt 2
> 
> Review of attachment 645193 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxAndroidPlatform.h
> @@ -32,5 @@
> >      virtual already_AddRefed<gfxASurface>
> >      CreateOffscreenSurface(const gfxIntSize& size,
> >                             gfxASurface::gfxContentType contentType);
> >      
> > -    virtual bool SupportsAzure(mozilla::gfx::BackendType& aBackend) { aBackend = mozilla::gfx::BACKEND_SKIA; return true; }
> 
> Why was this removed?

I should be more clear here, it was removed, does that mean support in gfxAndroidPlatform is identical to that in gfxPlatform?
Comment 75 Bas Schouten (:bas.schouten) 2012-07-24 01:25:18 PDT
Comment on attachment 645194 [details] [diff] [review]
patch 13: enable Azure canvas pt 3

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

It would probably be worth making sure the fallback backend is also reported here, but we can do that later.
Comment 76 Nick Cameron [:nrc] 2012-07-24 02:06:52 PDT
(In reply to Bas Schouten (:bas) from comment #74)
> (In reply to Bas Schouten (:bas) from comment #73)
> > Comment on attachment 645193 [details] [diff] [review]
> > patch 12: enable Azure canvas pt 2
> > 
> > Review of attachment 645193 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/thebes/gfxAndroidPlatform.h
> > @@ -32,5 @@
> > >      virtual already_AddRefed<gfxASurface>
> > >      CreateOffscreenSurface(const gfxIntSize& size,
> > >                             gfxASurface::gfxContentType contentType);
> > >      
> > > -    virtual bool SupportsAzure(mozilla::gfx::BackendType& aBackend) { aBackend = mozilla::gfx::BACKEND_SKIA; return true; }
> > 
> > Why was this removed?
> 
> I should be more clear here, it was removed, does that mean support in
> gfxAndroidPlatform is identical to that in gfxPlatform?

The behaviour is fixed to that of gfxPlatform, but the behaviour is to return mPreferredCanvasBackend, which is set differently for different platforms. Right now, we will return cairo for Android because Skia is kind of broken, but hopefully we'll fix that and switch to Skia, if it proves to have better perf (which it probably will).
Comment 77 Nick Cameron [:nrc] 2012-07-24 02:08:01 PDT
(In reply to Bas Schouten (:bas) from comment #75)
> Comment on attachment 645194 [details] [diff] [review]
> patch 13: enable Azure canvas pt 3
> 
> Review of attachment 645194 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It would probably be worth making sure the fallback backend is also reported
> here, but we can do that later.

What do you mean by report? I filed bug 776802 about providing better info from gfxPlatform about the Azure backend.
Comment 78 Nick Cameron [:nrc] 2012-07-24 17:40:22 PDT
(In reply to Bas Schouten (:bas) from comment #73)
> Comment on attachment 645193 [details] [diff] [review]
> patch 12: enable Azure canvas pt 2
> 
> Review of attachment 645193 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/d3d10/LayerManagerD3D10.cpp
> @@ +488,4 @@
> >    if ((aFormat != FORMAT_B8G8R8A8 &&
> > +       aFormat != FORMAT_B8G8R8X8) ||
> > +       !gfxPlatform::GetPlatform()->SupportsAzure(backend) ||
> > +       backend != BACKEND_DIRECT2D) {
> 
> This really can't happen with D3D10 layers.
> 

It can happen if the user sets the pref to use a different backend, and without this change, that pref would not be respected.
Comment 79 Nick Cameron [:nrc] 2012-07-24 20:14:03 PDT
Created attachment 645632 [details] [diff] [review]
patch 12: enable Azure canvas pt 2

improved comment, changed static variable with destructor to pointer
Comment 80 Nick Cameron [:nrc] 2012-07-25 21:01:27 PDT
Created attachment 646007 [details] [diff] [review]
patch 14: remove AzureEnabled from gfxInfo
Comment 81 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-25 21:07:19 PDT
Comment on attachment 646007 [details] [diff] [review]
patch 14: remove AzureEnabled from gfxInfo

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

::: gfx/thebes/gfxPlatform.cpp
@@ +1177,5 @@
> +    static bool azureEnabled;
> +    if (!checkedPref) {
> +        azureEnabled = Preferences::GetBool("gfx.canvas.azure.enabled", false);
> +        checkedPref = true;
> +    }

Why not just always check this pref? No need to cache it AFAICT.

@@ +1181,5 @@
> +    }
> +
> +    if (!azureEnabled) {
> +        mPreferredCanvasBackend = BACKEND_NONE;
> +        mFallbackCanvasBackend = BACKEND_NONE;

and return? Because you overwrite these below.
Comment 82 Nick Cameron [:nrc] 2012-07-25 21:13:37 PDT
Created attachment 646013 [details] [diff] [review]
patch 14: remove AzureEnabled from gfxInfo

fixed roc's comments and changed GetAzureBackendInfo slightly
Comment 83 Nick Cameron [:nrc] 2012-07-25 22:13:28 PDT
Created attachment 646031 [details] [diff] [review]
patch 14: remove AzureEnabled from gfxInfo

removed an unnecessary pref check from AzureCanvasEnabled in nsCanvasRenderingContext2DAzure and fixed some typo bugs. Carrying r=roc.
Comment 85 Nick Cameron [:nrc] 2012-07-25 23:51:06 PDT
backed out: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ff7d09c5c945

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