-
-
Notifications
You must be signed in to change notification settings - Fork 889
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
Adding blurhash to image_details. #5227
base: main
Are you sure you want to change the base?
Conversation
This is now available in pictrs v0.5.17-pre.8 |
K I updated to that pictrs, and tested this manually. Its correctly setting the blurhash DB column for uploaded images. I'm not sure whether to have a default blurhash, or just leave it as is with the blurhash column optional. Proxied images can't get blurhashes anyway, only uploaded ones can, so its probably fine to leave optional. |
-- Add a blurhash column for image_details | ||
ALTER TABLE image_details | ||
-- Supposed to be 20-30 chars, use 50 to be safe | ||
-- TODO this should be made not null for future versions of pictrs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You said this isnt possible for proxied images, then remove the comment (and in request.rs too).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
K, I'll do that now, and check out the failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine, but there are test failures.
Tests passed okay locally, so hopefully it was just an intermittent CI error. |
Nope it keeps failing consistently. Restarted it again just in case. |
Ugh, this one's frustrating. I've run it locally a ton of times, and can't replicate the CI errors. The only thing I've changed is the pictrs version. |
Seems like pictrs is throwing an error but its not getting logged properly: "ERROR pict_rs::middleware::log: server error". Try removing all the CI steps except build and api test, then have it run only a single image test only, and ensure that pictrs output is visible in ci. |
Pull request was converted to draft
Leaving this as draft until a new version of pictrs is released, with blurhash, and tests can be added.
The actual response from pictrs is
blurhash: String
, but we should leave this as an option for at least a few versions, in order to not break image details running older pictrs versions.