Closed Bug 1297201 Opened 3 years ago Closed 3 years ago

SVG rendering error when rotated


(Core :: Graphics, defect)

48 Branch
Not set



Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- fixed
firefox51 --- fixed


(Reporter: ben, Assigned: mchang)



(Keywords: regression, Whiteboard: [gfx-noted])


(10 files, 1 obsolete file)

Attached file
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
I'm using OSX El Capitan Version 10.11.6 (15G31)
Attached file repro.html
Attached image correctRendering.png
Attached image incorrectRendering.png
regression range:
Blocks: skia-osx
Component: Untriaged → Graphics
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
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?
Flags: needinfo?(ben)
Whiteboard: [gfx-noted]
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?
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
AzureCanvasAccelerated	1
AzureCanvasBackend	skia
AzureContentBackend	skia
AzureFallbackCanvasBackend	none
This seems to only happen on OS X. Windows with Skia doesn't reproduce the problem almost.
Assignee: nobody → mchang
Flags: needinfo?(ben)
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.
Attachment #8784118 - Flags: review?(lsalzman)
Attachment #8784118 - Attachment is obsolete: true
Attachment #8784118 - Flags: review?(lsalzman)
Attachment #8784120 - Flags: review?(lsalzman)
Attachment #8784120 - Flags: review?(lsalzman) → review+
try push for mac since that's the only place that should be affected by this -
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.
Attachment #8784512 - Flags: review?(mchang)
Attachment #8784512 - Flags: review?(mchang) → review+
Beware, last time I tried, native push layer was not working with DrawTargetTiled (used on OSX but not linux).
(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.
Pushing my MaskSurface fix while we're still figuring out how to resolve the PushLayer/tiling situation...
Keywords: leave-open
Pushed by
apply offset after pattern matrix in DrawTargetSkia::MaskSurface. r=mchang
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.
Has Regression Range: --- → yes
Has STR: --- → yes
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
Attachment #8787022 - Flags: review+
Attachment #8787022 - Flags: approval-mozilla-beta?
Attachment #8787022 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 1299354
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.
Attachment #8786506 - Flags: review?(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);
Attachment #8786506 - Flags: review?(lsalzman) → review+
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
Attachment #8787022 - Flags: approval-mozilla-beta?
Attachment #8787022 - Flags: approval-mozilla-beta-
Attachment #8787022 - Flags: approval-mozilla-aurora?
Attachment #8787022 - Flags: approval-mozilla-aurora+
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.