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

Unit test: AVIF image not supported #1518

Closed
mukeshpanchal27 opened this issue Sep 3, 2024 · 23 comments
Closed

Unit test: AVIF image not supported #1518

mukeshpanchal27 opened this issue Sep 3, 2024 · 23 comments
Labels
[Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Bug An existing feature is broken

Comments

@mukeshpanchal27
Copy link
Member

Bug Description

In unit tests, all tests that check AVIF support are being skipped because AVIF support appears to be non-functional in the test environment. However, when I checked AVIF support on the test environment (http://localhost:8889), it indicates that "Your site supports AVIF."

The unit tests being skipped can be seen here: https://github.com/WordPress/performance/actions/runs/10677521269/job/29592825477#step:9:310. Additionally, in #1517, the AVIF support check shows that it failed.

@mukeshpanchal27 mukeshpanchal27 added [Type] Bug An existing feature is broken [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) labels Sep 3, 2024
@github-project-automation github-project-automation bot moved this to Not Started/Backlog 📆 in WP Performance 2024 Sep 3, 2024
@mukeshpanchal27
Copy link
Member Author

@adamsilverstein Similar discuss going here https://core.trac.wordpress.org/ticket/60628

@adamsilverstein
Copy link
Member

Hmm, I'm also seeing other image formats that also "don't have support" which seems wrong, eg:

Mime type image/jpeg is not supported.

However, when I checked AVIF support on the test environment (http://localhost:8889), it indicates that "Your site supports AVIF."

@mukeshpanchal27 this is standard wp-env, with npm run wp-env:start, right? Can you try uploading an AVIF image and see if subsizes are created to test for actual support? I will test on my end as well.

@adamsilverstein
Copy link
Member

Test run for me locally against my MAMP install (some are failing though), trying with wp-env next.

@adamsilverstein
Copy link
Member

Testing with wp-env on my chromebook, I see that Imagick shows avif support on the Tools->Site Health page, but when I upload an AVIF, the srcset images are jpegs. Still trying to see what is happening there, will update here.

@adamsilverstein
Copy link
Member

This might actually be a core support detection bug? I thought it might be this line:

https://github.com/WordPress/wordpress-develop/blob/d525818f76607c7bfabaded787cf98e97268ed67/src/wp-includes/class-wp-image-editor-imagick.php#L109

will return false for support for non jpeg images if setIteratorIndex is not available. We should probably limit that to just gif images we KNOW are animated.

However, checking the wp-env build, we do have setIteratorIndex so this isn't the issue apparently. Still checking...

@mukeshpanchal27
Copy link
Member Author

@adamsilverstein

@mukeshpanchal27 this is standard wp-env, with npm run wp-env:start, right?

Yes

I spend some time to check why unit tests didn't run the AVIF mime image. Here is some of my finding.

Modern Image plugin with WP_ENV

For the AVIF mime image https://github.com/WordPress/wordpress-develop/blob/d525818f76607c7bfabaded787cf98e97268ed67/src/wp-includes/class-wp-image-editor-imagick.php#L115 return empty array. Further i check and found that in Unit test if we check var_dump( Imagick::getVersion() ) it show me

array(2) {
  ["versionNumber"]=> 1809
  ["versionString"]=>ImageMagick 7.1.1-26 Q16-HDRI aarch64 21914 https://imagemagick.org
}

while if i check version in WP site health is show

array(2) {
  ["versionNumber"]=> 1691
  ["versionString"]=> ImageMagick 6.9.11-60 Q16 aarch64 2021-01-25 https://imagemagick.org
}

@adamsilverstein
Copy link
Member

while if i check version in WP site health is show

I am also seeing a different environment when running wp-env locally vs. what appears to run in our action. I was able to get this set up for testing on a personal computer where I can run Docker. I suspect the fix here may be updating the environment we use for testing.

@adamsilverstein adamsilverstein self-assigned this Sep 9, 2024
@mukeshpanchal27
Copy link
Member Author

Per our unit tests setup https://github.com/WordPress/performance/blob/trunk/package.json#L52 it will runs on tests-cli container and if i check the var_dump( Imagick::getVersion() ); then it shows 7.1.1-26 and var_dump( Imagick::queryFormats( '*' ) ); return empty array.

See screenshot for more details:
Screenshot 2024-09-10 at 10 30 17 AM

cc. @swissspidy

@swissspidy
Copy link
Member

So the two Docker images have different ImageMagick versions, is that what you‘re both saying?

@mukeshpanchal27
Copy link
Member Author

So the two Docker images have different ImageMagick versions, is that what you‘re both saying?

Yes. Also the var_dump( Imagick::queryFormats( '*' ) ); return empty array in CLI so the unit tests for the mime type is skipped.

@swissspidy
Copy link
Member

The CLI containers (which are maintained at https://github.com/docker-library/wordpress) use Alpine and only contain the bare minimum needed.

I suspect libheif-dev (yes, heif) needs to be installed here alongside libwebp-dev & co to add AVIF support:

https://github.com/docker-library/wordpress/blob/3266f784485789a1ae1414eaeb0a87a95956a3cc/cli/php8.3/alpine/Dockerfile#L21-L47

Same for the other PHP versions.

@mukeshpanchal27
Copy link
Member Author

Thanks for sharing.

They already added warning in docker file. Do we needs to close this ticket with closed as not plan or any other way to fix this issue?

# WARNING: imagick is likely not supported on Alpine: https://github.com/Imagick/imagick/issues/328
# https://pecl.php.net/package/imagick
# https://github.com/Imagick/imagick/commit/5ae2ecf20a1157073bad0170106ad0cf74e01cb6 (causes a lot of build failures, but strangely only intermittent ones 🤔)
# see also https://github.com/Imagick/imagick/pull/641
# this is "pecl install imagick-3.7.0", but by hand so we can apply a small hack / part of the above commit

@swissspidy
Copy link
Member

swissspidy commented Sep 10, 2024

I understand this comment just as "There is no official support for Alpine, but we're trying to install imagick ourselves anyway".

Even the referenced issue Imagick/imagick#328 literally says "To be clear it works, but I don't want to spend time on it"

any other way to fix this issue?

As I said, I suspect libheif-dev needs to be installed here alongside libwebp-dev & co to add AVIF support. That means a PR could be opened on that Docker repo with a change as follows:

# install the PHP extensions we need (https://make.wordpress.org/hosting/handbook/handbook/server-environment/#php-extensions)
+# Note: imagick uses libheif for AVIF support, whereas GD uses libavif for AVIF support
RUN set -ex; \
	\
	apk add --no-cache --virtual .build-deps \
		$PHPIZE_DEPS \
		freetype-dev \
		icu-dev \
		imagemagick-dev \
		libjpeg-turbo-dev \
		libpng-dev \
		libwebp-dev \
+		libheif-dev \
+		libavif-dev \
		libzip-dev \
	; \
	\
	docker-php-ext-configure gd \
		--with-freetype \
		--with-jpeg \
		--with-webp \
+		--with-avif \
	; \
	docker-php-ext-install -j "$(nproc)" \
		bcmath \
		exif \
		gd \
		intl \
		mysqli \
		zip \
	; \

@mukeshpanchal27
Copy link
Member Author

Thanks. Open PR docker-library/wordpress#919

@mukeshpanchal27
Copy link
Member Author

The PR was approved with some minor feedback that I addressed, but I'm unable to run ./apply-templates.sh to get the CI passing. I’ve asked the maintainer for help with it, so it should be landed soon 🎉.

@mukeshpanchal27 mukeshpanchal27 moved this from In Progress 🚧 to Code Review 👀 in WP Performance 2024 Sep 12, 2024
@joemcgill
Copy link
Member

Looks like this was recently merged. Amazing work all!

@mukeshpanchal27
Copy link
Member Author

mukeshpanchal27 commented Sep 13, 2024

The unit tests still failing 🤔 Do we needs to wait for sometime to get new changes?

https://github.com/WordPress/performance/actions/runs/10842661947/job/30088605828?pr=1517

@swissspidy
Copy link
Member

I don‘t have Docker so I can‘t test it, but were those updates already released? When you create a new env locally it gets the new version?

If so, then there might be more build changes needed

@mukeshpanchal27
Copy link
Member Author

I followed up on the Docker PR to get more information on when it will be released, as I’m still getting the same result during testing.

Screenshot 2024-09-16 at 9 14 12 AM

@swissspidy
Copy link
Member

It sounded like it was already released, no? Can you compare your image hash with the latest available one?

Could also be that the PR wasn‘t sufficuent to add support and that more changes are needed.

@mukeshpanchal27
Copy link
Member Author

mukeshpanchal27 commented Sep 17, 2024

It seems like it working 🥳 for some PHP versions 8.1 to 8.3 but not for some old PHP versions

Previously skipped tests

https://github.com/WordPress/performance/actions/runs/10681470364/job/29605168994

There were 9 skipped tests:

1) Test_WebP_Uploads_Image_Edit::test_it_should_prevent_to_backup_the_full_size_image_if_only_the_thumbnail_is_edited
Editing image thumbnails separately is disabled

/var/www/html/wp-content/plugins/performance/plugins/webp-uploads/tests/test-image-edit.php:143

2) Test_WebP_Uploads_Image_Edit::test_it_should_backup_the_image_when_all_images_except_the_thumbnail_are_updated
Editing image thumbnails separately is disabled

/var/www/html/wp-content/plugins/performance/plugins/webp-uploads/tests/test-image-edit.php:196

3) Test_WebP_Uploads_Image_Edit::test_it_should_use_the_attached_image_when_updating_subsequent_images_not_the_original_version
Editing image thumbnails separately is disabled

/var/www/html/wp-content/plugins/performance/plugins/webp-uploads/tests/test-image-edit.php:239

4) Test_WebP_Uploads_Image_Edit::test_it_should_validate_source_attribute_update_when_webp_edited
Editing image thumbnails separately is disabled

/var/www/html/wp-content/plugins/performance/plugins/webp-uploads/tests/test-image-edit.php:295

5) Test_WebP_Uploads_Load::test_it_should_not_create_the_original_mime_type_for_jpeg_images with data set "avif" ('avif')
Mime type image/avif is not supported.

/var/www/html/wp-content/plugins/performance/plugins/webp-uploads/tests/test-load.php:52

6) Test_WebP_Uploads_Load::test_it_should_create_jpeg_and_webp_for_jpeg_images_if_opted_in with data set "avif" ('avif')
Mime type image/avif is not supported.

/var/www/html/wp-content/plugins/performance/plugins/webp-uploads/tests/test-load.php:117

7) Test_WebP_Uploads_Load::test_it_should_create_jpeg_and_webp_for_jpeg_images_if_generate_webp_and_jpeg_set with data set "avif" ('avif')
Mime type image/avif is not supported.

/var/www/html/wp-content/plugins/performance/plugins/webp-uploads/tests/test-load.php:154

8) Test_WebP_Uploads_Load::test_it_should_allow_the_upload_of_a_webp_image_if_at_least_one_editor_supports_the_format with data set "avif" ('avif')
No editors support the image type: avif

/var/www/html/wp-content/plugins/performance/plugins/webp-uploads/tests/test-load.php:686

9) Test_WebP_Uploads_Load::test_it_should_replace_the_featured_image_to_webp_when_requesting_the_featured_image with data set "avif" ('avif')
The image editor does not support the image type: avif

/var/www/html/wp-content/plugins/performance/plugins/webp-uploads/tests/test-load.php:726

Currently skipped tests

https://github.com/WordPress/performance/actions/runs/10897272598/job/30238408456#step:9:291

There were 4 skipped tests:

1) Test_WebP_Uploads_Image_Edit::test_it_should_prevent_to_backup_the_full_size_image_if_only_the_thumbnail_is_edited
Editing image thumbnails separately is disabled

/var/www/html/wp-content/plugins/performance/plugins/webp-uploads/tests/test-image-edit.php:143

2) Test_WebP_Uploads_Image_Edit::test_it_should_backup_the_image_when_all_images_except_the_thumbnail_are_updated
Editing image thumbnails separately is disabled

/var/www/html/wp-content/plugins/performance/plugins/webp-uploads/tests/test-image-edit.php:196

3) Test_WebP_Uploads_Image_Edit::test_it_should_use_the_attached_image_when_updating_subsequent_images_not_the_original_version
Editing image thumbnails separately is disabled

/var/www/html/wp-content/plugins/performance/plugins/webp-uploads/tests/test-image-edit.php:239

4) Test_WebP_Uploads_Image_Edit::test_it_should_validate_source_attribute_update_when_webp_edited
Editing image thumbnails separately is disabled

/var/www/html/wp-content/plugins/performance/plugins/webp-uploads/tests/test-image-edit.php:[295](https://github.com/WordPress/performance/actions/runs/10897272598/job/30238408456#step:9:296)

@mukeshpanchal27
Copy link
Member Author

It seems like still we needs to add support for PHP 8.0 and 7.X version. See #1517 PR for more info.

@mukeshpanchal27
Copy link
Member Author

Per docker image maintainer reply:

PHP 8.0 and PHP 7.x are EOL, so unfortunately are not and will not be updated further (for this or any other changes 😬)

I'm going to close the issue and related PR as AVIF is works for 8.1 and higher versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Bug An existing feature is broken
Projects
Status: Done 😃
Development

No branches or pull requests

4 participants