svsticky / chroma

Manage photo albums on S3 buckets. Successor to Pxl and Rstr
1 stars 1 forks source link

Improve image pipeline #14

Closed TobiasDeBruijn closed 9 months ago

TobiasDeBruijn commented 9 months ago

This PR aims to improve the image pipeline, in multiple ways. Mainly:

The speed ups are achieved by doing more work non-blocking (utilizing Tokio tasks) and parallel. E.g. uploading the original image to S3 is not done blocking anymore. Downside: No HTTP XXX is returned if the uploading fails. Should this be reverted?

Here's a quick overview of the current pipeline: chroma-image-pipeline

Blocks offset to the right do not block the main pipeline.

Opinions? @SilasPeters @HugoPeters1024

SilasPeters commented 9 months ago

This is big! (The commit as well, perhaps try to commit more often haha.)

How easy/intuitive is it to add steps to this pipeline?

TobiasDeBruijn commented 9 months ago

I mean, +108 -77 is pretty decent for my usual doing 🤣

Anyways, pretty easy. Every next step uses the output variable from the previous, so you could pretty easily stash something inbetween, e.g. for compression.

SilasPeters commented 9 months ago

How is the pipeline communicated to the user?

Also, does this also involve my idea in #8?:

When uploading pictures to chroma, the user might configure the album opting in to things like image compression, sorting, creating previews and what not. The user should not have to wait for these things, so perhaps a pipeline can pick all these tasks up and publish the album afterwards, notifying the user of every st[e]p and its progess.

TobiasDeBruijn commented 9 months ago

So the pipeline applies to only 1 image at the time. Say you have 50 images, your browser will make 50 requests. After every image completes, a counter is incremented in the frontend, with a countertje 5/50 e.g.

No those ideas weren't implemented yet

SilasPeters commented 9 months ago

No those ideas weren't implemented yet

Well that is out of scope for this issue anyway, but I actually tried to mention that that idea perhaps is related to this issue. However, this code is run per-image, and is thus not related I guess. Just trying to think ahead though

TobiasDeBruijn commented 9 months ago

I think the PR is ready now. The EXIF stripping step has been removed, as apparently just decoding the image already discards the EXIF metadata.

TobiasDeBruijn commented 9 months ago

Naar mijn mening kan met het mergen van deze PR Chroma ook naar productie toe. Wat is jullie mening @SilasPeters @HugoPeters1024 ?

SilasPeters commented 9 months ago

I still haven't been able to run it locally due to koala, but as far as I know chroma now has all (if not more) functionally than pxl. If so, the next step would be to switch to chroma! We will then also need to migrate the old content.

TobiasDeBruijn commented 9 months ago

Sweet!

Merging the old content should be simple enough, using the colorizer tool (see the colorizer dir at the repo root)