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

Ensure post meta reflects image mime type #75

Closed
Tracked by #22
adamsilverstein opened this issue Jan 1, 2022 · 9 comments
Closed
Tracked by #22

Ensure post meta reflects image mime type #75

adamsilverstein opened this issue Jan 1, 2022 · 9 comments
Assignees
Labels
[Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Bug An existing feature is broken
Milestone

Comments

@adamsilverstein
Copy link
Member

adamsilverstein commented Jan 1, 2022

When uploading jpeg images with the webp-uploads module, we use WebP as the default file type for sub sized images. The post meta that lists the image sizes includes a mime-type value; we should ensure that matches the sub sized image mime type.

As reported in #32 (comment)

@adamsilverstein adamsilverstein added [Type] Bug An existing feature is broken [Focus] Images [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) labels Jan 1, 2022
@adamsilverstein adamsilverstein self-assigned this Jan 1, 2022
@adamsilverstein adamsilverstein added this to the 1.0.0-beta.1 milestone Jan 1, 2022
@mitogh
Copy link
Member

mitogh commented Jan 5, 2022

@erikyo
Copy link

erikyo commented Jan 23, 2022

hi @adamsilverstein,
This is an experiment, I did would try to solve these two issues (the other one is the #122), but after all I guess this definitively isn't in a great way because it adds an additional filter in order to work. Maybe it would be better to add a filter to media_handle_upload (wp-admin/includes/media.php) because without this, I can be wrong but as far as I know, the uploaded image cannot be touched before this hook.

I also don't understand why after this filter the "cloned" webp isn't deleted when I delete the image from WordPress media library, even if the attachment metadata array seems correct. I suspect this issue is related to wp_unique_filename() but not sure.

https://gist.github.com/erikyo/f0aca6c34fd1d6bc5f73f953bb342a65
(needs to be used alongside Module WebP Uploads)

@adamsilverstein
Copy link
Member Author

I also don't understand why after this filter the "cloned" webp isn't deleted when I delete the image from WordPress media library, even if the attachment metadata array seems correct. I suspect this issue is related to wp_unique_filename() but not sure.

I think you need to add the new file data to the returned new_sizes? if you look at the meta data after uploading, does it reference both the jpeg and webp images, or just jpeg? or is your idea to add the webp type on the fly?

@erikyo
Copy link

erikyo commented Jan 31, 2022

yes, my idea is to replace also the "full" size image on the fly with the copy in webp format (so replacing the $meta['file']), since the original jpg was already stored (or can be stored) as "original_image" and since I don't want to show jpg images when I wp_get_attachment_image().

At the time the filter is fired, the image has already been saved (but the resizes are not and the sizes array is empty), my idea is to use this filter to hack the image_meta before the subsizes image were generated. This to avoid #69 because sometimes (eg. small images without the -scaled) the image meta are like below:

Array
(
    [width] => 945
    [height] => 1007
    [file] => 2022/01/small-image.jpeg
    [sizes] => Array
    (...)

    [image_meta] => Array
    (...)
)

this is the reason why i add this in my filter

$image_meta['file'] = $webp_file;
wp_update_attachment_metadata( $attachment_id, $image_meta );

And in addition i replace the "post_mime_type" of this post to ensure post type attachment meta reflects image mime type (see here). Change the post mime was a bad idea #67 (comment)

wp_update_post( array(
  'ID'             => $attachment_id,
  'post_mime_type' => 'image/webp'
) );

After my filter the image_metadata will look like this:

Array
(
    [width] => 945
    [height] => 1007
++  [file] => 2022/01/vert.webp
--  [file] => 2022/01/vert.jpg

But after the sub-sizes have been generated it appears that full is a jpg and not a webp. I didn't realise that (and this is the reason why the full webp image wasn't deleted), I'll try to find out why.

Array
(
    [width] => 945
    [height] => 1007
    [file] => 2022/01/vert.jpeg
    [sizes] => Array
        (
            [medium] => Array
                (
                    [file] => vert-441x470.webp
                    [width] => 441
                    [height] => 470
                    [mime-type] => image/webp
                )

            [thumbnail] => Array
                (
                    [file] => vert-150x150.webp
                    [width] => 150
                    [height] => 150
                    [mime-type] => image/webp
                )

            [medium_large] => Array
                (
                    [file] => vert-768x818.webp
                    [width] => 768
                    [height] => 818
                    [mime-type] => image/webp
                )

            [post-thumbnail] => Array
                (
                    [file] => vert-400x426.webp
                    [width] => 400
                    [height] => 426
                    [mime-type] => image/webp
                )

        )

    [image_meta] => Array
        (...)
)

edit: the reason why my filter was overridden was because _wp_make_subsizes() saves again the image metadata using the argument $image_meta which comes as it is generated here so doing it so early is not good.

Besides this, I discovered a way that is perhaps better for updating the meta of small images:

if (!isset($image_meta['original_image'])) {
    // replace the original image path with the webp image path
    $image_meta['original_image'] = $image_meta['file'];
    $image_meta['file'] = $webp_file;
    wp_update_attachment_metadata( $attachment_id, $image_meta );
}

@erikyo
Copy link

erikyo commented Feb 1, 2022

Moving the filter after the image subsizes generation (using wp_generate_attachment_metadata) has fixed the issue and the generated image full size image was deleted but the old "original_image" not 😥. Again, an additional filter (like 'delete_attachment') would be needed to delete the original_image because it does not happen automatically as I expected.

The image_meta array actually looks is like this:

Array
(
    [width] => 945
    [height] => 1007
    [file] => 2022/02/vertical_img-1.webp
    [sizes] => Array
        (
            [medium] => Array
                (
                    [file] => vertical_img-1-441x470.webp
                    [width] => 441
                    [height] => 470
                    [mime-type] => image/webp
                )

            [thumbnail] => Array
                (
                    [file] => vertical_img-1-150x150.webp
                    [width] => 150
                    [height] => 150
                    [mime-type] => image/webp
                )

            [medium_large] => Array
                (
                    [file] => vertical_img-1-768x818.webp
                    [width] => 768
                    [height] => 818
                    [mime-type] => image/webp
                )

            [post-thumbnail] => Array
                (
                    [file] => vertical_img-1-400x426.webp
                    [width] => 400
                    [height] => 426
                    [mime-type] => image/webp
                )

        )

    [image_meta] => Array
        (
            [aperture] => 0
            [credit] => 
            [camera] => 
            [caption] => 
            [created_timestamp] => 0
            [copyright] => 
            [focal_length] => 0
            [iso] => 0
            [shutter_speed] => 0
            [title] => 
            [orientation] => 0
            [keywords] => Array
                (
                )

        )

    [original_image] => 2022/02/vertical_img-1.jpeg
)

@erikyo
Copy link

erikyo commented Feb 1, 2022

the original_image would have been deleted if I had written the path correctly.

updated the gist now the filter it work as expected (or at least it does in the way I would like it work).
https://gist.github.com/erikyo/f0aca6c34fd1d6bc5f73f953bb342a65

Honestly, I would like the original jpg to be used only when I have to regenerate images, when I have to serve it to users I would prefer it to be always webp.

@adamsilverstein
Copy link
Member Author

Honestly, I would like the original jpg to be used only when I have to regenerate images, when I have to serve it to users I would prefer it to be always webp.

If you have the right srcset, this should be the case, even if the image src says jpeg, the browser will load the correct webp from the srcset list. I tested mostly with twentytwentytwo, the behavior may be theme dependent

@erikyo
Copy link

erikyo commented Feb 1, 2022

I created a test for this case:
https://codepen.io/erikyo/pen/Rwjaoqa
https://modul-r.codekraft.it/2022/02/image-test/

Currently, what I've noticed is that chrome uses the image in src if it fits perfectly in its container or if the images is smaller than that, and this happens also if there is a webp image of the same size of the original in srcset. I'm using canary which is chrome 3 versions ahead and i don't think that will change anytime soon. Tested also in FF, Edge and safari seem to have the same behaviour (in this case, with my blog).

image

https://html.spec.whatwg.org/multipage/images.html#image-request
https://html.spec.whatwg.org/multipage/images.html#images-processing-model

@adamsilverstein
Copy link
Member Author

Currently, what I've noticed is that chrome uses the image in src if it fits perfectly in its container or if the images is smaller than that, and this happens also if there is a webp image of the same size of the original in srcset. I'm using canary which is chrome 3 versions ahead and i don't think that will change anytime soon. Tested also in FF, Edge and safari seem to have the same behaviour (in this case, with my blog).

Excellent (and disappointing) research @erikyo - I think this means that when outputting the image tag, we need to ensure the src also uses the WebP type. Currently this is the case for large uploads, but I was planning on changing that in #143.

One thing we could consider is adjusting the output, so the img src references the large image size instead of the original image. this output change could be behind a separate filter.

I am planning to close this issue because the original reason I opened it is essentially resolved... we can continue the conversation on #142

When uploading jpeg images with the webp-uploads module, we use WebP as the default file type for sub sized images. The post meta that lists the image sizes includes a mime-type value; we should ensure that matches the sub sized image mime type.

I can confirm that the mime type is correct in the meta data. The issue originally reported in #32 (comment) was likely due to the behavior of the -scaled image being off which I aim to fix in #143.

I am closing this issue as resolved for now.

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
None yet
Development

No branches or pull requests

3 participants