Skip to content

fix crop with downsamplingRatio#85

Merged
blueimp merged 1 commit into
blueimp:masterfrom
fleg:master
Feb 16, 2017
Merged

fix crop with downsamplingRatio#85
blueimp merged 1 commit into
blueimp:masterfrom
fleg:master

Conversation

@fleg

@fleg fleg commented Feb 13, 2017

Copy link
Copy Markdown
Contributor

It not actually broken, because I can't get canvas data (Tainted canvases may not be exported).
I append cropped image to body, and it transparent, but it should be black.

I think, bug in renderImageToCanvas calling with non-zero sourceX and sourceY in cycle, and image just moving out beyond the border of canvas.

If you agree, I can fix it, but I don't know how to check content of cropped image in test

@blueimp

blueimp commented Feb 15, 2017

Copy link
Copy Markdown
Owner

Thanks for your contribution, @fleg

To check the content of the image, you'll need to retrieve the canvas Image data.
You'll get back an ImageData object with a data property representing a one-dimensional array containing the data in the RGBA order, with integer values between 0 and 255 (included).

@fleg

fleg commented Feb 15, 2017

Copy link
Copy Markdown
Contributor Author

getImageData throws error in browser (because of file:/// protocol), but if perfectly working in phantomjs, and now test failing

@fleg

fleg commented Feb 15, 2017

Copy link
Copy Markdown
Contributor Author

I fix scale, but it hard to properly test this feature with single colored image

@fleg fleg changed the title add broken test case for crop with downsamplingRatio fix crop with downsamplingRatio Feb 15, 2017
@blueimp

blueimp commented Feb 15, 2017

Copy link
Copy Markdown
Owner

Thanks, I think your test is sufficient.

Just one note regarding your fix:
Instead of introducing another variable (cropped), please simply add the following two lines after the first loadImage.renderImageToCanvas call in the downsampling loop:

          sourceX = 0
          sourceY = 0

After this change, please squash your commits into one (or two if you want to add the test separately).
Then I'll merge your changes gladly. :)

@fleg

fleg commented Feb 15, 2017

Copy link
Copy Markdown
Contributor Author

done

@blueimp

blueimp commented Feb 16, 2017

Copy link
Copy Markdown
Owner

Perfect, thanks!

@blueimp blueimp merged commit 42028c8 into blueimp:master Feb 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants