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 aspect ratio to Bluesky image embeds #843

Closed
JoelOtter opened this issue Dec 10, 2024 · 7 comments · Fixed by #844
Closed

Add aspect ratio to Bluesky image embeds #843

JoelOtter opened this issue Dec 10, 2024 · 7 comments · Fixed by #844

Comments

@JoelOtter
Copy link
Contributor

I've noticed that Bluesky (at least via the official clients) has recently stopped interpreting image aspect ratio for itself, relying instead on the information provided in the record:

image

Annoyingly I think the only way to address this will be to download the whole image into memory (or onto disk) and get the dimensions that way (we currently stream the image data). I'm happy to take this one, @snarfed do you have any opinions about this?

@snarfed
Copy link
Owner

snarfed commented Dec 10, 2024

Yes! We've been discussing this in snarfed/bridgy-fed#1571 . I haven't prioritized it due to the in-memory image processing, but if you want to add that - maybe with https://pypi.org/project/pillow/ ? - I'm all for it!

Also sadly Bridgy Fed uses different code to generate image blobs, https://github.com/snarfed/arroba/blob/c518924d31587dc9efa367c287c9d7d0d8654eca/arroba/datastore_storage.py#L361-L363 , but on the plus side, it should be straightforward to copy whatever we do here.

@JoelOtter
Copy link
Contributor Author

per the Bluesky docs images are max 1MB - imo we're fine to put that in memory. I think the only reliable way to ensure a max size of image downloaded is to iterate the chunks manually in the response and append them to some buffer? My understanding is we can't rely on Content-Length headers, and some image formats like JPEG don't reliably put their size information at the start of the file.

@snarfed
Copy link
Owner

snarfed commented Dec 10, 2024

Thanks for checking! We do check Content-Length on text and JSON responses, https://github.com/snarfed/webutil/blob/6384196b72a3e4768652dc45ad55f0ff8ef9100e/util.py#L1763-L1777 , but not others. I'm not too worried about a DOS though, it's not an amplification attack, an attacker has to send us as much data as we read, and Bridgy's not a big target anyway. I'd say don't worry about it for now.

@snarfed
Copy link
Owner

snarfed commented Dec 10, 2024

...oh actually you could use FileLimiter, it does the chunked limited stream reading you mentioned: https://github.com/snarfed/webutil/blob/6384196b72a3e4768652dc45ad55f0ff8ef9100e/util.py#L1571-L1596

@JoelOtter
Copy link
Contributor Author

@snarfed did this roll out to bridgy? My recent post didn't have the right aspect ratio unfortunately https://bsky.app/profile/joelotter.com/post/3ld45wjrxw52w

@JoelOtter
Copy link
Contributor Author

Here it is when run locally, assume it just hasn't rolled out yet :) https://bsky.app/profile/joelotter.com/post/3ld46btlkdp2w

@snarfed
Copy link
Owner

snarfed commented Dec 12, 2024

Yes! Sorry about that, deploying now.

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 a pull request may close this issue.

2 participants