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

Closes #796: Auto AVIF Convertion on settings save #811

Merged
merged 8 commits into from
Feb 28, 2024

Conversation

Miraeld
Copy link
Contributor

@Miraeld Miraeld commented Feb 23, 2024

Description

Fixes #796

Documentation

User documentation

When uploading images to wordpress, when the latest is over a certain dimension, WordPress will resize it and named it -scaled. Originally, we were doing a backup of the Original image, while on the bulk optimization it's trying to reach the -scaled version as WordPress takes it as default.
So we've added the -scaled version to be backed-up.

Technical documentation

Before trying to save the -scaled version of the image in backup, we check if it exists. Same with the deletion of the image.

Type of change

  • Bug fix (non-breaking change which fixes an issue).

Checklists

Feature validation

  • I validated all the Acceptance Criteria. If possible, provide sreenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.

Code style

  • I wrote self-explanatory code about what it does.
  • I wrote comments to explain why it does it.
  • I named variables and functions explicitely.
  • I protected entry points against unexpected inputs.
  • I did not introduce unecessary complexity.

Observability

  • I handled errors when needed.

@Miraeld Miraeld added the AVIF Avif branch/feature label Feb 23, 2024
@Miraeld Miraeld added this to the 2.2 milestone Feb 23, 2024
@Miraeld Miraeld self-assigned this Feb 23, 2024
@Miraeld Miraeld changed the base branch from develop to feature/avif February 23, 2024 04:20
@Miraeld Miraeld force-pushed the fix/796-convert-avif-on-save-big-image branch from e32ea03 to 3674096 Compare February 23, 2024 04:33
@Miraeld
Copy link
Contributor Author

Miraeld commented Feb 23, 2024

@Mai-Saad ,
The problem we encountered with big images like this was the fact that WordPress is resizing the image and name it -scaled, so for example, for an image bigimage.jpg it would become bigimage-scaled.jpg. In the DB, the -scaled version is saved.

So while trying to get back the IDs of unoptimized medias while running the bulk optimization, it wouldn't keep it as the backup file does not exists.

To counter that issue, I've added the scaled version to the backup, as that's the main one Wordpress is using, and the bulk optimization will work without trouble.

I've also taken care of the deletion of the image, when we delete the image, it will delete both backup, the original and the scaled version.

@Mai-Saad
Copy link

Mai-Saad commented Feb 26, 2024

@Miraeld Thanks for the PR, now AVIF is created for large image with the following notes.
1- if the image have webp before enable avif, we are deleting the webp version before generating the avif (this is not happening with small image, here we add avif and keep webp there). @piotrbak shouldn't it be same as with small image?
2- if we uploaded image(s) before activating imagify. Those images won't be automatically processed after activate imagify and enable AVIF. @piotrbak are we ok here or we need separate GH?
3- if internet went down while optimization is in-progress then once back, it won't be resumed automatically. (probably same on trunk, needs further investigation and may need separate GH)

@piotrbak
Copy link

@Mai-Saad @Miraeld 1st and 2nd point should be fixed.

@Miraeld
Copy link
Contributor Author

Miraeld commented Feb 26, 2024

Hello hello,

@Mai-Saad @piotrbak ,
for the 1st point, it's been fixed in the before last commit.
2nd point is fixed by the last commit.

@Miraeld Miraeld requested a review from a team February 26, 2024 13:35
@Mai-Saad
Copy link

Mai-Saad commented Feb 27, 2024

@Miraeld Thanks for the update, compared to trunk and feature/avif branch, we will have regression here:
1- Create avif is enable
2- upload image => image have avif created
3- disable avif
4- permanently delete the image from media library => avif won't be removed from upload folder (same will occur if webp image was created while avif is enabled and we try to permanently delete the image)
can you please check?

@piotrbak
Copy link

@Miraeld Should it be moved to the QA again? Or it needs another CR?

Adds a parameters to delete next-gen files with all format or current one.

Fix the button `Delete permanently`
@Miraeld Miraeld force-pushed the fix/796-convert-avif-on-save-big-image branch from 8fee66f to 9731a75 Compare February 27, 2024 14:35
@Miraeld Miraeld force-pushed the fix/796-convert-avif-on-save-big-image branch from 9731a75 to 7ec715c Compare February 27, 2024 14:49
@Miraeld
Copy link
Contributor Author

Miraeld commented Feb 27, 2024

Umm,
I think it doesn't need another CR as I just applied what @jeawhanlee advised.
I can move it back to QA

@Mai-Saad
Copy link

Mai-Saad commented Feb 28, 2024

@Miraeld Thanks for the update.

  • The regression mentioned here is now fixed
  • Remaining that now, for large image uploaded before enable AVIF, we reoptimize the image i.e update timestamp for optimized images => we shouldn't reoptimize the image but just add avif, no? /cc @piotrbak (Note: after enable avif, if we refresh media and click generate link before background process start, it will work correctly i.e just generating avif so may be we can compare the generate code to the code on PR to catch the root cause)
    update: remaining issue(s) already on main branch will open separate GH about it

@Mai-Saad Mai-Saad merged commit 324c4fa into feature/avif Feb 28, 2024
2 checks passed
@Mai-Saad Mai-Saad deleted the fix/796-convert-avif-on-save-big-image branch February 28, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AVIF Avif branch/feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AVIF images arenot automatically generated for existing images after enabling the option
6 participants