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 draft support to retrieve Exif from HEIF file #13443

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

Conversation

benstonezhang
Copy link

@benstonezhang benstonezhang commented Feb 20, 2024

Test passed on HEIC file retrieve from iPhoneSE3 and some pictures get from internet.

The attachments are patch files for version 8.1 and 8.2
php_8.1_exif_heif.patch
php_8.2_exif_heif.patch

@jorgsowa
Copy link
Contributor

Hey, @benstonezhang!
You have some tests failing in the pipeline from the extension you modified. Also, it would be cool to always include the tests for your changes to the PRs. In this case, remember about the license of the HEIF file you would upload.

@benstonezhang
Copy link
Author

@jorgsowa Is there any test about exif I can use as reference? I'm not family with php.

@benstonezhang
Copy link
Author

benstonezhang commented Feb 21, 2024

@jorgsowa I added one test case for heif, please check. The image is just taken by my phone's camera.

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I will need to do a bit more research but initially it looks good except few smaller things that I commented on.

ext/standard/image.c Outdated Show resolved Hide resolved
ext/standard/image.c Outdated Show resolved Hide resolved
ext/exif/exif.c Outdated Show resolved Hide resolved
ext/exif/exif.c Outdated Show resolved Hide resolved
ext/exif/exif.c Outdated Show resolved Hide resolved
@jorgsowa
Copy link
Contributor

I think the last missing piece is to add the entry to the NEWS file.

@bukka
Copy link
Member

bukka commented Mar 17, 2024

Well this still needs a proper review. I have got it on my TODO list. It might take couple of weeks though.

Copy link
Author

@benstonezhang benstonezhang left a comment

Choose a reason for hiding this comment

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

seems I need to submit it

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

I had quick look throught and it will still need some testing.

That said I'm actually wondering how useful this really is. There's actually related PR to add HEIF support for ext/gd: #14504 . Last comment from @cmb69 questioned the usefulness of HEIF support in GD. I'm actually wondering how much in use this is. I just shared some photos from my iphone (15 pro) and it gets always converted either to gif or jpg so it doesn't like new iPhones make it easy to get the images in the HEIF. Could you clarify the use case for this please?

@@ -4285,11 +4297,128 @@ static bool exif_process_IFD_in_TIFF(image_info_type *ImageInfo, size_t dir_offs
return result;
}

static int exif_isobmff_parse_box(unsigned char *buf, isobmff_box_type *box)
{
box->size = php_ifd_get32u(buf, 1);
Copy link
Member

Choose a reason for hiding this comment

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

is it supposed to be always little endian for heif?

item_count = php_ifd_get32u(p, 1);
p += 4;
}
for (i=0, p2=p; i<item_count; i++, p2 += 16) {
Copy link
Member

Choose a reason for hiding this comment

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

CS: missing spaces before and after = and <

@benstonezhang
Copy link
Author

I had quick look throught and it will still need some testing.

That said I'm actually wondering how useful this really is. There's actually related PR to add HEIF support for ext/gd: #14504 . Last comment from @cmb69 questioned the usefulness of HEIF support in GD. I'm actually wondering how much in use this is. I just shared some photos from my iphone (15 pro) and it gets always converted either to gif or jpg so it doesn't like new iPhones make it easy to get the images in the HEIF. Could you clarify the use case for this please?

Please check you iphone "Settings -> Photos -> TRANSFER TO MAC OR PC -> Keep Originals". If you set is as "Automatic", all the photos will convert to JPG.

@adamsilverstein
Copy link

I had quick look throught and it will still need some testing.

That said I'm actually wondering how useful this really is. There's actually related PR to add HEIF support for ext/gd: #14504 . Last comment from @cmb69 questioned the usefulness of HEIF support in GD. I'm actually wondering how much in use this is. I just shared some photos from my iphone (15 pro) and it gets always converted either to gif or jpg so it doesn't like new iPhones make it easy to get the images in the HEIF. Could you clarify the use case for this please?

Dropping in to provide a use case for this...

As a media component maintainer for the WordPress project, I can add that we regularly get support requests from users who have HEIC image files on their computer and try to upload them to their WordPress site. I understand that iPhones can be set to copy files over as JPEG (and that might even be the default?) - nonetheless, HEIC images are widespread already and people do try to upload them to use on their sites.

Until recently, WordPress would just throw an error when users uploaded these images. In this ticket we added support for converting these uploaded files into JPEG or WebP images, as long as the system has Imagick installed with HEIC support. If PHP's GD supported HEIF directly, we would able to provide this feature for those users as well..

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.

4 participants