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

Add avif #1428

Merged
merged 4 commits into from
Feb 6, 2023
Merged

Add avif #1428

merged 4 commits into from
Feb 6, 2023

Conversation

hashbender
Copy link
Contributor

@hashbender hashbender commented Jan 31, 2023

@cryptoquick
Copy link
Contributor

AVIF is a good format, it's much more compact, so it's better on fees and make sense to add to ordinals. I'd like to also see AV1 support once that's more widely supported. Maybe it already is in browsers, actually.

@cryptoquick
Copy link
Contributor

Hm, not currently supported on Safari or Edge:
https://caniuse.com/av1

Similar story for AVIF, but there's a preview version of Safari:
https://caniuse.com/avif

@casey casey mentioned this pull request Jan 31, 2023
Copy link
Collaborator

@casey casey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

My bar for browser support for new content types has been around ~95% of users, according to caniuse.com. It looks like AVIF is currently around 80%. My worry is that people publish inscriptions that then can't be viewed by many users.

What I suggest we do is add it to the media table, but not give it an extension, see my comment inline.

src/media.rs Outdated
@@ -56,6 +56,7 @@ const TABLE: &[(&str, Media, &[&str])] = &[
("audio/mpeg", Media::Audio, &["mp3"]),
("audio/wav", Media::Audio, &["wav"]),
("image/apng", Media::Image, &["apng"]),
("image/avif", Media::Image, &["avif"]),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove the extension. This will allow inscriptions with the image/avif content type to display on explorers, but won't let users create them with ord wallet inscribe FILE.avif, since they won't have a recognized extension.

Suggested change
("image/avif", Media::Image, &["avif"]),
("image/avif", Media::Image, &[]),

This will cause some tests to break, but they can be fixed by doing this in content_type_for_extension:

    let mut extensions = TABLE
      .iter()
      .flat_map(|(_, _, extensions)| extensions.first().cloned())
      .collect::<Vec<&str>>();

@cryptoquick
Copy link
Contributor

@casey Only problem is that approach always breaks the MacOS test

@cryptoquick
Copy link
Contributor

Come to think of it, since AVIF images can be much smaller, sometimes 2-5x smaller in size, this could be a good way for fee-conscious users to save money at the expense of some compatibility. There will be gaps in the explorer and those could be awkward, but maybe those could be hidden through browser feature detection... It appears something like this code could do the detection to hide unviewable inscriptions:
https://github.com/Kagami/avif.js/blob/69b52903c1f0d713e9a5082daac162b377b6ad83/avif.js#L66-L69

Copy link
Collaborator

@casey casey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I removed the .avif extension, I'm still not sure about allowing formats that might preview in the user's local browser, but not for others.

@casey casey enabled auto-merge (squash) February 6, 2023 07:23
@casey
Copy link
Collaborator

casey commented Feb 6, 2023

See #1521 for enabling this all the way.

Crazy to think that the more efficient we make inscriptions, the more efficiently everyone can use the chain, including normal bitcoin transactions 🤯

@casey casey merged commit afd54d0 into ordinals:master Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants