GNOME Bugzilla – Bug 53726
load_at_size functionality would be nice.
Last modified: 2010-07-10 04:08:21 UTC
gdk_pixbuf_load_at_size would be nice. For loaders that don't implement it, we could add a very naive fallback. For those like the jpeg loader it could use builtin functions.
Yes, thank you very much. I had to do some libjpeg hacking in nautilus to quickly make jpeg thumbnails. Having this capability in gdk-pixbuf would rock.
Alex, Jonathan, I'm currently looking into implementing this. One issue that came up is that I need to know more exactly what degree of detailed control over the scaling behaviour you want. Do you need the option to keep the aspect ratio, or would simply scaling to an exactly specified size be good enough ? If keeping aspect ratio is required, would it be ok if the image comes out smaller, or do you need the more complicated scale-as-far-as-possible-then-crop ?
[ As an aside, the issue that brought this up was nautilus using libjpeg directly to load thumbnails at the thumbnail side ] I think we may want to do this in a fashion similar to librsvg. That lets you provide a callback to determine the height based upon the height passed in. We would prolly want to add this to GdkPixbufLoader. We can also add a few convenience functions (like gdk_pixbuf_load_from_file_at_size) or something for those who don't need to use the loader.
Yeah. Look at how librsvg does it. It has a variety of options that are all useful.
Ok, I will take a look at librsvg. For now, here is an implementation of the aspect-ratio-ignorant variant. It has a generic implementation which is just load-and-scale, and a more efficent one for libjpeg.
Created attachment 7743 [details] [review] the patch
As I said, I don't think this is the right way to do it. I think you should add a call to the vtable (load_at_size or something) that calls the callback when time comes to actually load the image (and when it knows how big the image is). This way you can have complete control over how it works. The gdk_pixbuf_new_from_file_at_size should just use the loader as an implementation detail.
I had a separate vtable entry first, but then switched over to this way of doing things, because it is less intrusive. But I can go back and try the callback-approach with a separate vtable entry. I hope you don't want incremental load-at-size as well, because that would be quite a bit more work.
That's exactly what I was proposing -- incremental load at size. With the assumption that really nothing other than the jpeg loader will support it. Is it hard to get libjpeg to do this?
Your proposal of a load_at_size vtable entry sounded like it would take a FILE * and thus be non-incremental. For the incremental variant you would probably need something like a begin_load_at_size vtable entry taking one more function pointer than the current begin_load. Incremental load_at_size is just as easy for libjpeg. Its only hard if we have to do the scaling ourselves, since that requires figuring out the necessary bookkeeping for scaling an image in chunks (the pixops are not a bad starting point for that, since they already operate line-based, but you have to figure out how many source lines you have to feed in per result line, and the necessary overlap between chunks. Its possible, just not trivial). And I wouldn't like to add jpeg-only functionality. If load_at_size is added, it should work with all formats.
Good point. Might be worth modifying the incremental callback to include the get_size function. I was thinking of doing load_at_size with those that don't support the functionality by simply scaling the image when done loading it. Do you think that would be painful?
No, but it would effectively make the loading non-incremental, since we wouldn't be able to call the update_func before the image has been properly scaled.
Actually, I was wrong about libjpeg. Incremental load_at_size is just as hard for jpeg, since we have to scale outselves anyway (look at the patch: libjpeg supports only scaling down by 1/2, 1/4 and 1/8). That was one of the reasons why I implemented it that way: Since we have to scale anyway, we can just treat the jpeg load_at_size function like any other (non-scaling) load function and do the necessary scaling in gdk_pixbuf_new_from_file_at_size. What libjpeg offers is merely a way to reduce the overhead spent on decoding a much too large image.
Ok, here is another attempt, based on the callback + vtable entry strategy. Note that I made the load_at_size vtable entry a proper virtual function (ie it has a "this" argument), in order to make the generic_load_at_size function possible. I think the same should be done for the load vtable entry to enable a generic_load based on the incremental loading functions (this code is currently duplicated in a number of loaders). What I dislike about this approach is the amount of code duplication between gdk_pixbuf_new_from_file and gdk_pixbuf_new_from_file_at_size. But apart from that, it works as well as the first patch and is quite a bit more general.
Created attachment 7744 [details] [review] second attempt
So we're reading a FILE and scaling that. Do we know if libpng supports this at all?
No, libpng doesn't support it in general, but we could make it stop early for interlaced pngs. That would give us the same set of scale factors as libjpeg: 1/2, 1/4 and 1/8.
Had a look at the nautilus/eel thumbnailing stuff now, I see that you're not really interested in incremental loading (ie the update_area signal), just in the loader_write interface, since you don't have a FILE *, just an URI. Even if chunked scaling turns out to be too hard, we could probably get away with only emitting update_area after the complete image has been loaded and scaled (there are already some loaders which do this because they fake incremental loading by writing and loading a temp file).
Yes. Using a FILE is not good enough for nautilus, since it uses gnome-vfs. But doing the scaling at the end (after having libjpeg do some scaling while loading) is ok.
Ok, third attempt, this time hopefully fulfilling your needs. This version adds a new function gdk_pixbuf_loader_set_size_func, which can be called on a loader before the area_prepared signal is emitted. Things work as usual otherwise, with the only caveat that the caller must be prepared for the area_prepared and area_updated signals to be emitted from the gdk_pixbuf_loader_close function. One open issue is animations: I don't know what to do on them and just ignore the size_func for them.
Created attachment 7766 [details] [review] third attempt
Alex, Jonathan, you didn't comment on my last version. Does this look like what you need ?
(Sorry for the delay, Matthias) It looks basically good to me. A couple comments: 1) You need a GtkDestroyNotify for the user data in gdk_pixbuf_loader_set_size_func 2) We need to either update the docs to tell people that load at size can fail or maybe even set a GError warning? Does GError do warnings? 3) It'd be nice to add a gdk_pixbuf_load_from_file_at_size (); wrapper function that falls back to just scaling it, if loading at size fails. Thanks a lot for doing this. It's very cool.
I agree on the DestroyNotify and adding a convenience wrapper is easy. Don't know what you mean about the warning though. With the current implementation, load-at- size works for all formats, not just jpeg (its just not very incremental for anything but jpeg until we figure out incremental scaling). Of course, any loading function can fail due to format errors or OOM, thats what the GError parameter is for. GError doesn't do warnings, btw. Will provide a new patch soon.
Created attachment 8439 [details] [review] added a DestroyNotify
Owen, how about committing this now ? I'd like to shorten my queue of outstanding pixbuf patches a bit...
Hmm, wouldn't it be cleaner to have something on the order of a: ::size_ready Signal then: gboolean gdk_pixbuf_loader_get_original_size (GdkPixbufLoader *loader, int *width int *height); gboolean gdk_pixbuf_loader_get_scaled_size (GdkPixbufLoader *loader, int *width int *height); void gdk_pixbuf_loader_set_scaled_size (GdkPixbufLoader *loader, int width, int height); The basic idea is that; set_scaled_size() may be ignored by some loaders (though I suppose we could fake it for all loaders) set_scaled_size() is ignored if it is called after ::size-ready. get_original_size() returns FALSE if it is called before ::size-ready. So, the idea is that to load at a scaled size, you connect to ::size-ready, get the original size, and then set the scaled size. Anytime we can use signals rather than special functions we simplify the API and simplify language bindings; and GdkPixbufLoader already is a signal-based-API.
Good idea, new patch to follow soon. What I currently have looks like: a size_prepared signal getting the original width and height as parameters and a function void gdk_pixbuf_loader_set_size (GdkPixbufLoader *loader, gint width, gint height) _set_size can also be called outside of the size_ready emission, but it will be ignored *after* the emission. Would you prefer a parameter-less signal with separate getters for the size ?
Created attachment 9620 [details] [review] another variant: signals
Looks good. Few comments. * The reason I went with separate functions was the general principle that signal parameters can't be changed compatibly while separate getters can be added to as needed. But I can't really see why that flexibility would be needed here. And your variant definitely is simpler and cleaner. I think it's fine. * I don't think the conditionalization if (context->size_func != NULL) { in io-jpeg.c is necessary; we only have one core calling the loaders and we know it always passes in a size func. * I think it's considered OK for a caller using the loader to clear the pixbuf in area_prepared; so it's probably necessary to call area_prepared before scaling then use pixbuf_scale() rather than pixbuf_scale_simple(). * Doesn't the computation of cinfo->scale_denom go awry if the caller specified width/height that were greater than the original size. (scale_denom ends up zero?) * The indentation in your patch may need some fixing up .. gdk-pixbuf generally has 8 space indents. (If you use emacs, it may be worth adding the magic: /* -*- mode: C; c-file-style: "linux" -*- */ To the top of each file.) * There seems to be an unrelated tiff change in your patch.
Committed to HEAD now with your fixes and a bit of documentation.