shading-pattern: While drawing patterns, use transform to baseTransform first #6684

dsprenkels · 2015-11-24T15:31:19Z

I propose a fix for issue #6296.

I am, however, a bit uncertain about the implementation: maybe it would be better to alter the coordinates directly, instead of using ctx.save() and ctx.restore(). (Or maybe doing this in RadialAxialClosure).

dsprenkels · 2015-11-25T17:21:46Z

This PR should be ready now. I'll squash the commits on request.

bwesterb · 2015-11-25T17:23:03Z

test/test_manifest.json

@@ -1839,7 +1839,7 @@
       "link": false,
       "type": "load",
       "password": "\u00E6\u00F8\u00E5",
-       "about": "The password (���) is UTF8 encoded."
+       "about": "The password (���) is UTF8 encoded."


dsprenkels · 2015-11-25T18:20:29Z

I have not yet looked into the following: I applied the change in CanvasGraphics.fill, but a similar change may be needed in CanvasGraphics.{stroke,shadingFill}, don't you think?

timvandermeij · 2015-11-26T19:09:12Z

@dsprenkels Could you squash the commits? Looking at the diff in https://github.com/mozilla/pdf.js/pull/6684/files, I don't see how the owner argument is ever passed to getPattern. You have added it in the function header, but not in any calls. You might also need to handle owner being undefined.

dsprenkels · 2015-11-27T10:14:46Z

I dabbled trying to revert the transformation, but as you say, I believe I have the "wrong" getPattern function. I think that, at the moment https://github.com/dsprenkels/pdf.js/commit/8a9480d893a5b0ba35f6102029c823ac57a81681 is the best fix.

I have seen, however, that this bug not only occurs in CanvasGraphics_fill, but also in CanvasGraphics_stroke. I would be preferable to fix this while we are at it, I believe. The point is, however, that if I use the method of using

ctx.save();
// (...)
var m = this.baseTransform;
ctx.setTransform(m[0], m[1], m[2], m[3], m[4], m[5]);
// (...)

the stroke width is not correct anymore, (and I am not sure if other variables may be influenced as well).

The method I use in https://github.com/dsprenkels/pdf.js/commit/e5adbf62c37a964824b40fc52243acbd87eff05b is bad (but illustrates the idea of reverting the transformation done by the 0.1 0 0 0.1 0 0 cm instruction). If I could maybe get some guidance (perhaps via IRC) about where to implement a correction for the "current transformation", I'd be very happy. At the moment I feel like I'm just dabbling around. On the other hand, if you like, we could use the fix in https://github.com/dsprenkels/pdf.js/commit/8a9480d893a5b0ba35f6102029c823ac57a81681. The issue would be fixed, but the same problem would remain in CanvasGraphics_stroke.

timvandermeij · 2015-11-29T13:29:10Z

This patch completely fixes #6296, #6486, #6293 (except for the last PDF which will become a separate issue) and #5436. It largely fixes #6298. Let's revert to the situation before the latest changes as that seems a reasonable way to fix this issue. Other methods can be updated in a follow-up patch, as it would be good to land this since it fixes a bunch of issues.

You can always contact us in IRC to discuss this patch.

dsprenkels · 2015-12-02T10:23:15Z

@timvandermeij
I reverted to the state where I used the save()-restore() fix and squashed into one commit.

yurydelendik · 2015-12-02T14:31:29Z

src/display/canvas.js

@@ -1089,6 +1089,9 @@ var CanvasGraphics = (function CanvasGraphicsClosure() {

      if (isPatternFill) {
        ctx.save();
+        var m = this.baseTransform ||
+                this.ctx.mozCurrentTransform.slice();
+        ctx.setTransform(m[0], m[1], m[2], m[3], m[4], m[5]);


So this code just performs: if (this.baseTransform) { ctx.setTransform.apply(ctx, this.baseTransform); }? (Now I wonder if we ever will get this.baseTransform == null.)

I don't think you need to perform this.ctx.mozCurrentTransform and its slicing -- it will just create unneeded objects.

Shall stroke have the same statements?

So this code just performs: if (this.baseTransform) { ctx.setTransform.apply(ctx, this.baseTransform); }? (Now I wonder if we ever will get this.baseTransform == null.)

I initially just used this.baseTransform, got null in some test cases from the test suite, and saw that in other places the expression this.baseTransform || this.ctx.mozCurrentTransform.slice(); was used. I did not know that the current tranformation was actually stored in this.ctx.mozCurrentTransform, (although now it makes a lot of sense).
You are right that this piece of code could better be if (this.baseTransform) { ctx.setTransform.apply(ctx, this.baseTransform); }.

Shall stroke have the same statements?

I some manner of speaking: "Yes". However, when transforming the stroke, the stroke width would be transformed as well. I do not know if other side effects occur when using ctx.setTransform(), that's why I initially thought that it may be a better idea to not use ctx.save() and ctx.restore(), but to alter the matrices of the patterns directly.

the stroke width would be transformed as well

IMHO if it needs to be done, then we shall do it.

…rm first

timvandermeij · 2015-12-14T15:49:39Z

/botio-linux preview

pdfjsbot · 2015-12-14T15:49:40Z

From: Bot.io (Linux)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/20102b3966f74cd/output.txt

pdfjsbot · 2015-12-14T15:50:30Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/20102b3966f74cd/output.txt

Total script time: 0.84 mins

Published

timvandermeij · 2015-12-14T15:53:43Z

/botio test

pdfjsbot · 2015-12-14T15:53:43Z

From: Bot.io (Linux)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/57736848603eae8/output.txt

pdfjsbot · 2015-12-14T15:53:43Z

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/bb7ed3406c1e2d8/output.txt

pdfjsbot · 2015-12-14T16:13:57Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/bb7ed3406c1e2d8/output.txt

Total script time: 20.22 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2015-12-14T16:14:00Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/57736848603eae8/output.txt

Total script time: 20.27 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

timvandermeij · 2015-12-14T18:11:47Z

/botio makeref

pdfjsbot · 2015-12-14T18:11:47Z

From: Bot.io (Windows)

Received

Command cmd_makeref from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/f119b893fd27fd1/output.txt

pdfjsbot · 2015-12-14T18:11:48Z

From: Bot.io (Linux)

Received

Command cmd_makeref from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/f2b78b29f43227d/output.txt

pdfjsbot · 2015-12-14T18:31:08Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/f119b893fd27fd1/output.txt

Total script time: 19.33 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2015-12-14T18:31:26Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/f2b78b29f43227d/output.txt

Total script time: 19.64 mins

Lint: Passed
Make references: Passed
Check references: Passed

shading-pattern: While drawing patterns, use transform to baseTransform first

timvandermeij · 2015-12-14T19:39:15Z

Thank you for the fix!

Snuffleupagus · 2015-12-16T13:09:46Z

This also fixes https://bugzilla.mozilla.org/show_bug.cgi?id=870041.

Snuffleupagus · 2016-01-02T17:08:55Z

This also fixed https://bugzilla.mozilla.org/show_bug.cgi?id=1105594.

timvandermeij added shading-pattern image-quality 4-work-in-progress labels Nov 24, 2015

dsprenkels changed the title ~~shading-pattern: While drawing patterns, use owner.baseTransform~~ shading-pattern: While drawing patterns, use transform to baseTransform first Nov 25, 2015

bwesterb reviewed Nov 25, 2015
View reviewed changes

yurydelendik reviewed Dec 2, 2015
View reviewed changes

shading-pattern: While drawing patterns, transform to the baseTransfo…

a908165

…rm first

timvandermeij removed the 4-work-in-progress label Dec 3, 2015

timvandermeij assigned yurydelendik Dec 3, 2015

timvandermeij assigned timvandermeij and unassigned yurydelendik Dec 14, 2015

timvandermeij added a commit that referenced this pull request Dec 14, 2015

Merge pull request #6684 from dsprenkels/issue-6296-radial-shading-size

f93a220

shading-pattern: While drawing patterns, use transform to baseTransform first

timvandermeij merged commit f93a220 into mozilla:master Dec 14, 2015

timvandermeij mentioned this pull request Dec 14, 2015

Radial shading (type 3) too small #6296

Closed

Snuffleupagus mentioned this pull request Dec 14, 2015

Pdf doesn't render appropriately #3946

Closed

dsprenkels deleted the issue-6296-radial-shading-size branch December 14, 2015 22:03

dsprenkels mentioned this pull request Dec 17, 2015

Transform to resource transform when stroking pattern #6769

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shading-pattern: While drawing patterns, use transform to baseTransform first #6684

shading-pattern: While drawing patterns, use transform to baseTransform first #6684

dsprenkels commented Nov 24, 2015

dsprenkels commented Nov 25, 2015

bwesterb Nov 25, 2015

dsprenkels commented Nov 25, 2015

timvandermeij commented Nov 26, 2015

dsprenkels commented Nov 27, 2015

timvandermeij commented Nov 29, 2015

dsprenkels commented Dec 2, 2015

yurydelendik Dec 2, 2015

dsprenkels Dec 2, 2015

yurydelendik Dec 2, 2015

timvandermeij commented Dec 14, 2015

pdfjsbot commented Dec 14, 2015

pdfjsbot commented Dec 14, 2015

timvandermeij commented Dec 14, 2015

pdfjsbot commented Dec 14, 2015

pdfjsbot commented Dec 14, 2015

pdfjsbot commented Dec 14, 2015

pdfjsbot commented Dec 14, 2015

timvandermeij commented Dec 14, 2015

pdfjsbot commented Dec 14, 2015

pdfjsbot commented Dec 14, 2015

pdfjsbot commented Dec 14, 2015

pdfjsbot commented Dec 14, 2015

timvandermeij commented Dec 14, 2015

Snuffleupagus commented Dec 16, 2015

Snuffleupagus commented Jan 2, 2016

shading-pattern: While drawing patterns, use transform to baseTransform first #6684

shading-pattern: While drawing patterns, use transform to baseTransform first #6684

Conversation

dsprenkels commented Nov 24, 2015

dsprenkels commented Nov 25, 2015

bwesterb Nov 25, 2015

Choose a reason for hiding this comment

dsprenkels commented Nov 25, 2015

timvandermeij commented Nov 26, 2015

dsprenkels commented Nov 27, 2015

timvandermeij commented Nov 29, 2015

dsprenkels commented Dec 2, 2015

yurydelendik Dec 2, 2015

Choose a reason for hiding this comment

dsprenkels Dec 2, 2015

Choose a reason for hiding this comment

yurydelendik Dec 2, 2015

Choose a reason for hiding this comment

timvandermeij commented Dec 14, 2015

pdfjsbot commented Dec 14, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 14, 2015

From: Bot.io (Linux)

Success

Published

timvandermeij commented Dec 14, 2015

pdfjsbot commented Dec 14, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 14, 2015

From: Bot.io (Windows)

Received

pdfjsbot commented Dec 14, 2015

From: Bot.io (Windows)

Success

pdfjsbot commented Dec 14, 2015

From: Bot.io (Linux)

Success

timvandermeij commented Dec 14, 2015

pdfjsbot commented Dec 14, 2015

From: Bot.io (Windows)

Received

pdfjsbot commented Dec 14, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 14, 2015

From: Bot.io (Windows)

Success

pdfjsbot commented Dec 14, 2015

From: Bot.io (Linux)

Success

timvandermeij commented Dec 14, 2015

Snuffleupagus commented Dec 16, 2015

Snuffleupagus commented Jan 2, 2016