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

Let libvips check whether we received a valid image or not #166

Merged
merged 1 commit into from Mar 1, 2015

Conversation

mcuelenaere
Copy link
Contributor

This removes the custom image fingerprinting code and uses the libvips
is_a_buffer() infrastructure instead.

This requires https://github.com/jcupitt/libvips/pull/240.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.35%) to 97.79% when pulling 503ad29 on mcuelenaere:no-custom-image-header-checking into 1f7e80e on lovell:master.

@lovell
Copy link
Owner

lovell commented Feb 15, 2015

Thanks again; loving these changes.

This PR slightly changes the API in that the constructor no longer fails fast with an unsupported/invalid Buffer. Perhaps we should verify the Buffer synchronously?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.35%) to 97.79% when pulling 75e27c1 on mcuelenaere:no-custom-image-header-checking into 1f7e80e on lovell:master.

@mcuelenaere
Copy link
Contributor Author

Yes, that's the only controversial change. The only downside to doing it synchronously is that it might cause a possible performance hit. But I suppose that doing a couple of memory comparisons won't be that slow.

@lovell
Copy link
Owner

lovell commented Feb 15, 2015

The more I think about about, the more I'd rather "break" the API and keep your change as it is.

I'd describe it as very useful rather than "controversial" :)

I'll wait for John to merge in the related libvips change before merging this PR. Thanks again!

/*
Determine image format of a buffer.
*/
ImageType DetermineImageType(void *buffer, size_t const length) {
ImageType imageType = ImageType::UNKNOWN;
if (length >= 4) {
if (memcmp(MARKER_JPEG, buffer, 2) == 0) {
#if (VIPS_MAJOR_VERSION >= 7 && VIPS_MINOR_VERSION >= 42 && VIPS_MICRO_VERSION > 3)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this condition now needs to use VIPS_MAJOR_VERSION >= 8.

@lovell
Copy link
Owner

lovell commented Feb 27, 2015

Hi @mcuelenaere, John's merged the dependent PR. Are you able to rebase your mcuelenaere:no-custom-image-header-checking branch against upstream/master and squash to a single commit?

@mcuelenaere mcuelenaere force-pushed the no-custom-image-header-checking branch from 75e27c1 to 2da4d7c Compare March 1, 2015 10:50
This removes the custom image fingerprinting code and uses the libvips
is_a_buffer() infrastructure instead.
@mcuelenaere mcuelenaere force-pushed the no-custom-image-header-checking branch from 2da4d7c to 125ee83 Compare March 1, 2015 10:53
@mcuelenaere
Copy link
Contributor Author

Changed, squashed and rebased.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.34%) to 97.85% when pulling 125ee83 on mcuelenaere:no-custom-image-header-checking into 3ca2f00 on lovell:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.34%) to 97.85% when pulling 125ee83 on mcuelenaere:no-custom-image-header-checking into 3ca2f00 on lovell:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.34%) to 97.85% when pulling 125ee83 on mcuelenaere:no-custom-image-header-checking into 3ca2f00 on lovell:master.

@lovell
Copy link
Owner

lovell commented Mar 1, 2015

Marvellous, thank you Maurus.

lovell added a commit that referenced this pull request Mar 1, 2015
Let libvips check whether we received a valid image or not
@lovell lovell merged commit 5240eeb into lovell:master Mar 1, 2015
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.

None yet

3 participants