Created attachment 314140 [details] [review]
gdk_pixbuf_get_from_window: honor device scale

gdk_pixbuf_get_from_window() paints the given window onto a new cairo
surface. Create that new surface with the same device scale as the
window so that the result is not scaled down on hidpi screens.

Review of attachment 314140 [details] [review]:

Looks good

Attachment 314140 [details] pushed as 657a43e - gdk_pixbuf_get_from_window: honor device scale

I am pretty sure this patch is wrong. In particular, the documentation clearly states that

> This function will create an RGB pixbuf with 8 bits per channel with the same size specified by the width and height arguments.

and this patch violates that.

This patch also breaks the behavior of gdk_pixbuf_get_from_surface() for surfaces with device scale which now expects the arguments to be given relative to the surface's device scale.

I do not think we have any API inside GTK or GDK currently where a pixbuf does not assume a device scale of 1 and therefore on hidpi it is expected behavior that a pixbuf gotten from whatever API (without an explicit scale argument) is scaled down.

What is even worse is that this change doesn't allow getting a downscaled pixbuf at all. So in the case where you want to use it to implement the previous behavior (like I just wanted in GtkIconHelper/gtk_entry_get_icon_pixbuf()), you can't anymore.

I intend to revert this patch and then we can figure out a way that doesn't break the functions.

What's the actual use case for calling gdk_pixbuf_get_from_window() btw? Nobody should ever call the function anyway because the contents you get back are rather undefined and depend on details of the windowing system.

So I've always wanted top deprecate (and then get rid of) this function.

(In reply to Benjamin Otte (Company) from comment #4)
> I am pretty sure this patch is wrong. In particular, the documentation
> clearly states that
> 
> > This function will create an RGB pixbuf with 8 bits per channel with the same size specified by the width and height arguments.
> 
> and this patch violates that.

I don't think it does. The documentation always talks about logical pixels.


> This patch also breaks the behavior of gdk_pixbuf_get_from_surface() for
> surfaces with device scale which now expects the arguments to be given
> relative to the surface's device scale.

Every API we have that deals with pixels lengths gives them in logical units. Why should this one be different?

When is scaling down ever the right thing to do? Surely noone wants to get a blurry result from a function that is documented as "efficiently reading individual pixels from cairo surfaces".


> What's the actual use case for calling gdk_pixbuf_get_from_window() btw?
> Nobody should ever call the function anyway because the contents you get
> back are rather undefined and depend on details of the windowing system.

gnome-screenshot uses it in its fallback path.

Scaling down is the right idea whenever you want to use it on APIs that scale up on hidpi. The most prominent example is gdk_cairo_set_source_pixbuf().

It is also the right thing when supporting legacy code that treats logical units the same as screen units, like when using gdk_pixbuf_get_width() in get_preferred_width() callbacks.

I also think it is confusing for newcomers that don't know about scale when GTK functions return valuse from coordinates that they haven't specified, in particular when using gdk_pixbuf_get_from_surface(), which with this patch does respect device scale but not device offset.

Shall we add a new api then ?

After discussing this on IRC, I've made up my mind:

(1) I don't care what gdk_pixbuf_get_from_window() does

The API is outdated for all I care. And it doesn't work reliably anyway (try root window on Wayland). So I don't mind if it multiplies by scale if that's the most useful way to use it. Though strictly speaking, this is an ABI break.

I do however care that we don't introduce new APIs here (that support scale). Because we already know this function doesn't work. We should probably also deprecate this function.

(2) gdk_pixbuf_get_from_surface() should keep doing what it did before

gdk_pixbuf_get_from_surface(surface, x, y, w, h) should be identical to doing this in cairo:

result = gdk_pixbuf_new (w, h);
cr = cairo_create (result); /* just assume this works */
cairo_set_source_surface (cr, surface, x, y);
cairo_paint (cr);
return result;

If you want anything else you should do that yourself using cairo. Otherwise we just get into arguments about scale factors, bilinear vs nearest filters etc.

Created attachment 317058 [details] [review]
gdk_pixbuf_get_from_surface: restore old behavior

Thanks for the input. This patch restores the old behavior of
get_from_surface(), but keeps it for get_from_window(), as there's no way to
get unscaled window content otherwise.

Comment on attachment 317058 [details] [review]
gdk_pixbuf_get_from_surface: restore old behavior

Looks good.

I think I reverted the original patch. So this patch probably doesn't apply to master.

Also, should/do the docs say that you will get a pixbuf with width * gdk_window_get_scale_factor() pixels?

Thanks!

> Also, should/do the docs say that you will get a pixbuf with width *
> gdk_window_get_scale_factor() pixels?

Indeed. Changed it in the final commit.