1297201 - SVG rendering error when rotated

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[ben]

Attachment: repro.zip

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160817112116

Steps to reproduce:

Updated firefox from version 47.0.1 to 48.0.1. Went to page included the html snippet included in repro.html

Note the svg renders correctly if I remove the CSS transformation on the widget  "transform: rotate(-90deg)". I'm including repro.html (included in zip) which reproduces the issue.


Actual results:

SVG renders as incorrectRendering.png (included in zip folder). A mangled crescent with the majority filled in as pink, a little bit of blue, and a couple of missing divots in the lower portion.


Expected results:

SVG renders as correctRendering.png (included in zip folder). A ring with a majority filled in as pink the rest is blue

Comment 1

[ben]

I'm using OSX El Capitan Version 10.11.6 (15G31)

Comment 2

[ben]

Attachment: repro.html

Comment 3

[ben]

Attachment: correctRendering.png

Comment 4

[ben]

Attachment: incorrectRendering.png

Comment 5

[Robert Longson [:longsonr]]

regression range:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9873db170eaa08b203fd39fbf8f02fcb8358a10e&tochange=66ee9fcbbbd3f207c3fcc141aa2da12ca9534894

Comment 6

[Lee Salzman [:lsalzman]]

I tried running mozregression on Linux using Skia content/canvas with accelerated canvas and it would not reproduce there. So somehow this is localized Mac only despite seemingly being a "Skia issue". There is probably something non-obvious going on here.

Ben and Robert, can you paste the Graphics section from about:support?

Comment 7

[Lee Salzman [:lsalzman]]

We got a confirmation that this problem seems to be present even on nightly on OS X. Maybe something with layers is messing things up here?

Comment 8

[Robert Longson [:longsonr]]

Attachment: about:support

Comment 9

[ben]

Graphics
Features
Compositing	OpenGL
Asynchronous Pan/Zoom	none
WebGL Renderer	Intel Inc. -- Intel(R) Iris(TM) Graphics 6100
Hardware H264 Decoding	Yes
GPU #1
Active	Yes
Vendor ID	0x8086
Device ID	0x162b
Diagnostics
AzureCanvasAccelerated	1
AzureCanvasBackend	skia
AzureContentBackend	skia
AzureFallbackCanvasBackend	none

Comment 10

[Mason Chang [Inactive] [:mchang]]

This seems to only happen on OS X. Windows with Skia doesn't reproduce the problem almost.

Comment 11

[Mason Chang [Inactive] [:mchang]]

Attachment: Even smaller testcase

Comment 12

[Mason Chang [Inactive] [:mchang]]

Attachment: Use native push / pop layer on OS X

This was happening because we wouldn't use native push/poplayer on OS X as previously the CG backend didn't support it. However, Skia does, and using native push/poplayer was always true for linux / windows, which is why this only happened on OS X.

This deletes the pref to use push/pop layer and always uses it unless we have a CG backend. We can delete this part in 53 once we delete CG.

Comment 13

[Mason Chang [Inactive] [:mchang]]

Attachment: Use native push / pop layer on OS X

Comment 14

[Mason Chang [Inactive] [:mchang]]

try push for mac since that's the only place that should be affected by this - https://treeherder.mozilla.org/#/jobs?repo=try&revision=d914301a24e3

Comment 15

[Lee Salzman [:lsalzman]]

Attachment: apply offset after pattern matrix in DrawTargetSkia::MaskSurface

When we subtract off the mask's offset from the source pattern, we need to make sure we do this only after the pattern matrix is applied.

The problem is ->makeWithLocalMatrix() applies that matrix before the pattern matrix that was already set on the bitmap shader, thus getting the order all screwed up. The only other alternative would be to invert the pattern matrix, apply to the offset, and create a local matrix shader with that inverse transformed offset, but that would be slow and messy...

Thus I went with the slightly ugly but faster alternative of just chucking the offset onto the pattern matrix when we build the bitmap shader. Skia does not allow the bitmap shader's matrix to be modified after it is constructed, unfortunately, so this is necessary.

Comment 16

[Nicolas Silva [:nical]]

Beware, last time I tried, native push layer was not working with DrawTargetTiled (used on OSX but not linux).

Comment 17

[Mason Chang [Inactive] [:mchang]]

(In reply to Nicolas Silva [:nical] from comment #16)
> Beware, last time I tried, native push layer was not working with
> DrawTargetTiled (used on OSX but not linux).

This is true too, so we have to fix that.

Comment 18

[Lee Salzman [:lsalzman]]

Pushing my MaskSurface fix while we're still figuring out how to resolve the PushLayer/tiling situation...

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c9e8b9fa637
apply offset after pattern matrix in DrawTargetSkia::MaskSurface. r=mchang

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c9e8b9fa637

Comment 21

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP Rolled up push layer

Fixes push/poplayer and tiling, but found out that we're using cairo on android. Works on OS X, but tiling + cairo on android doesn't seem to work yet.

Comment 22

[Lee Salzman [:lsalzman]]

Attachment: Bug 1297201 - apply offset after pattern matrix in DrawTargetSkia::MaskSurface (49 and 50). r=mchang

This is just a version of the MaskSurface offset patch rebased for aurora/beta. This should at least mitigate the issue for those while we're working on the cleaner PushLayer/tiling solution for 51+.

Approval Request Comment
[Feature/regressing bug #]: bug 1207332 in 48
[User impact if declined]: Broken SVG rendering.
[Describe test coverage new/current, TreeHerder]: mochitests/reftests
[Risks and why]: None. Patch has happily been living in central for a while now, and takes something that we're 100% certain was broken and makes it 100% working.
[String/UUID change made/needed]: None

Comment 24

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8786506 [details] [diff] [review]
WIP Rolled up push layer

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

Push and poplayer work with tiling now thanks to skia android. There were two main problems:

1) We'd push/pop layer on every tile, even if nothing was going to be drawn to that tile. This caused us to sometimes blend with nothing, causing transparent rects to show up.
2) When getting the CGContext and locking bits around skia, we'd hit an assert because we didn't pass in an origin. I don't think we actually need it since we just grab the bits and flip the bitmap around to set the origin at the top left, then replay all the transforms anyway.
3) We were failing to pass in the aCopyBackground parameter from DrawTargetTiling down to the actual underlying draw target.

Comment 25

[Lee Salzman [:lsalzman]]

Comment on attachment 8786506 [details] [diff] [review]
WIP Rolled up push layer

r+ but with some nits:

I would recommend we just make GetDeviceClipBounds return an IntRect like so:

IntRect gfxContext::GetDeviceClipBounds() {
  IntRect clipBounds = RoundedOut(GetAzureDeviceSpaceClipBounds());
  clipBounds.width = std::max(1, clipBounds.width);
  clipBounds.height = std::max(1, clipBounds.height);
  return clipBounds;
}

That way, we don't have to do all those superfluous Rect to IntRect conversions.

Also: IntRect tileBounds(mTiles[i].mTileOrigin.x, mTiles[i].mTileOrigin.y, tileSize.width, tileSize.height);

IntRect has an IntRect(IntPoint,IntSize) constructor, so that can just be: IntRect tileBounds(tiles[i].mTileOrigin, tileSize);

Comment 26

[Mason Chang [Inactive] [:mchang]]

Try looks good - https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcfcb6d4ccbd

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8787022 [details] [diff] [review]
Bug 1297201 - apply offset after pattern matrix in DrawTargetSkia::MaskSurface (49 and 50). r=mchang

As we already shipped 48 with the issue and we are super late in the 49 cycle, we should let it ride the train from 50

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e14feb2cfb60