search

withastro / astro

The web framework for content-driven websites. ⭐️ Star to support our work!
https://astro.build
Other
46.65k stars 2.48k forks source link

Assets: Can't use AVIF image source #8056

Closed mattrossman closed 1 year ago

mattrossman commented 1 year ago

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

mattrossman commented 1 year ago

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

https://github.com/withastro/astro/blob/f39a80c583e9966b81bf5f7d3651a0778a5cf566/packages/astro/src/assets/consts.ts#L4-L11

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 commented 1 year ago

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 https://github.com/lovell/sharp/issues/1105#issuecomment-516010573

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

natemoo-re commented 1 year ago

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?

Princesseuh commented 1 year ago

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 commented 1 year ago

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. https://github.com/GoogleChromeLabs/squoosh/issues/733#issuecomment-596927199

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

For reference here is the commit that adds AVIF input support in my fork: https://github.com/withastro/astro/commit/7b2bcbc14dd0f13a84d79ba02bf46012e5c1e2c0

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

Princesseuh commented 1 year ago
  • 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.)