[Azure] Get Azure/Cairo canvas passing tests

RESOLVED FIXED in mozilla17

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: nrc, Assigned: nrc)

Tracking

(Depends on: 1 bug, Blocks: 4 bugs)

15 Branch
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(18 attachments, 24 obsolete attachments)

950 bytes, patch
bas
: review+
Details | Diff | Splinter Review
1.24 KB, patch
bas
: review+
Details | Diff | Splinter Review
11.67 KB, patch
bas
: review+
Details | Diff | Splinter Review
7.10 KB, patch
bas
: review+
Details | Diff | Splinter Review
2.23 KB, patch
bas
: review+
Details | Diff | Splinter Review
2.87 KB, patch
bas
: review+
Details | Diff | Splinter Review
2.41 KB, patch
karlt
: review+
Details | Diff | Splinter Review
1011 bytes, patch
bas
: review+
Details | Diff | Splinter Review
12.71 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
1.98 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.33 KB, patch
nrc
: review+
Details | Diff | Splinter Review
13.58 KB, patch
nrc
: review+
Details | Diff | Splinter Review
6.18 KB, patch
bas
: review+
Details | Diff | Splinter Review
2.17 KB, patch
bas
: review+
Details | Diff | Splinter Review
3.21 KB, patch
bas
: review+
Details | Diff | Splinter Review
9.82 KB, patch
bas
: review+
Details | Diff | Splinter Review
20.97 KB, patch
bas
: review+
Details | Diff | Splinter Review
15.63 KB, patch
nrc
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Updated

5 years ago
Depends on: 761895
Created attachment 632382 [details] [diff] [review]
Fixes for 2d.toDataURL.*

The attached patch should fix the 2d.toDataURL.* by implementing DrawTargetCairo::CopySurface()
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.
Attachment #632382 - Attachment is obsolete: true
Created attachment 633021 [details] [diff] [review]
Implemented DrawTargetCairo::CopySurface to fix 2d.toDataURL.*

Updated patch.
Attachment #632389 - Attachment is obsolete: true
Created attachment 633022 [details] [diff] [review]
Fixed surface edges on Linux to make 2d.drawImage.5arg pass
Created attachment 633023 [details] [diff] [review]
Possible logic error affecting Cairo win32
(Assignee)

Comment 6

5 years ago
Created attachment 636212 [details] [diff] [review]
patch 1: Add DrawTarget arg to GetScaledFontForFont
Attachment #636212 - Flags: review?(bas.schouten)
(Assignee)

Comment 7

5 years ago
Created attachment 636213 [details] [diff] [review]
patch 2: small fix
Attachment #636213 - Flags: review?(bas.schouten)
(Assignee)

Comment 8

5 years ago
Created attachment 636214 [details] [diff] [review]
patch 1: Add DrawTarget arg to GetScaledFontForFont
Attachment #636212 - Attachment is obsolete: true
Attachment #636212 - Flags: review?(bas.schouten)
Attachment #636214 - Flags: review?(bas.schouten)
(Assignee)

Comment 9

5 years ago
Created attachment 636215 [details] [diff] [review]
patch 3: pass around the size of a gfxASurface when creating a DrawTarget
Attachment #636215 - Flags: review?(bas.schouten)
(Assignee)

Comment 10

5 years ago
Created attachment 636216 [details] [diff] [review]
patch 4: store a reference to the backing surface in DrawTargetCairo
Attachment #636216 - Flags: review?(bas.schouten)
(Assignee)

Comment 11

5 years ago
Created attachment 636217 [details] [diff] [review]
patch 5: drawing changes to DrawTargetCairo
Attachment #636217 - Flags: review?(bas.schouten)
(Assignee)

Comment 12

5 years ago
Created attachment 636218 [details] [diff] [review]
patch 6: Fix a bug where a path with a transform is re-used
Attachment #636218 - Flags: review?(bas.schouten)
(Assignee)

Comment 13

5 years ago
Created attachment 636219 [details] [diff] [review]
patch 7: changes to nsCanvasRenderingContext2DAzure
Attachment #636219 - Flags: review?(joe)
Attachment #636214 - Flags: review?(bas.schouten) → review+
Attachment #636213 - Flags: review?(bas.schouten) → review+
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)
Created attachment 637004 [details] [diff] [review]
patch 1.5: implement DrawTargetCairo::CopySurface
Attachment #633021 - Attachment is obsolete: true
Attachment #637004 - Flags: review?(bas.schouten)
Attachment #637004 - Attachment description: part 1.5: implement DrawTargetCairo::CopySurface → patch 1.5: implement DrawTargetCairo::CopySurface
Created attachment 637006 [details] [diff] [review]
patch 8: Fix canvas 2d.drawImage.5arg and 2d.drawImage.9arg surface scaling tests
Attachment #633022 - Attachment is obsolete: true
Attachment #637006 - Flags: review?(karlt)
Created attachment 637007 [details] [diff] [review]
patch 9: fixed crash test fail on gtk caused by null
Attachment #637007 - Flags: review?(bas.schouten)
Created attachment 637009 [details] [diff] [review]
patch 10: Fixed build break for qt
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"
Attachment #637006 - Flags: review?(karlt) → review+
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();
Attachment #637004 - Flags: review?(bas.schouten) → review-
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.
Attachment #637007 - Flags: review?(bas.schouten) → review+
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.
Attachment #636216 - Flags: review?(bas.schouten) → review+
Attachment #636218 - Flags: review?(bas.schouten) → review+
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.
Attachment #637004 - Attachment is obsolete: true
Attachment #638865 - Flags: review?(bas.schouten)
(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? ;)
(In reply to Bas Schouten (:bas) from comment #24)
> I assume you checked if it worked? ;)

Your assumption is correct.
Attachment #637009 - Attachment description: patch 10: Fixed build break for b2g/qt → patch 10: Fixed build break for qt
Attachment #637009 - Flags: review?(gwright)
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.
Attachment #633023 - Flags: review?(bas.schouten)
(Assignee)

Comment 27

5 years ago
Created attachment 639196 [details] [diff] [review]
patch 9.5 test tweaks
Attachment #639196 - Flags: review?(joe)
(Assignee)

Comment 28

5 years ago
Created attachment 639197 [details] [diff] [review]
patch 5: drawing changes to DrawTargetCairo
Attachment #636217 - Attachment is obsolete: true
Attachment #636217 - Flags: review?(bas.schouten)
Attachment #639197 - Flags: review?(bas.schouten)
(Assignee)

Updated

5 years ago
Attachment #636219 - Flags: review?(joe) → review?(bas.schouten)
(Assignee)

Comment 29

5 years ago
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.
Attachment #639196 - Flags: review?(joe) → review?(bas.schouten)
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.
(Assignee)

Comment 31

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

Comment 32

5 years ago
Created attachment 640100 [details] [diff] [review]
patch 7: changes to nsCanvasRenderingContext2DAzure

rebased and fixed a bug
Attachment #636219 - Attachment is obsolete: true
Attachment #636219 - Flags: review?(bas.schouten)
Attachment #640100 - Flags: review?(bas.schouten)
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?
Attachment #637009 - Flags: review?(gwright) → review?(jfkthame)
Attachment #638865 - Flags: review?(bas.schouten) → review?(jmuizelaar)
Attachment #636215 - Flags: review?(bas.schouten) → review?(jmuizelaar)
Attachment #639197 - Flags: review?(bas.schouten) → review?(jmuizelaar)
Attachment #640100 - Flags: review?(bas.schouten) → review?(jmuizelaar)
Attachment #639196 - Flags: review?(bas.schouten) → review?(jmuizelaar)
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.
Attachment #638865 - Flags: review?(jmuizelaar) → review-
(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.
Attachment #640100 - Flags: review?(jmuizelaar) → review?(bas.schouten)
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
Attachment #636215 - Flags: review?(jmuizelaar) → review-
Created attachment 640848 [details] [diff] [review]
patch 1.5: implement DrawTargetCairo::CopySurface [v3]

cairo_fill instead of cairo_clip and cairo_paint
Attachment #638865 - Attachment is obsolete: true
Attachment #640848 - Flags: review?(jmuizelaar)
Attachment #640848 - Flags: review?(jmuizelaar) → review+
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.
Attachment #639196 - Flags: review?(jmuizelaar) → review-
(Assignee)

Comment 39

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

Comment 40

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

Comment 41

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

Updated

5 years ago
Blocks: 773460
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.
Attachment #639197 - Flags: review?(jmuizelaar) → review+
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
Attachment #639196 - Flags: review- → review+
(Assignee)

Comment 44

5 years ago
Created attachment 642528 [details] [diff] [review]
patch 9.5 test tweaks

Added comment, carrying r=jrmuizel
Attachment #639196 - Attachment is obsolete: true
Attachment #642528 - Flags: review+
(Assignee)

Comment 45

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

Comment 47

5 years ago
Created attachment 642533 [details] [diff] [review]
patch 9.5 test tweaks

with a conditional test, rather than commenting it out
Attachment #642533 - Flags: review+
(Assignee)

Comment 48

5 years ago
(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.
Attachment #633023 - Flags: review?(bas.schouten) → review+
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).
(Assignee)

Comment 50

5 years ago
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
Attachment #642528 - Attachment is obsolete: true
Attachment #642533 - Attachment is obsolete: true
Attachment #643330 - Flags: review+
(Assignee)

Comment 51

5 years ago
Created attachment 643342 [details] [diff] [review]
patch 5: drawing changes to DrawTargetCairo

changes as requested; carrying r=jrmuizel
Attachment #643342 - Flags: review+
(Assignee)

Updated

5 years ago
Blocks: 775063
(Assignee)

Comment 52

5 years ago
Created attachment 643351 [details] [diff] [review]
patch 7: changes to nsCanvasRenderingContext2DAzure

removed the mNewPath stuff (to Bug 775063).
Attachment #640100 - Attachment is obsolete: true
Attachment #640100 - Flags: review?(bas.schouten)
Attachment #643351 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #643351 - Flags: review? → review?(bas.schouten)
Attachment #636215 - Flags: review- → review+
(Assignee)

Updated

5 years ago
Blocks: 775145
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.
Attachment #637009 - Attachment is obsolete: true
Attachment #637009 - Flags: review?(jfkthame)

Updated

5 years ago
Blocks: 622842
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.)
(Assignee)

Comment 55

5 years ago
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
Attachment #644796 - Flags: review?
(Assignee)

Updated

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

Comment 56

5 years ago
Created attachment 644797 [details] [diff] [review]
patch 10: mochitest for isPointInPath and multiple transforms
Attachment #644797 - Flags: review?(bas.schouten)
(Assignee)

Updated

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

Updated

5 years ago
Attachment #644796 - Flags: review? → review?(bas.schouten)
(Assignee)

Comment 57

5 years ago
Created attachment 644831 [details] [diff] [review]
patch 10: mochitest for isPointInPath and multiple transforms
Attachment #644831 - Flags: review?(bas.schouten)
(Assignee)

Comment 58

5 years ago
Created attachment 644832 [details] [diff] [review]
patch 11: enabling Azure Canvas pt 1
Attachment #644832 - Flags: review?(bas.schouten)
(Assignee)

Comment 59

5 years ago
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?
Attachment #644833 - Flags: review?(bas.schouten)
(Assignee)

Comment 60

5 years ago
Created attachment 644834 [details] [diff] [review]
patch 13: enable Azure canvas pt 3
Attachment #644834 - Flags: review?(bas.schouten)
(Assignee)

Comment 61

5 years ago
Created attachment 644836 [details] [diff] [review]
patch 11: enabling Azure Canvas pt 1

core -> cg
Attachment #644832 - Attachment is obsolete: true
Attachment #644832 - Flags: review?(bas.schouten)
Attachment #644836 - Flags: review?(bas.schouten)
(Assignee)

Comment 62

5 years ago
Created attachment 644837 [details] [diff] [review]
patch 12: enable Azure canvas pt 2

core -> cg
Attachment #644833 - Attachment is obsolete: true
Attachment #644833 - Flags: review?(bas.schouten)
Attachment #644837 - Flags: review?(bas.schouten)
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".
Attachment #644833 - Attachment is obsolete: false
What about different kinds of cairo backends? Should we split cairo into cairo-image, cairo-xlib, cairo-gdi, etc?
Attachment #644833 - Attachment is obsolete: true
(Assignee)

Comment 65

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

Comment 66

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

Updated

5 years ago
Blocks: 776685
(Assignee)

Updated

5 years ago
Blocks: 776802
(Assignee)

Comment 70

5 years ago
Created attachment 645191 [details] [diff] [review]
patch 11: enabling Azure Canvas pt 1
Attachment #644836 - Attachment is obsolete: true
Attachment #644836 - Flags: review?(bas.schouten)
Attachment #645191 - Flags: review?(bas.schouten)
(Assignee)

Comment 71

5 years ago
Created attachment 645193 [details] [diff] [review]
patch 12: enable Azure canvas pt 2
Attachment #644837 - Attachment is obsolete: true
Attachment #644837 - Flags: review?(bas.schouten)
Attachment #645193 - Flags: review?(bas.schouten)
(Assignee)

Comment 72

5 years ago
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!)
Attachment #644834 - Attachment is obsolete: true
Attachment #644834 - Flags: review?(bas.schouten)
Attachment #645194 - Flags: review?(bas.schouten)
Attachment #644796 - Flags: review?(bas.schouten) → review+
Attachment #644831 - Flags: review?(bas.schouten) → review+
Attachment #645191 - Flags: review?(bas.schouten) → review+
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.
(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 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.
Attachment #645194 - Flags: review?(bas.schouten) → review+
(Assignee)

Comment 76

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

Comment 77

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

Comment 78

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

Comment 79

5 years ago
Created attachment 645632 [details] [diff] [review]
patch 12: enable Azure canvas pt 2

improved comment, changed static variable with destructor to pointer
Attachment #645193 - Attachment is obsolete: true
Attachment #645193 - Flags: review?(bas.schouten)
Attachment #645632 - Flags: review?(bas.schouten)
Attachment #645632 - Flags: review?(bas.schouten) → review+
(Assignee)

Comment 80

5 years ago
Created attachment 646007 [details] [diff] [review]
patch 14: remove AzureEnabled from gfxInfo
Attachment #646007 - Flags: review?(roc)
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.
(Assignee)

Comment 82

5 years ago
Created attachment 646013 [details] [diff] [review]
patch 14: remove AzureEnabled from gfxInfo

fixed roc's comments and changed GetAzureBackendInfo slightly
Attachment #646007 - Attachment is obsolete: true
Attachment #646007 - Flags: review?(roc)
Attachment #646013 - Flags: review?(roc)
Attachment #646013 - Flags: review?(roc) → review+
(Assignee)

Comment 83

5 years ago
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.
Attachment #646013 - Attachment is obsolete: true
Attachment #646031 - Flags: review+
(Assignee)

Comment 84

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=eb4f2872a6ca
(Assignee)

Comment 85

5 years ago
backed out: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ff7d09c5c945
(Assignee)

Comment 86

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c5125dde4bbf
https://hg.mozilla.org/mozilla-central/rev/9b9acad7f6d0
https://hg.mozilla.org/mozilla-central/rev/06a5feea546f
https://hg.mozilla.org/mozilla-central/rev/f4c2417994aa
https://hg.mozilla.org/mozilla-central/rev/f5a58948203f
https://hg.mozilla.org/mozilla-central/rev/ad4139ad6247
https://hg.mozilla.org/mozilla-central/rev/f731bb5b22b8
https://hg.mozilla.org/mozilla-central/rev/e2deb8b45529
https://hg.mozilla.org/mozilla-central/rev/1260e5f010c0
https://hg.mozilla.org/mozilla-central/rev/0223c2ea0033
https://hg.mozilla.org/mozilla-central/rev/cce58a8050f3
https://hg.mozilla.org/mozilla-central/rev/0f8c4f6e6248
https://hg.mozilla.org/mozilla-central/rev/0278d5acd5f2
https://hg.mozilla.org/mozilla-central/rev/fcc82e42815f
https://hg.mozilla.org/mozilla-central/rev/03e169cce29a
https://hg.mozilla.org/mozilla-central/rev/79a562f4b12d
https://hg.mozilla.org/mozilla-central/rev/148d57e5d062
https://hg.mozilla.org/mozilla-central/rev/3c73222e6e1c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 778195

Updated

5 years ago
Depends on: 780392
Depends on: 921132

Updated

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