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

RFC: Support image aspect ratio metadata #1306

Merged
merged 6 commits into from
Sep 6, 2023

Conversation

DavidBuchanan314
Copy link
Contributor

@DavidBuchanan314 DavidBuchanan314 commented Jul 9, 2023

An optional aspectRatio field has been added to the app.bsky.embed.images.image and app.bsky.embed.images.view lexicons.

The latter seems a bit redundant, but since the alt field was already "mirrored" into the view, I thought it made sense to duplicate the aspectRatio field there too.

The aspectRatio object has two fields, width and height integers, which encode the aspect ratio together as a pair.

The aspect ratio of an image is (optionally) detected by clients when a post is being made, and embedded into the post record. This could be encoded as a "standard" aspect ratio like 16:9 (width=16, height=9), or more likely, the client will just give the raw pixel dimensions of the original image, e.g. 1920:1080, because that's the most convenient logic to use. To be totally clear, it doesn't have to give the pixel dimensions, it can use any pair of integers that encode the desired ratio (ideally one close to the truth!)

The rationale behind encoding a ratio as opposed to the resolution is that the ratio stays the same after the image has been scaled/thumbnailed. Furthermore, some supported image formats (like SVG) don't necessarily use pixel units, but ratios are unitless.

A possible edge-case to consider is what happens when an image contains orientation metadata. I'm inclined to say that clients ought to normalise image orientation prior to upload. (but if they don't, the aspect ratio should be stored according to the expected on-screen orientation of the image)

There is no need for the PDS or AppView to parse media files to verify the reported aspect ratio, it can be taken at face value without much consequence. However, clients should still have logic to prevent overly tall or overly wide images from causing UI/UX breakage (but similar logic ought to be there anyway, for images that really do have extreme aspect ratios). If an incorrect aspect ratio is specified, the worst that can happen is just "it looks a bit weird in the UI".

Thanks to @redsolver, @swgws, and others for earlier discussion related to this feature.

Relevant issues:

#565

#427

@@ -223,6 +223,7 @@ export class FeedViews {
img.image.ref,
),
alt: img.alt,
...(img.aspectRatio && { aspectRatio: img.aspectRatio }),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could probably done more cleanly with a ternary operator or something? I'm bad at JS...

Copy link
Member

Choose a reason for hiding this comment

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

I think that’s probably the cleanest way to do it in one line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, would a simple aspectRatio: img.aspectRatio, do the right thing? Looking at the other parts of the same source file, I think an undefined value results in the field being omitted from the final record output.

Copy link
Member

Choose a reason for hiding this comment

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

I’d have to check the code but the JSON stringifier removes undefined fields so yeah it would almost certainly be fine. The only issue typically is if you do something like “aspectRatio” in object but I highly doubt that’s the case here

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah i think just img.aspectRatio would be fine here

@pfrazee
Copy link
Collaborator

pfrazee commented Jul 20, 2023

Just popping in to say I'm +1 to this idea

@dholms
Copy link
Collaborator

dholms commented Aug 31, 2023

Sorry for letting this sit so long! I'm +1 on the ideas as well.

Two quick things:

  • Mind merging in main, rebuilding lex-cli and rerunning codegen? we've made some tweaks to codegen & I just want to ensure we don't get any strange artifacts
  • Can you port the logic from the pds views service to the bsky views service of the same name
    • the code in both should look basically exactly the same, but we're actively deprecating the pds appview logic

@devinivy
Copy link
Collaborator

devinivy commented Sep 1, 2023

My only concern is that there's no guarantee aspect ratio of the original is preserved by resized images, and in particular we don't always preserve it. E.g. profile banner images and avatars are served resized w/ rules akin to the to background-size: cover property, which doesn't preserve the aspect ratio.

@mozzius
Copy link
Member

mozzius commented Sep 1, 2023

I think since this only applies to embed images, images used for other purposes (profiles etc) don't need to worry about this. Just like how pfps don't have alt text

Do you foresee resizing potentially happening to embed images too?

@dholms
Copy link
Collaborator

dholms commented Sep 1, 2023

yea agreed - I think it's important that this is an attribute on posts not images at large

I think resizing on embed images, if it happens at all, would only happen in extreme scenarios - in which case they would also likely be resized on the client & whatever optimistic aspect ratio is supplied by the writer won't be useful anyway

@DavidBuchanan314
Copy link
Contributor Author

Sorry for letting this sit so long! I'm +1 on the ideas as well.

Two quick things:

* Mind merging in main, rebuilding `lex-cli` and rerunning codegen? we've made some tweaks to codegen & I just want to ensure we don't get any strange artifacts

* Can you port the logic from the pds views service to the bsky views service of the same name
  
  * the code in both should look basically exactly the same, but we're actively deprecating the pds appview logic

all done, I think

Copy link
Collaborator

@dholms dholms 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 👍

i'll get this merged & rockin tmrw 👀

@dholms dholms merged commit 90b0a9d into bluesky-social:main Sep 6, 2023
10 checks passed
estrattonbailey added a commit that referenced this pull request Sep 6, 2023
* origin:
  Only do read after write on own author feed (#1544)
  RFC: Support image aspect ratio metadata (#1306)
estrattonbailey added a commit that referenced this pull request Sep 6, 2023
* origin:
  Only do read after write on own author feed (#1544)
  RFC: Support image aspect ratio metadata (#1306)
mloar pushed a commit to mloar/atproto that referenced this pull request Sep 26, 2023
* Add aspectRatio field to embed image lexicon

* lexicon codegen

* pass aspectRatio through to imagesEmbed views

* simplify aspectRatio logic, port to bsky package

* re-run lexicon codegen
mloar pushed a commit to mloar/atproto that referenced this pull request Nov 15, 2023
* Add aspectRatio field to embed image lexicon

* lexicon codegen

* pass aspectRatio through to imagesEmbed views

* simplify aspectRatio logic, port to bsky package

* re-run lexicon codegen
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.

5 participants