Created attachment 335120 [details] [review]
gl: Correctly scale height when calculating clipped source y coordinate

The v coordinate was being miscalculated, causing the origin for the source to
be incorrectly offset on the y axis.

Ha! Good catch! Should it also take in account `buffer_scale`?

(For reference, this is in the context of avoiding glReadPixels() in WebKit under Wayland, see https://bugs.webkit.org/show_bug.cgi?id=161530)

Pretty sure this is not the desired behaviour — actually, I'm pretty sure that this patch will break GtkGLArea if applied.

The idea is that you're passing a texture (or a renderbuffer) object backing a framebuffer object, as well as its size, to gdk_cairo_draw_from_gl(); you have the responsibility to create the FBO and set it up so that it handles different scaling factors — for instance, see gtkglarea.c:

  https://git.gnome.org/browse/gtk+/tree/gtk/gtkglarea.c#n440

Then, GtkGLArea will use the scaled size of the target:

  https://git.gnome.org/browse/gtk+/tree/gtk/gtkglarea.c#n705

Review of attachment 335120 [details] [review]:

The passed in width and height should already by scaled.

The documentation should really make it clearer.

Thanks, let me give that a go!

I thought it was the expected behaviour because the function applies the window_scale to height and width in a lot of places. Also, I would expect to see issues on the x axis as well if the problem was with how the texture was made available, wouldn't it be the case? In any case, it does break GLArea.

Created attachment 335139 [details] [review]
This patch fixes GLArea by addressing Emanuele Aina's catch that buffer_scale had to be taken into consideration. I believe this patch is correct now. The code is already prepared to handle buffer and window having different scales. In our case, the textu

If you look at the code that we are touching with this patch, it is always reasoning about sizes with window scale applied. For instance:

          dest.x = dx * window_scale;
          dest.y = dy * window_scale;
          dest.width = width * window_scale / buffer_scale;
          dest.height = height * window_scale / buffer_scale;

Note that it removes the buffer scale and applies the window scale. When the clipped_src variables are created, everything also has window scale applied:

              int clipped_src_x = x + (dest.x - dx * window_scale);
              int clipped_src_y = y + (height - dest.height - (dest.y - dy * window_scale));

The only variable lacking it is height, and it has dest.height, which is window scale, being subtracted from it, which is probably not what we want. So I scaled it to window scale. It broke GLArea because it provides a buffer with a scale of 2 - which we then have to remove, so we would have this:

              int clipped_src_x = x + (dest.x - dx * window_scale);
              int clipped_src_y = y + (height * window_scale / buffer_scale - dest.height - (dest.y - dy * window_scale));

However, if you look at the definition of dest.height you'll see that we are now subtracting dest.height from… dest.height. So it turns out we do not want that bit, we should just do what we do for the x axis. The reason why it worked before is buffer_scale == window_scale. Does this make more sense to you?

> if you look at the definition of dest.height you'll see that we are now subtracting dest.height from… dest.height

Yes and no. :)

&dest is passed through gdk_rectangle_intersect() so dest.height may no longer have its original value.

It would make sense to store the scaled `height` in a variable to be used to initialize dest.height and again in clipped_src_y.

Erm. You're right, of course! We need that calculation to get the offset right due to the y axis being flipped. Also, the x and y variables outside the parens are not scaled to window scale. OK. So perhaps the problem is the opposite: we should be using buffer_scale for all of this. This would make sense given we use these variables with buffer-scaled variables later on. I'll be back =)

Created attachment 335188 [details] [review]
gl: Fix calculation of clipped source y coordinate

OK, I think this is as far as I managed to go. I brought back the subtraction em mentioned, which we need for drawing clipped. I tried improving the naming of the variables, but I am afraid I am not being able to come up with a better way of structuring the code. Anyways, this makes our code work without breaking GLArea.

After additional investigation I did find an issue with my code, so I guess ebassi was right on the spot. Thanks!