-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Use the same base sharp instance for multiple transformation chains #235
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
Comments
Hello Julian, you're definitely not the first person to run into problems trying to pipe the same input image through multiple pipelines/output Streams. Both Buffer and Stream based input could benefit from improvements along the lines of your suggestion. As an aside, Filesystem inputs will suffer less of this kind of problem as (a) the OS can cache the compressed input file and (b) libvips can cache the decompressed image data given a suitably large enough opcache to work with. Off the top of my head, a combination of the following two changes could make the existing
This would make your example look something like: var transformer = sharp();
readableStream.pipe( transformer );
transformer.resize( 800, 600 ).pipe( firstWritableStream );
transformer.extract( 20, 20, 100, 100 ).pipe( secondWritableStream ); Leave it with me for a day or two to investigate what's possible without breaking existing (publicly documented) behaviour. Worst case this would make an excellent candidate for the breaking changes of a v1.0.0 release. (Aussi, je suis desole pour la reponse tarde, je suis visiter Paris pour le weekend!) |
First of all, thanks for looking into this :)
The approach seems a bit fragile to me. Looking at the code sample, it reads as if two distinct objects are being created. Two objects that a developer could be tempted to store and re-use. But then you're in for a big surprise if you shuffle the calls to var transformer = sharp();
readableStream.pipe( transformer );
var chain1 = transformer.resize( 800, 600 );
var chain2 = transformer.extract( 20, 20, 100, 100 );
chain1.pipe( firstWritableStream ); // outputs the image after a resize AND an extract
chain2.pipe( secondWritableStream ); // outputs the image without transformation This seems a bit confusing to me.
Indeed. The only solution I can see if you don't wanna use the factory approach is to have methods that add to the pipeline return a new instance: var transformer = sharp();
readableStream.pipe( transformer );
var resizer = transformer.resize( 800, 600 );
resizer.extract( 20, 20, 100, 100 ).pipe( firstWritableStream ); // resize AND extract
transformer.extract( 20, 20, 100, 100 ).pipe( secondWritableStream ); // extract ONLY
transformer.pipe( thirdWritableStream ); // no transformation A VERY breaking change to be sure.
Hope you enjoyed your stay in the most beautiful city in the world ;) |
@jaubourg Your comment about the fragility of the snapshot approach is completely correct. Any initial, rambling thoughts about the possibility of shoe-horning this into the existing API have gone 💥 in my mind :) I'll take a look at how your suggested API could be implemented in practice. The factory object would be responsible for piping data into the instances it produces, like an efficient Stream splitter. I see this tying up nicely with #140 and another idea I've been toying with where an optional, user-defined JavaScript function is called as soon as image metadata is available but before decompression takes place. In such a function, As always, thoughts and comments from other contributors and users interested in Streams are welcome. I know @davidmarkclements, @LinusU and @tybenz will all have good ideas. |
Commit 01b4d6e on the var factory = sharp();
readableStream.pipe( factory );
factory.clone().resize( 800, 600 ).pipe( firstWritableStream );
factory.clone().extract( 20, 20, 100, 100 ).pipe( secondWritableStream ); Cloned instances share input data with their siblings so this approach is fairly memory efficient. This can be tested via The functional tests contain an example where two cloned instances inherit the same angle of rotation but differ in resize dimensions. |
Love the API! 👍 One little question, from someone who has no knownledge of sharp's native bindings nor of eventual limitations libvips-side, seeing as the buffer is "mem-copied" in there, am I right in assuming libvips will have duplicates of the base image internally? If so, do you think it possible to rather have clones share the reference within libvips rather than the buffer? That would allow the garbage collector to clear memory JavaScript-side early and libvips to decode the image only once. Unless I'm missing some magic stream-based code feeding directly into libvips? (I'll have a look into the code in order to make more informed guesses and not bother you with more stupid questions) |
Thanks for the positive feedback Julian. Getting API design right is hard. It's possible for the V8 GC to enter its compacting phase after Clones could share compressed data in the (copied) input buffer with the addition of a look-up table keyed against a hash of the buffer content along with some form of reference counting. When using |
Here is what my JavaScript-centric, and quite ignorant, mind had imagined at first:
Then I did some reality check by looking at the libvips API docs and the only thing I see that kinda handles streaming into libvips is creating an image by providing a filename. I'd need to look into libvips source code to know if it actually streams anything or just gets the file in a big chunk. Also saw you could write pixels into a libvips image instance but, afaik, not compressed data (which would have made streaming into libvips as simple as writing a little wrapper around the image instance C-side). So you have a very sad panda here :/
Yeah, that seems like a good compromise for avoiding excessive memory usage. Not sure if it is worth the hassle unless V8 provides decent GC hooks that would take care of this for us.
Ah! I knew I was missing something subtle here. Thanks so much for the info. Do you know if it only concerns input JPEGs or does libvips provide the same kind of tricks for WEBP or even PNG? Anyway, so sorry to take so much of your time. I can't express how much I appreciate the work you're doing on this project and the fact you're kind enough to answer my very newby questions. |
Wouldn't something very cool be if it could be something like this:
This way we can still let |
Now I might be getting a bit out of hand but regarding nice api, I actually don't like function chaining. It makes state very hard to reason about, and having different function that manipulates the option that libvips gets can quickly become a mess. I would actually prefer some kind of options object which handles everything. let myFile = 'linus.jpg'
let operations = { resize: '100x100', gravity: 'center' }
sharp(myFile, operations).pipe(fs.createWriteStream('linus-thumb.jpg')) Here are some discussion on this from mongo-sql which dose something similar for sql where most libraries have the same chaining api style. I thought it was very interesting...
|
@LinusU, I'm so used to chaining now that it feels really fluid and natural to me. I'd concede that having methods that set options and methods that manipulate the image can get confusing. I'd rather have all options set when the sharp instance is created and limit methods to actual image transformations. The problem I see in using an object to specify a list of operations is that it becomes impossible to apply the same kind of transformation multiple times. I happen to do
That makes sense to me. |
@jaubourg That's good for you but I think that it would hold true for even the most awkward api. Use it enough and it'll start feeling natural. Now, I'm not saying that chaining is that bad, but it does have a number of drawbacks. Having a "pipeline" would be cool but that is not at all supported at the moment. The only one that actually works happens to be the one you specified Sometimes a chaining api can be a nice way to specify things and that could certainly stay, but I would propose that we do it like this. 1. All the functions that currently only manipulate (writing in ES6, because faster) class Sharp {
constructor (input) {
this.input = input
this.queue = []
this.transformPipeline = []
}
rotate (angle) {
this.transformPipeline.push({ op: 'rotate', angle: angle })
return this
}
// ...
}
export default function sharp (input) {
return new Sharp(input)
} 2. We have a function to run the chained commands // ...
run (cb) {
this.queue.push({ ops: this.transformPipeline, cb: cb })
this.transformPipeline = []
this.scheduleWork()
}
// ... 3. We also provide a function that runs an entire pipeline. // ...
transform (ops, cb) {
this.queue.push({ ops: ops, cb: cb })
this.scheduleWork()
}
// ... Usage example: let linus = sharp('linus.jpg')
let thumbnailOps = [
{ op: 'resize', size: '100x100' },
{ op: 'normalize' }
]
linus.transform(thumbnail, function (err, out) {
if (err) throw err
// out is your image :)
}) 4. All of this can be run using the method for paralellization that I posted earlier // ...
scheduleWork () {
if (this.workScheduled) return
this.workScheduled = true
setImmediate(() => {
let queue = this.queue
this.queue = []
this.workScheduled = false
// Call out to C++ and handle all the lists in `queue` with the input `this.input`
})
}
// ... Example usage let linus = sharp('linus.jpg')
let thumbnailOps = [
{ op: 'resize', size: '100x100', type: 'crop' },
{ op: 'normalize' }
]
let iphoneOps = [
{ op: 'resize', size: '800x800', type: 'max' }
]
let desktopOps = [
{ op: 'resize', size: '2048x2048', type: 'max' },
]
function handleImage (err, out) {
if (err) throw err
// out is your image :)
}
linus.transform(thumbnailOps, handleImage)
linus.transform(iphoneOps, handleImage)
linus.transform(desktopOps, handleImage) Wow that was a bit longer than I originally intended :) What do you guys think? I understand that this is quite large changes but it's just to get the discussion started. I think that we can accomplish something very great here! 🎉 |
@LinusU That looks a lot like what we have in our own app. We also have an optimizer that uses the image metadata to get the original width and height and then optimize the chain of transformations to have at most a preExtract, a resize and a postExtract in the end. Not a fan of the size as a string though. I'd rather have separate width and height: let iphoneOps = [ {
op: "resize",
type: "max",
width: 800,
height: 800
} ] That's pretty much the internal representation we use ourselves. I think the big issue, though, is more about how to share the compressed image data (and associated libvips image instance), especially with a stream input, but I may be wrong and @lovell will know a lot more about it. At least, that's the initial issue we have.
Agreed, like I said, I'm not a fan of chaining as a means to construct an internal options object. However, I much prefer method calls to perform actual actions as opposed to arrays of plain objects. I'm quite comfortable with objects describing a state and methods a change of state. Code logic is code logic if you see what I mean. |
Thank you for all these suggestions. I really like the pipeline approach and it's a lot more in keeping with the way libvips itself operates. The work to add this will be a lot easier after #152 is complete and will start to provide a more complete binding for libvips (rather than "just" being an image resizer). This proposed API enhancement deserves an issue in its own right - I will (selectively) copy and paste the most salient points into a new issue after we're done with the other things under discussion. In regards to inputs sharing memory, I'm going to experiment with using V8 Persistent handles for this. There's a small problem here in that when libvips emits the "postclose" signal to say it's finished with a buffer, we're not within a V8 HandleScope to Dispose or MakeWeak. My previous, brief attempt at this led to selecting a safe memcpy instead - see also #151 (comment) I've created #236 to track the addition of a post-metadata, pre-pipeline callback function to modify options based on the image dimensions/format/etc. |
V8 still requires its thread-local stack to be present to use Persist/Dispose as a means of preventing GC. This thread-local data isn't present for glib-invoked signal handlers. This means the memcpy protection has to remain until the minimum version of libvips is 8.0.0, which includes commit jcupitt/libvips@6ff1d53 that allows for earlier GC. I'll make a note of this in #152. |
The clone method is now available on the |
The clone method is now in master awaiting release. |
v0.11.0 will be released later today. #241 created to discuss the more major API improvements. |
Thank you, @lovell, for being so pro-active and opened to suggestions. :) |
I would like to second what @jaubourg said, it feels like we are in good hands! |
Hi all, regarding @LinusU 's note on multiple output images:
I can see that this might be possible. libvips currently has a thing called https://github.com/jcupitt/libvips/blob/master/libvips/conversion/sequential.c I think you'd need to extend @jaubourg mentioned streaming. libvips will stream most formats to and from files (though not webp, sadly, due to libwebp problems), but needs to have the whole image there at once for memory sources and sinks. There's a libvips branch which adds true streaming for memory sources and sinks too, see #30 and #179, if you've not come across it. |
I'm currently working on a server that delivers images manipulated by sharp under the hood.
We expect that several different chains of transformations will be requested in short windows of time for the same base image (pretty much concurrently when you take into account the time to read said base image) but, afaik, each transformation chain requires its own sharp instance.
The first obvious drawback is that libvips will use as many times the resources for the exact same image (it will decode and store the same base image for each transformation chain).
The less obvious drawback is that it makes it impossible to use a
ReadableStream
and pipe it into the sharp instance. We have to flush the image into aBuffer
so that we can create other instances when needs be without reading the base image every time, meaning more memory consumption (only, this time, within V8) and more latency for any transformation chain requested while the image is being read.Would it be possible to add some means of cloning sharp instances and avoid those caveats ?
In order not to break everything, how about factory instances, separate from current sharp instances?
Do you think this would be feasible ?
The text was updated successfully, but these errors were encountered: