Initial work to fix detail loss with additional canvas needed on drawImage #1255

asturur · 2018-09-25T19:05:03Z

The problem

In the case when we specify a small destination area, but with a big scale of the context, we need to take in account that if generating an addtional canvas.

This PR introduces a function to extrapolate the scale value of the current transform ( together with the other properties ) And we will increase the area of the temporary surface in order to account the scale so that details are preserved.

After various iterations this code looks. good. I understand it adds complexity.

In case we need an additional canvas we want to know how bit it will be before creating it, so that we can create one that is as small as required to get the right amount of details.

In case the context has has a scaling of 3, and the drawImage is something like:

drawImage(source, 0, 0, 100, 100, 0, 0, 30, 30)

The draw image will need to create a 90x90 surface where to draw the 100x100 pixel from the source.
(since 30x3 = 90)

Also when drawImage gets used like:

drawImage(source, -10, -10, 100, 100, 0, 0, 30, 30)

the 10pixel from sx and sy arguments are blank, we do not need to have space in the extra canvas to allocate them.

There is an added level of complexity that is still missing:
It would be nice to understand when we are about to draw an image at more than 100% size and avoid allocating space in the temporary surface.

asturur · 2018-09-25T19:06:26Z

This raw code fixed the bug and does not seem introducing more, but is not the final form of it and not want to commit. I opened the PR now so that you can start make questions or rise concerns or whatever you want to do , in advance

asturur · 2018-10-22T11:08:39Z

@LinusU @zbjornson i understand this code is a bit unclear what is doing.
I would like to work together on this so that:

the code is readabale
drawImage can continue to work on the 9 args call with global composite operation
drawImage creates small temporary surfaces ( no more than the pixels needed to draw )

I understand not everything can be prevented, an drawCall outside the canvas width/height will not draw anything but if needed will still create a tempSurface.
I do not think we should write code to prevent that too, but if an image is 4000x4000 pixels, i would like to be able to create a temp surface that is not bigger than the image itself.

asturur · 2018-12-22T08:40:14Z

this fixes also: #1335

@zbjornson @bennlich

asturur · 2018-12-22T09:38:15Z

@zbjornson @LinusU i understand this PR is not of immediate readability and needs time to understand what is it doing.

Please find some time to read and make me all the questions, so i can move this forward and think about a couple of other things that deserve their own pr.
(further limit the extra surface, re-render SVG on ctx scaling so that it scales correctly )

I added a new test, and also i see improvements in other tests:

this test takes small slices of images and draw them super big ( like 90k x 90k ), this to test the extra surface reduction works ( this tests crashes current master )

this test scale up an svg and show some improvements compared to master:
CURRENT:

MASTER:

the super blurred effect is actually a regression introduced with the code from me, those are extreme edge cases usage of canvas, but those use cases happens with high level libraries sometimes ( fabricJS for example triggers high level of scaling )

zbjornson · 2018-12-22T21:00:26Z

src/CanvasRenderingContext2d.cc

+  // extract the scale value from the current transform so that we know how many pixels we
+  // need for our extra canvas in the drawImage operation.
+  double current_scale_y = transforms[1];
+  double current_scale_x = transforms[2];


Are these flipped? Your code comment above says 1: scaleX, 2: scaleY.

Likewise I think one of the comments on line 1296 or 1297 is supposed to say Y.

I like where this is headed (a systematic approach as opposed to special handling of individual cases) 👍 . Will give a closer read-through once the above are clarified.

yeah i flipped those, trying just with standard scaling i did not notice.

asturur · 2018-12-29T22:55:12Z

@zbjornson i updated the accidentally swapped values. Thanks for spotting that!

asturur · 2019-01-05T22:45:03Z

any update?

zbjornson

Sorry for the long delay. Getting there... Didn't get all the way through it, need to figure out these questions before getting through the rest of it though.

FIrefox's implementation is here if it's useful as a reference: https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#4885. I think this is Chromium's but it's a bit harder to follow: https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/canvas/canvas2d/base_rendering_context_2d.cc?q=drawImage+canvasrenderingcontext2d&sq=package:chromium&dr=CSs&l=1159

zbjornson · 2019-01-21T00:16:14Z

src/CanvasRenderingContext2d.cc

+  // extract the scale value from the current transform so that we know how many pixels we
+  // need for our extra canvas in the drawImage operation.
+  double current_scale_x = transforms[1];
+  double current_scale_y = transforms[2];


I think these values should just be matrix.xx and matrix.yy instead of decomposing the matrix. Try this test case -- it's broken with transforms[1|2] but works with matrix.xx|yy

tests['transformed drawimage'] = function (ctx) { ctx.fillStyle = 'white' ctx.fillRect(0, 0, 200, 200) ctx.fillStyle = 'black' ctx.fillRect(5, 5, 50, 50) ctx.transform(1.2, 1, 1.8, 1.3, 0, 0); ctx.drawImage(ctx.canvas, 0, 0) }

(The sign of scale_y from the decomposed matrix is negative when it should be positive.)

if matrix.xx is always the matrix[0] position, the number that we generally associate with scaleX, that is not enough. When you have a rotation in that position there are mixed values of scaling and angle of rotation, as well as in matrix[3] or matrix.yy.

Now the negative sign, that is a point and should be fixed, let me see if i can have some code that works with any sign ( the scale is the abs value, while the image needs a flip )

Do you have an example test case that fails using just xx and yy? My understanding of this code (which may be wrong) is that both the canvas and the image are rotated together, so xx and yy are all you need. I tried quite a few random matrices.

Also I was unclear on the "negative when it should be positive" bit -- yy was like ~2.0 and transforms[2] was like -0.6 -- not just an abs() would have fixed it.

well i may have done something wrong. i ll triple check. i use this decomposing with success elsewhere in javascript, not in c.

zbjornson · 2019-01-21T00:17:23Z

src/CanvasRenderingContext2d.cc

+  float extra_dx = 0;
+  float extra_dy = 0;
+  float fx = (float) dw / sw * current_scale_x; // transforms[1] is scale on X
+  float fy = (float) dh / sh * current_scale_y; // transforms[2] is scale on X


You don't need this cast; it happens implicitly.

zbjornson · 2019-01-21T00:37:58Z

src/CanvasRenderingContext2d.cc

+  // need for our extra canvas in the drawImage operation.
+  double current_scale_x = transforms[1];
+  double current_scale_y = transforms[2];
+  float extra_dx = 0;


This function (before your PR even) is a weird mix of doubles and floats. They should all be doubles to minimize implicit conversions: the arguments (info array) are doubles, and the cairo APIs that take floating-point numbers take doubles. Can you please change the new variables you're adding in this PR to double, along with the variables declared on line 1208 while you're here?

sure, i can see to clean everything in double.

zbjornson · 2019-01-21T00:40:03Z

src/CanvasRenderingContext2d.cc

+    scaled_dx *= current_scale_x;
+    scaled_dy *= current_scale_y;
+  }
+  // cairo_translate(ctx, extra_dx, extra_dy);


Does this need to be uncommented?

no, deleted.

zbjornson · 2019-01-21T00:47:07Z

src/CanvasRenderingContext2d.cc

  bool needScale = dw != sw || dh != sh;
  bool needCut = sw != source_w || sh != source_h || sx < 0 || sy < 0;
-  bool needCairoClip = sx < 0 || sy < 0 || sw > source_w || sh > source_h;
-
+  bool needAdditionalCut = sx < 0 || sy < 0 || sw > source_w || sh > source_h;


I don't quite understand needCut vs. needAdditionalCut. Both of them test sx < 0 || sy < 0. sw != source_w is also true for all cases where sw > source_w (ignoring NaN, which shouldn't be encountered here).

no with the code where it is now, those booleans can become 1. Probably before there was extra code paths to follow in a case or the other.

@zbjornson @asturur it looks to me like needAdditionalCut is a renaming of needCairoClip, which was introduced to fix #1249 in this related PR: #1251.

I think both this current PR and #1251 are fixes for (among other things) issues that were introduced with the use of CAIRO_EXTEND_REFLECT.

Please excuse comment from left-field here :) I hope it's helpful history.

Yes i introduced both, they can be unified. Before the code had different srategies depending on what was the reason of the cut.

asturur · 2019-02-16T23:41:25Z

add busy period, i ll try to fix what is left

asturur · 2019-03-10T11:11:40Z

@zbjornson i fixed your comment using abs, and i'm trying to demonstrate the reason behind it with this fiddle.

https://jsfiddle.net/gkwdo1ez/3/

So in this fiddle you see an image painted on a canvas using transform with the matrix you suggested.
Then the same image is painted on another canvas decomposing the matrix and then executing the oprations in this order:

ctx.translate, ctx.rotate, ctx.scale, ctx.skewX.

Then pixels of the resulting canvas are compared for differences, and no differences are found.

While is true that matri.xx generally represent the scale, this is no more true when skewing comes in play.

This decomposition aims at finding in the whole transformation, which part is a transform that is pure scaling. In this way we know how much our temporary surface has to be big to avoid detail loss or to avoid creating a giant unnecessary surface.

this can also be verified multiplying back the matrices together still in the fiddle.

now the scaling factor to me are X 1.5 and Y -0.15 ( and indeed the image is flipped somehow ), i see you get different numbers, i wonder if i implemented it bad.

Let me know what you think of this explanation, meanwhile i investigate why number here are different.

asturur · 2019-03-10T13:15:49Z

So i see this code is breaking something. I'm trying to figure out what and why.
But yes is not mergeable right now, but please go over my description and see if it makes sense for you.

Numbers between JS and the C version are the same.

drawing output is not.

asturur · 2019-03-10T13:57:44Z

I think i understand the error.
I cannot just scale back if there is scale.
But i need to swap the ctx transform with one built with the same logic, but without the scaling part.

asturur · 2019-03-10T18:31:17Z

Ok after spending hours on this code i m more confident is not a nest of snakes and spiders.

It works with weird skewing with or without flippings

Let me know, i would like to work on other node-canvas things, but i would like to park settle this change here.

Of course if it does not convince you, let me know and i ll do my best to convince you.

asturur · 2019-03-11T12:15:41Z

@zbjornson @LinusU ready for re-review.

LinusU

I think it looks good, would love to have some more 👀 from the other maintainers before merging though ☺️

zbjornson · 2019-03-13T19:14:06Z

Thanks @asturur. I'm probably not going to have time to review closely until Friday or this weekend, but will get to it ASAP!

asturur · 2019-03-22T11:23:59Z

any news? i would like to start working on my next pr ( svg resizing with scale ) but i do not like having 2 open pr at time.

LinusU · 2019-03-28T19:10:32Z

Pardon the delay, merging 🚀

asturur · 2019-03-28T19:49:24Z

thanks!!

save work

688c0ac

asturur added 5 commits October 14, 2018 22:31

Merge branch 'master' into detail-loss

ce325f8

Merge branch 'master' into detail-loss

6f3d205

middle-way

0e6d712

fixed

2c65fc0

swap a line

cdfa083

asturur closed this Oct 21, 2018

asturur reopened this Oct 22, 2018

asturur added 2 commits December 22, 2018 09:08

Merge branch 'master' into detail-loss

ff64266

saved tests

9808f39

asturur added 2 commits December 22, 2018 10:25

,aybe mergeable

8328af6

,aybe mergeable

bf138bb

removed commented code, added a comment

e5dbbb0

zbjornson reviewed Dec 22, 2018

View reviewed changes

fixed comment

d7912b1

zbjornson mentioned this pull request Jan 10, 2019

Release proposal: 2.3.0 #1349

Closed

zbjornson requested changes Jan 21, 2019

View reviewed changes

asturur added 3 commits March 10, 2019 10:54

Merge branch 'master' into detail-loss

eb0d2db

removed float for doubles

cdffdb4

removed extra comment

72f70b8

asturur force-pushed the detail-loss branch from 0aa8dff to 72f70b8 Compare March 10, 2019 10:06

removed extra boolean

b9ae0a0

asturur added 2 commits March 10, 2019 19:31

added tests

d29b18e

no semicolons

74e7b9c

LinusU approved these changes Mar 11, 2019

View reviewed changes

LinusU mentioned this pull request Mar 12, 2019

Add typescript definitions #1330

Merged

1 task

LinusU merged commit 39aa8ab into Automattic:master Mar 28, 2019

LinusU mentioned this pull request May 8, 2019

drawing image fails if ctx.rotate() precision is too deep #1417

Closed

Initial work to fix detail loss with additional canvas needed on drawImage #1255

Initial work to fix detail loss with additional canvas needed on drawImage #1255

Uh oh!

Conversation

asturur commented Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The problem

Uh oh!

asturur commented Sep 25, 2018

Uh oh!

asturur commented Oct 22, 2018

Uh oh!

asturur commented Dec 22, 2018

Uh oh!

asturur commented Dec 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asturur commented Dec 29, 2018

Uh oh!

asturur commented Jan 5, 2019

Uh oh!

zbjornson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asturur commented Feb 16, 2019

Uh oh!

asturur commented Mar 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asturur commented Mar 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asturur commented Mar 10, 2019

Uh oh!

asturur commented Mar 10, 2019

Uh oh!

asturur commented Mar 11, 2019

Uh oh!

LinusU left a comment

Choose a reason for hiding this comment

Uh oh!

zbjornson commented Mar 13, 2019

Uh oh!

asturur commented Mar 22, 2019

Uh oh!

LinusU commented Mar 28, 2019

Uh oh!

asturur commented Mar 28, 2019

Uh oh!

Uh oh!

asturur commented Sep 25, 2018 •

edited

Loading

asturur commented Mar 10, 2019 •

edited

Loading

asturur commented Mar 10, 2019 •

edited

Loading