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

Assets: Can't use AVIF image source #8056

Closed
1 task done
mattrossman opened this issue Aug 13, 2023 · 9 comments · Fixed by #8380
Closed
1 task done

Assets: Can't use AVIF image source #8056

mattrossman opened this issue Aug 13, 2023 · 9 comments · Fixed by #8380
Labels
- P2: nice to have Not breaking anything but nice to have (priority) feat: assets Related to the Assets feature (scope)

Comments

@mattrossman
Copy link
Contributor

mattrossman commented Aug 13, 2023

What version of astro are you using?

2.10.7

Are you using an SSR adapter? If so, which one?

None

What package manager are you using?

yarn

What operating system are you using?

macOS

What browser are you using?

Chrome

Describe the Bug

When using experimental Assets:

Referencing a relative .avif image from a .md page produces a console error about "Missing image dimensions...".

Importing an .avif image in an .astro file produces a "Cannot find module..." type error.

What's the expected result?

AVIF can be used as an image input similar to other formats like JPEG, PNG, WebP, etc.

Link to Minimal Reproducible Example

https://github.com/mattrossman/astro-assets-avif-input

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Aug 13, 2023
@mattrossman
Copy link
Contributor Author

mattrossman commented Aug 13, 2023

I see here that .avif and a couple other image formats are a TODO item, blocked by support from image-size:

// TODO: `image-size` does not support the following formats, so users can't import them.
// However, it would be immensely useful to add, for three reasons:
// - `heic` and `heif` are common formats, especially among Apple users.
// - AVIF is a common format on the web that's bound to become more and more common.
// - It's totally reasonable for an user's provided image service to want to support more image types.
//'heic',
//'heif',
//'avif',

It looks like relavant usage of image-size is here:

https://github.com/withastro/astro/blob/f39a80c583e9966b81bf5f7d3651a0778a5cf566/packages/astro/src/assets/utils/metadata.ts

Since sharp is already a dependency, and astro's imageMetadata() util is already marked async, one idea is to replace image-size with sharp's metadata() function, which I think returns all the relevant metadata needed here.

@mattrossman
Copy link
Contributor Author

mattrossman commented Aug 14, 2023

Note that while sharp works out-of-the-box with AVIF, it sounds like some patenting issues prevent inclusion of HEIF / HEIC support without a custom libvips install lovell/sharp#1105 (comment)

probe-image-size supposedly works out of the box with AVIF, HEIF and HEIC, along with existing supported formats.

@natemoo-re natemoo-re added the feat: assets Related to the Assets feature (scope) label Aug 14, 2023
@natemoo-re
Copy link
Member

I know sharp is the default image service and is in our dependency tree, but I think we've avoided having the core assets logic depend on Sharp so that custom image services can be used.
I vaguely remember us looking into using probe-image-size, but I'm not sure what the status of that was.

Since there's already a TODO in the codebase, I think it's safe to say that we're already aware of/tracking this. Not sure if @Princesseuh has anything else to add?

@natemoo-re natemoo-re added the - P2: nice to have Not breaking anything but nice to have (priority) label Aug 14, 2023
@github-actions github-actions bot removed the needs triage Issue needs to be triaged label Aug 14, 2023
@Princesseuh
Copy link
Member

Yep, exactly what Nate said. Until Sharp can safely run everywhere, we can't depend on it in the core codebase super safely. Replacing with probe-image-size would be an option, though I remember encountering an issue (that I don't remember now) that prevented me from doing so 🤔

image-size shipping this, or an awesome new ESM, non-Node library coming out for this would be the way forward otherwise if probe-image-size doesn't turn out to work correctly

@mattrossman
Copy link
Contributor Author

I tried making the following changes:

I then tried importing an AVIF image into an <Image /> in one of the examples. The image loads as expected and the existing tests pass ✅

I also tried importing an HEIC image (sample). I didn't see an error in the console, but the converted image failed to load with a 500 response—I assume this is because the Squoosh image service doesn't support HEIC / HEIF due to licensing issues, similar to why sharp can't support those formats. GoogleChromeLabs/squoosh#733 (comment)

Since the AVIF part works and isn't tied up in licensing difficulties, maybe we could start with that format? That's all I need for my use case at least.

I can make a PR for this.

@gvkhna
Copy link

gvkhna commented Aug 23, 2023

@mattrossman out of curiosity are you manually converting your images to avif/heic and what is your use case more specifically, are you trying to use picture element for multiple formats or you would just like to only serve modern formats?

I ask because I would like to support modern formats with Astro as well but this is area is still in development, hopefully might help drive this forward.

@mattrossman
Copy link
Contributor Author

@gvkhna My reason for using AVIF image inputs is that they tend to be the smaller on-disk than other formats I've tested, so for images I'm storing locally in a project (e.g. for a personal blog) it's creates less bloat in my Git history.

I use the avif CLI to batch convert images to AVIF before commiting them to Git.

I would probably want them to be down-converted to a more widely supported format in the build. If it could also serve the modern format in a picture element too, that'd be nice.

@mattrossman
Copy link
Contributor Author

For reference here is the commit that adds AVIF input support in my fork: 7b2bcbc

I think in order to turn this into a proper PR I'd like to know:

  • Should I add any tests for this? (if so where)
  • Should I vendor probe-image-size similar to how image-size is vendored?
  • Should I replace all usage of image-size in the project, or only the part shown in that commit?

@Princesseuh
Copy link
Member

  • Should I add any tests for this? (if so where)

It would be nice, but it's not necessary, we already have a lot of tests that test the entire pipeline that would fail if this didn't work, so heh.

  • Should I vendor probe-image-size similar to how image-size is vendored?

Not if it works as a normal dependency, the only reason image-size is vendored is because it's CJS and it was causing weird issues.

  • Should I replace all usage of image-size in the project, or only the part shown in that commit?

The part shown in the commit is fine, you can also remove the image-size folder completely (I believe the only remaining parts using image-size in the projects would be in the will-be-deprecated-in-two-weeks @astrojs/image, so no worries about it still existing somewhere.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P2: nice to have Not breaking anything but nice to have (priority) feat: assets Related to the Assets feature (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants