GNOME Bugzilla – Bug 442452
gdk-pixbuf's Scaling page omits lots of information
Last modified: 2017-01-10 12:29:53 UTC
I have a few complaints: 1. It uses the word compositing. That is, if not inaccurate, at least vague. A better term would be blending or even alpha blending. 2. Which blend function is used? I'm guessing Cd = CsAs + Cd(1-As), but it should be mentioned explicitly. 3. It doesn't mention if there are any optimized paths. For example, is the case when you call gdk_pixbuf_scale_simple() with dest_width and dest_height equal to the sizes of the original pixbuf optimized? Knowing this is important because then you may not need some special cases in your own code that uses gdk-pixbuf. 4. The example uses gdk_pixbuf_render_to_drawable which is deprecated. 5. The doc for @check_size is wrong - it must not be a power of 2. Other numbers work.
Philip, is this something you could look into?
(In reply to Bastien Nocera from comment #1) > Philip, is this something you could look into? That sounds interesting. I will try and find time soon.
(In reply to Björn Lindqvist from comment #0) > I have a few complaints: > > 1. It uses the word compositing. That is, if not inaccurate, at least vague. > A better term would be blending or even alpha blending. Done. > 2. Which blend function is used? I'm guessing Cd = CsAs + Cd(1-As), but it > should be mentioned explicitly. From my reading of composite_line_color() in pixops.c, I think that’s the case, but it’s probably a good idea for someone to double-check if they know that code. > 3. It doesn't mention if there are any optimized paths. For example, is the > case when you call gdk_pixbuf_scale_simple() with dest_width and dest_height > equal to the sizes of the original pixbuf optimized? Knowing this is > important because then you may not need some special cases in your own code > that uses gdk-pixbuf. Done. There are not that many fast paths: I’ve documented all the ones I could find in gdk-pixbuf-scale.c, and added the new one which you suggested (it did not exist before). > 4. The example uses gdk_pixbuf_render_to_drawable which is deprecated. Fixed. > 5. The doc for @check_size is wrong - it must not be a power of 2. Other > numbers work. The code in get_check_shift() (which is called on all paths) assumes that a single bit is set, i.e. that the check_size is a power of 2.
Created attachment 343098 [details] [review] docs: Fix reference to a non-existent function That one moved to gdk-pixbuf-xlib a while ago.
Created attachment 343099 [details] [review] docs: Use ‘alpha blending’ rather than ‘compositing’ It’s a slightly more accurate term, as compositing seems to mostly refer to bit-mask composition of two images, rather than blending.
Created attachment 343100 [details] [review] docs: Document the alpha blending function used
Created attachment 343101 [details] [review] docs: Document existing scaling and transformation fast paths There are not many, as far as I can see.
Created attachment 343102 [details] [review] gdk-pixbuf-scale: Add a fast path for a no-op scale If calling gdk_pixbuf_scale_simple() with equal src and dest dimensions, there’s nothing to do, so just return a copy of the src pixbuf. Document the new fast path.
Review of attachment 343098 [details] [review]: The rest of the paragraph could also do with updating :/
Review of attachment 343099 [details] [review]: ++
Review of attachment 343100 [details] [review]: Looks fine, though I'm not certain that's the function actually used. Maybe Federico knows?
Review of attachment 343101 [details] [review]: ::: gdk-pixbuf/gdk-pixbuf-scale.c @@ +418,3 @@ * result in a new pixbuf. * + * If @angle is 0, a fast path is used. I'd rather we said "a copy of @src is returned". Is that a fast path? At least that would mention the behaviour (a copy, rather than a ref) rather than the programmatic flow.
Review of attachment 343102 [details] [review]: ::: gdk-pixbuf/gdk-pixbuf-scale.c @@ +332,3 @@ * + * If @dest_width and @dest_height are equal to the @src width and height, a + * fast path is used which avoids any scaling. again, mention that a copy is returned, not a reference.
(In reply to Bastien Nocera from comment #11) > Review of attachment 343100 [details] [review] [review]: > > Looks fine, though I'm not certain that's the function actually used. Maybe > Federico knows? Federico, can you help?
(In reply to Bastien Nocera from comment #14) > (In reply to Bastien Nocera from comment #11) > > > Looks fine, though I'm not certain that's the function actually used. Maybe > > Federico knows? > > Federico, can you help? Yes, this is the correct function.
Review of attachment 343100 [details] [review]: Looks good then.
Attachment 343098 [details] pushed as a442192 - docs: Fix reference to a non-existent function Attachment 343099 [details] pushed as f3b6576 - docs: Use ‘alpha blending’ rather than ‘compositing’ Attachment 343100 [details] pushed as 0d51eb1 - docs: Document the alpha blending function used
Pushed the other two patches with the requested changes (good suggestions, thanks). Keeping the bug open to have a crack at updating the rest of the GdkPixbufAlphaMode documentation. The following fixes have been pushed: bab37cd gdk-pixbuf-scale: Add a fast path for a no-op scale f4b9774 docs: Document existing scaling and transformation fast paths
Created attachment 343224 [details] [review] gdk-pixbuf-scale: Add a fast path for a no-op scale If calling gdk_pixbuf_scale_simple() with equal src and dest dimensions, there’s nothing to do, so just return a copy of the src pixbuf. Document the new fast path.
Created attachment 343225 [details] [review] docs: Document existing scaling and transformation fast paths There are not many, as far as I can see.
(In reply to Bastien Nocera from comment #9) > Review of attachment 343098 [details] [review] [review]: > > The rest of the paragraph could also do with updating :/ Actually, it’s all still entirely correct. Only bi-level alpha blending is supported by the XLib renderer: https://git.gnome.org/browse/gdk-pixbuf/tree/contrib/gdk-pixbuf-xlib/gdk-pixbuf-xlib-render.c#n315
(In reply to Philip Withnall from comment #21) > (In reply to Bastien Nocera from comment #9) > > Review of attachment 343098 [details] [review] [review] [review]: > > > > The rest of the paragraph could also do with updating :/ > > Actually, it’s all still entirely correct. Only bi-level alpha blending is > supported by the XLib renderer: > > https://git.gnome.org/browse/gdk-pixbuf/tree/contrib/gdk-pixbuf-xlib/gdk- > pixbuf-xlib-render.c#n315 It's in the main gdk-pixbuf docs section, but it's only used by the Xlib extension that we're trying to remove. Should that be moved to gdk-pixbuf-xlib instead?
(In reply to Bastien Nocera from comment #22) > (In reply to Philip Withnall from comment #21) > > (In reply to Bastien Nocera from comment #9) > > > Review of attachment 343098 [details] [review] [review] [review] [review]: > > > > > > The rest of the paragraph could also do with updating :/ > > > > Actually, it’s all still entirely correct. Only bi-level alpha blending is > > supported by the XLib renderer: > > > > https://git.gnome.org/browse/gdk-pixbuf/tree/contrib/gdk-pixbuf-xlib/gdk- > > pixbuf-xlib-render.c#n315 > > It's in the main gdk-pixbuf docs section, but it's only used by the Xlib > extension that we're trying to remove. Should that be moved to > gdk-pixbuf-xlib instead? That would make sense, but it would be an API break: the xlib stuff is only compiled if USE_X11 is set; and the symbol would move to a different header and .pc file.