Fix drawImage regression with particular use case of 9 arguments. #1251
Conversation
| } | ||
| img1.src = imageSrc('existing.png') | ||
| } | ||
| }) |
There was a problem hiding this comment.
copy of the test where the color effect of the global composite operation is less visible, but the complication of using the drawImage with free 9 args in a transformed context are better visible.
| @@ -1199,6 +1199,7 @@ gco.forEach(op => { | |||
| var img2 = new Image() | |||
| img1.onload = function () { | |||
| img2.onload = function () { | |||
| ctx.globalAlpha = 0.7 | |||
There was a problem hiding this comment.
this will surface better the properties of compositing when images overlap.
|
@LinusU @zbjornson @chearon Do you feel like we should add the particular test case that discovered the bug? |
Yeah, or make a small repro case similar to one of the existing drawImage tests -- I don't think we need another test image added to this repository to reproduce it. |
|
ok all add it. |
|
Added a test similar to the issue: With no fix: with fix: |
|
@LinusU @zbjornson This should be done too. |
src/CanvasRenderingContext2d.cc
Outdated
| @@ -1183,7 +1183,8 @@ NAN_METHOD(Context2d::DrawImage) { | |||
| float fx = (float) dw / sw; | |||
| float fy = (float) dh / sh; | |||
| bool needScale = dw != sw || dh != sh; | |||
| bool needCut = sw != fw || sh != fh; | |||
| bool needCut = sw != source_h || sh != source_h || sx < 0 || sy < 0; | |||
| bool needClip = sx < 0 || sy < 0 || sw > source_w || sh > source_h; | |||
There was a problem hiding this comment.
needCut = sw != source_w ... (you have source_h)
Might also consider renaming needCut since "clip" and "cut" have very similar meanings. ("clip" corresponds to cairo_clip at least.) I don't know what else to call it though.
There was a problem hiding this comment.
yes those are both to address
|
i ll check those cairo groups, i m not very deep into cairo |
|
@zbjornson i changed needClip in needCairoClip, is the only thing i could do to differentiate clip and cut. Some of this clip and cut logic will be reworked in my next pr that should address the loss of details. |
|
Looks good, thanks! Don't worry too much about groups. I think the only ("only") benefit they bring is a small, potential performance improvement, and possibly neater code. If you figure them out, great. :) Travis got set to not build pull requests somehow. I just turned that back on, but I don't see a way to trigger a build for this PR. (The tests pass locally though.) Will leave open for another ~day in case anyone else wants to look. |
|
Backported to v1.x in a0af588. |
|
Released as:
|
So this should fix 95% of the cases, and almost all real usage cases.
I think my code introduced another regression that i have no time to verify and i ll verify ( and in case fix ) as soon as possible, is NOT visible from the screenshots, when a scaling is applied on the canvas, and is not considered in drawImage code with the extra canvas, some detail loss will happen. I have an idea how to fix this, but i think should be a different PR.
This will happen only when a small destination size is specified but with upscaled receiving context.
Is a weird use case that will likely happen on higher level libraries like fabricJS and not on direct canvas usage ( since the pattern is a bit strange )
I changed a bit the tests, i should reverify them with the 1.6.9 to be sure the failing one are failing for my code or also before.
I set the globalAlpha of the globalCompositeOperation tests lower than 1 so that particular interaction can be noted.
NEW BUG UNRELATED WITH THIS FIX
something is wrong with the copy version
saturation is not perfect yet
FIXES RELATED TO THIS PR
TESTS RESULT WITH MASTER CODE
.... and so on ....
IMAGE SAMPLING TEST
This test is the reason why we need to use an extend in first place and works fine.
zbjornson edited to put all the images in
<details>