Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a failing test for vips::VError exception #399

Merged
merged 3 commits into from
Apr 8, 2016

Conversation

salzhrani
Copy link
Contributor

There is a discrepancy between the raw buffer size that is emitted and expected. it is my understanding that a raw image buffer size should be the product of width * height * channels but it seems that there is a difference in rounding of the image dimensions. in the specific case in this test the buffer length should be 840 * 473 * 3 = 1191960 but instead 1189440 is emitted which seems to be the product of 840 * 472 * 3

@salzhrani salzhrani changed the title Add failed test for vips::VError exception Add a failing test for vips::VError exception Apr 6, 2016
@lovell
Copy link
Owner

lovell commented Apr 6, 2016

Hi Samy, there was a recent problem highlighted by #387 (comment) where it was possible to get incorrect values for info.width and info.height passed to the callback. I think this might be related, in that info.size is correct but info.height is off-by-one due to a rounding that takes place after the height is calculated.

If so, the fix here is probably to ensure the width and height are based on the actual dimensions of the final output image rather than an intermediate calculation.

@salzhrani
Copy link
Contributor Author

It might be best if vips is the single source of image metadata if possible

@lovell
Copy link
Owner

lovell commented Apr 7, 2016

I agree, thanks Samy. Setting width and height on the baton approximately here should fix this. Happy to accept a PR for this if you're willing and able :)

@salzhrani
Copy link
Contributor Author

Thanks for the opportunity ... I also took the opportunity to add a try/catch.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.832% when pulling cff1602 on salzhrani:failed-test into f214269 on lovell:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.832% when pulling 053ba09 on salzhrani:failed-test into f214269 on lovell:master.

@lovell
Copy link
Owner

lovell commented Apr 7, 2016

This looks great, thanks for the work on this.

What's the origin of the flower.jpeg test image? Are you able to add a source URL (you'll see these against most of the other test fixtures), assuming it's been made available for us to use under a suitable licence?

@salzhrani
Copy link
Contributor Author

The image can be found here. Not sure about the licence. I can replace it with a another with the same dimensions and more explicit license if you want.

@salzhrani
Copy link
Contributor Author

Added a blank image with the correct dimensions.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.832% when pulling d9b6993 on salzhrani:failed-test into f214269 on lovell:master.

@lovell lovell merged commit ef61da3 into lovell:master Apr 8, 2016
@lovell
Copy link
Owner

lovell commented Apr 8, 2016

Fantastic work, than you very much for this Samy!

@lovell lovell added the bug label Apr 8, 2016
@lovell lovell added this to the v0.14.1 milestone Apr 8, 2016
@salzhrani salzhrani deleted the failed-test branch April 8, 2016 08:05
@salzhrani
Copy link
Contributor Author

@lovell If appropriate please cut us a 0.14.1 release. It seems all issues on the milestone are covered.

@lovell
Copy link
Owner

lovell commented Apr 16, 2016

@salzhrani v0.14.1 now available via npm, thanks for your additions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants