davidbyttow / govips

A lightning fast image processing and resizing library for Go
MIT License
1.28k stars 199 forks source link

Streaming support #370

Closed negator closed 1 year ago

negator commented 1 year ago

Adds Source and Target for full streaming support.

https://www.libvips.org/2019/11/29/True-streaming-for-libvips.html

This is a fairly straightforward patch. We add two new functions NewSourceFromReader and NewTargetToWriter, along with an ExportWriter function and all secondary Export[Jpeg|Tiff...]Target functions.

Inspired by: https://github.com/vansante/go-vips-thumbnailer

cc @tonimelisma

negator commented 1 year ago

ping @AttilaTheFun

AttilaTheFun commented 1 year ago

Hey @negator ! I've contributed to the repo previously but I don't have permission to merge PRs. cc @tonimelisma @davidbyttow

davidbyttow commented 1 year ago

This looks great! Give me a day to get back to this and respond.

davidbyttow commented 1 year ago

I'd love to see more tests added, but will merge.

davidbyttow commented 1 year ago

noticed BMP failure with "not supported", not sure if related?

negator commented 1 year ago

Interesting, let me take a look and get back you.

negator commented 1 year ago

@davidbyttow I added several more tests to ensure all image types can be read and written. As well as updated the golden tests to ensure exporting to writers works correctly.

Some things I discovered:

negator commented 1 year ago

I discovered that there are severe performance penalties that are incurred when C code calls Go code, which this implementation requires. Especially since it performs many 100s of C-to-Go calls from both the reader and writer. I don't really feel comfortable merging this as-is since it might affect performance for the users of the NewImageFromReader function. Additionally the ExportWriter functions will incur similar performance issues.

However I believe there will be a performance boost for users of the NewImageFromFile function which leverages the input streaming capability of libvips from files (incurring no C to Go overhead). Replacing the ExportWriter functions with functions that export to files will similarly yield a more performant implementation.

For now I'd like to close this PR and potentially reopen one with the above mentioned updates at a later time. Let me know how your like to proceed.

axkirillov commented 1 year ago

@negator Thank you so much for looking into this! It's very disappointing to hear about the C-to-Go issue.