Bug 24534 – Fix a bug in pattern transformation in PatternSkia

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Hin-Chung Lam]

PatternSkia should respect the local transformation in createPlatformPattern and apply the local transformation to shader. This patch fixes the following layout tests in chromium:

LayoutTests/svg/batik/paints/patternRegionA.svg
LayoutTests/svg/W3C-SVG-1.1/pservers-grad-06-b.svg
LayoutTests/svg/W3C-SVG-1.1/pservers-pattern-01-b.svg
LayoutTests/svg/W3C-SVG-1.1/coords-units-01-b.svg

Note that Skia is bad about having positive translation in tile mode. The code  takes special care of that. See the code for more details.

Review in chromium is here: http://codereview.chromium.org/42100/show

[Hin-Chung Lam]

Created attachment 28509 [details]
Patch to patternSkia.cpp

[Hin-Chung Lam]

Created attachment 28511 [details]
patch + change log

[Dimitri Glazkov (Google)]

Brett, can you look at this change?

[Brett Wilson (Google)]

I would like to let the Skia folks know about this before we work around
something. If it is a bug, it should be fixed there, or we should know a fix is
pending before checking in. I'll forward it.

[Hin-Chung Lam]

(In reply to comment #4)
> I would like to let the Skia folks know about this before we work around
> something. If it is a bug, it should be fixed there, or we should know a fix is
> pending before checking in. I'll forward it.
> 

Any updates from skia people?

[Dirk Schulze]

There is maybe another way to support transformations for patterns. On CG and Qt we transform the context right before we stroke/fill anything with a pattern, the path should not be affected. Just look at fillPath/strokePath in GraphicsContextCg.cpp or GraphicsContextQt.cpp to know what I mean. Could that avoid this hack at the end of the pattern-patch? To round and cut off some values seems to be incorrect for me.

[Eric Seidel (no email)]

Comment on attachment 28511 [details]
patch + change log

r- based on Dirk's comments.  Mostly I can't quite tell if this is right, Dirk is questioning the correctness, and we're trying to clear out the review queue one.  Feel free to mark this r=? with answers to Dirk's questions.

[Dirk Schulze]

No progress on this bug? :-(

[Stephen White]

Created attachment 47643 [details]
New fix

The good news is, the Skia bug should be fixed, so the hack part of Alpha's fix (above) should no longer be necessary.
The bad news is, I changed the allocation of SkShaders a few months back to be done once on first draw, so it was necessary to rework Patterns a bit so that the m_patternSpaceTransform could be forwarded to the SkShader.

[Stephen White]

Created attachment 47644 [details]
ChangeLog and non-Skia fixes

[WebKit Review Bot]

Attachment 47644 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/217145

[Stephen White]

Created attachment 47652 [details]
Same as above, plus QT fix

[Eric Seidel (no email)]

Comment on attachment 47652 [details]
Same as above, plus QT fix

I would have probably put the "else" into its own function, but OK.

[WebKit Commit Bot]

Comment on attachment 47652 [details]
Same as above, plus QT fix

Clearing flags on attachment: 47652

Committed r54203: <http://trac.webkit.org/changeset/54203>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.