Skip to content

windows jpeg support #841

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

Merged
merged 1 commit into from
May 3, 2017
Merged

windows jpeg support #841

merged 1 commit into from
May 3, 2017

Conversation

timknip
Copy link

@timknip timknip commented Nov 8, 2016

Cleaned up replacement for PR #815

@timknip timknip mentioned this pull request Nov 8, 2016
@skrat
Copy link

skrat commented Nov 18, 2016

Any update on this one? Can it be merged?

@zbjornson
Copy link
Collaborator

👍 Looks good.

Builds and tests pass on Win 10, VC2015, libjpeg-turbo-1.5.1 x64.
Builds without error without libjpeg-turbo.

@LinusU
Copy link
Collaborator

LinusU commented Nov 22, 2016

Awesome 👍

Should this be backported to 1.x also? (I can fix it)

Hopefully I'll get some time this week to merge a few of the ready PRs, including this :)

@zbjornson
Copy link
Collaborator

It's non-breaking, so seems suited to 1.x, yes.

Thanks!

@zbjornson
Copy link
Collaborator

(I used libjpeg-turbo-1.5.1-vc64.exe 2016-09-21 from your link, which has bin/libjpeg62.dll)

@@ -880,7 +880,11 @@ cairo_status_t
Image::loadJPEG(FILE *stream) {
cairo_status_t status;

#if defined(_MSC_VER)
if (false) { // Force using loadJPEGFromBuffer
#else
if (data_mode == DATA_IMAGE) { // Can lazily read in the JPEG.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not put the #if defined(_MSC_VER) inside of this this if-statment and avoid if (false) { and duplicating if (data_mode == DATA_IMAGE) {?

Copy link
Author

Choose a reason for hiding this comment

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

@LinusU To prevent repeating the buffer reading stuff https://github.com/Automattic/node-canvas/blob/master/src/Image.cc#L901-L911
Can of course refactor if you wish. Let me know.

@mohamedmansour
Copy link

@LinusU what needs to be done to integrate this? If @timknip is busy, I can help out with anything else

@LinusU
Copy link
Collaborator

LinusU commented May 3, 2017

Sorry for the delay on this, merging now 👍

@LinusU LinusU merged commit c810f8f into Automattic:master May 3, 2017
@LinusU LinusU mentioned this pull request May 3, 2017
@timknip
Copy link
Author

timknip commented May 6, 2017

@LinusU Great! Thanks.

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.

6 participants