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

ext/gd: bug #80828 HEIF support. #14504

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

devnexen
Copy link
Member

@devnexen devnexen commented Jun 8, 2024

No description provided.

@@ -54,6 +55,7 @@
#define PHP_IMG_BMP 64
#define PHP_IMG_TGA 128
#define PHP_IMG_AVIF 256
#define PHP_IMG_HEIF 384
Copy link
Member

Choose a reason for hiding this comment

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

Why is this 384 instead of 512? I didn't understand this.

@@ -1408,6 +1418,11 @@ PHPAPI int php_getimagetype(php_stream *stream, const char *input, char *filetyp
return IMAGE_FILETYPE_IFF;
} else if (!memcmp(filetype, php_sig_ico, 4)) {
return IMAGE_FILETYPE_ICO;
} else if (!memcmp(filetype, php_sig_heifheic, 12) ||
Copy link
Member

Choose a reason for hiding this comment

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

The comment above indicates that only 4 bytes have been read, and a cursory glance shows me that indeed that is the case. So this memcmp won't do the job, I think you need to read additional bytes.

Copy link
Member Author

@devnexen devnexen Jun 8, 2024

Choose a reason for hiding this comment

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

AFAIK, it s far from finished, e.g. gd does not support all HEIF formats (e.g. multi image format). imagecreatefromstring overall for this not gonna work for now (I wrote an email on internals).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I went here because I saw your email, I guess I should've waited a bit before reading the code.

@cmb69
Copy link
Member

cmb69 commented Jul 27, 2024

Frankly, I'm not convinced that we should add HEIF support. Although that has obviously been merged into libgd, it has not been released yet. Furthermore, HEIF is only supported by Safari, and I presume that it will stay this way. Now, PHP is not only useful for Web applications, but still, it is mostly used on the Web. And there is JPEG XL, and surely next year there'll be the next big image format. And if we generally support HEIF, users will soon request Windows support for it, and I remember the PITA building AVIF on Windows, and apparently that is even broken yet.

To be clear, I'm not strictly against adding HEIF support, but we should be aware that this may open up a can of worms, for limited usefulness (especially given that Safari appears in the process of supporting AVIF with new releases).

And after all, Gif ought to be enough for anybody. ;)

@adamsilverstein
Copy link

To be clear, I'm not strictly against adding HEIF support, but we should be aware that this may open up a can of worms, for limited usefulness (especially given that Safari appears in the process of supporting AVIF with new releases).

This would be especially useful for WordPress (or other PHP CMSs like Drupal) to be able to add HEIC image support (to enable converting uploaded HEICs to a web-safe format like JPEG). See my comment on the related ticket.

@ziegenberg
Copy link

Having support for HEIF would prevent issues like the following from arising: https://tracker.moodle.org/browse/MDL-79819

We have students uploading PDFs (containing HEIF images) and HEIF images into Moodle and TCPDF trips over its feet because it tries to call getimagesize() on the HEIF file.

@cmb69
Copy link
Member

cmb69 commented Sep 25, 2024

Note that HEIF/HEIC for getimagesize() and for ext-gd are not necessarily the same. For getimagesize() we likely need to bundle at least parts of the HEIF libraries (see e.g. https://github.com/php/php-src/tree/master/ext/standard/libavifinfo, and note the sentence about the easy update and the last commit to this directory). For HEIF/HEIC support in ext-gd, we can build against an existing system libheif (or alternative); that would only be an issue for our Windows builds.

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

Successfully merging this pull request may close these issues.

5 participants