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

LoadImageFromFile should use native libvips implementation #289

Open
ugljesas opened this issue Jun 15, 2022 · 3 comments
Open

LoadImageFromFile should use native libvips implementation #289

ugljesas opened this issue Jun 15, 2022 · 3 comments

Comments

@ugljesas
Copy link

This is more of a feature request - but I think there are multiple use-cases where it would be beneficial to use the native libvips implementation for loading from a file. The current implementation reads the whole file into memory and calls LoadImageFromBuffer.

In our case, we are using govips as part of the detect pipeline for uploaded user media which could be one of multiple types. Natively, libvips only reads the file header to return the metadata. In the case of a large file (e.g. video of 600MB), and a cloud environment where disk throughput is suboptimal (capped at 200MB/s) it can take a substantial amount of time to fetch the file meta (or determine that the file is not supported). We reverted to calling the vipsheader CLI here to save time.

Also, loading the whole file into memory seems unnecessary as libvips has optimizations to not load the whole file at once where possible in order to be more RAM efficient.

@tonimelisma
Copy link
Collaborator

Hey @ugljesas thanks for the comment. I feel like that would require a complete refactoring of the govips library, the data structures and how we use libvips. As usual, this is an open source project which is mostly maintained by a few eager volunteers in their spare time, so I don't think we have capacity for something like that. If we'd start all over, I'm sure we might consider an approach like that, besides using glib introspection for automatic API generation and a bunch of other things.

Do you think you'd personally have bandwidth to plan and/or implement parts of such a change?

@ugljesas
Copy link
Author

ugljesas commented Jul 4, 2022

Hi @tonimelisma, thank you for your reply. I'm fairly new to Golang and haven't had much hands-on experience with c-bindings yet. That said, looking into the code I had the impression it could be done without a bigger refactor, in line with the pull request @cshum made here (thank you!!). Of course, due to my lack of experience with this, I'm not aware of all the implications of the change. Could you please elaborate on why you feel like this would require a complete refactoring of govips? I'd be open to helping develop and/or test this in some capacity but would probably need some guidance.

@tonimelisma
Copy link
Collaborator

Perhaps you're right, it could be I didn't look into it deep enough. Unfortunately I've just moved to the US and don't have much capacity to dig deeper into this the coming days.

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

No branches or pull requests

2 participants