[Wayland-bugs] [Bug 744932] Wrong (ultra tiny/small) cursor size on HiDPI screen

[mutter (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=744932

--- Comment #110 from Owen Taylor <otaylor at redhat.com> ---
Review of attachment 309538:

Didn't quite finish going through meta-wayland-pointer.[ch], but here is a
first pass at some comments... I think the main one is that I'd prefer if the
"buffer swapping" in meta-cursor-renderer-native.c was done in a more logical
fashion - currently it seems to flip things around "randomly" and hope for the
best.

::: src/backends/meta-cursor-renderer.c
@@ +65,2 @@
   if (priv->displayed_cursor && !priv->handled_by_backend)
+    texture = meta_cursor_sprite_get_cogl_texture (priv->displayed_cursor);

Shouldn't this use cursor_sprite? - or alternatively, why is cursor_sprite
passed in when it seems like it will always be priv->displayed_cursor?

@@ +91,3 @@
+MetaRectangle
+meta_cursor_renderer_calculate_rect (MetaCursorRenderer *renderer,
+                                     MetaCursorSprite *cursor_sprite)

In general we don't use struct returns in Mutter or elsewhere in GNOME -
there's not really a big reason for that with modern ABIs... I'd probably
prefer consistency, but don't care that much, so feel free to leave this.

@@ +111,3 @@
+  return (MetaRectangle) {
+    .x = (int)(priv->current_x - (hot_x * texture_scale)),
+    .y = (int)(priv->current_y - (hot_y * texture_scale)),

Seems rounding would be more appropriate

::: src/backends/meta-cursor-renderer.h
@@ +44,3 @@

+  gboolean (* update_cursor) (MetaCursorRenderer *renderer,
+                              MetaCursorSprite *cursor_sprite);

I found it confusing reading the code that:

 update_cursor

Updates the internal state of the *renderer* based on the cursor_sprite. While

 realize_cursor_for_wl_buffer

Updates the state of the *cursor*. 

I don't really understand why the change to add the cursor_sprite parameter was
needed, and I think it contributes to the problem by making the non-parallel
methods in the interface look more parallel.

::: src/backends/native/meta-cursor-renderer-native.c
@@ +62,3 @@
+{
+  guint current_bo;
+  struct gbm_bo *bos[2];

A comment about why the two bos would be *very* useful.

This was discussed some in IRC and email, and it turns out that the likely
situation that was going wrong and causing glitches was:

 We call drmModeSetCursor2() with buffer A
 We call gbm_bo_destroy() on the old buffer
 Instead of freeing the old buffer, the intel libdrm driver stores it in a
client-side pool.
 We call gbm_bo_create() and get the *same* BO back - which is still being
scanned out as the cursor
 We modify it, causing the glitch

I think for now we can consider that drmModeSetCursor2() completely replaces
any references to previously set cursors and at that point the old buffer can
be reused, so that mostly the approach you are taking here should work. 

As was noted in your email, the logic here isn't really right for handling that
,because the "flip" is done when the cursor is changed, rather than at the time
the drmModeSetCursor2() call is called. To me, this makes the code here
extremely hard to understand ... it logically doesn't hang together. Honestly,
I'd like to see that fixed before this is committed even if it mostly works
now.

@@ +144,3 @@
+      bo = get_current_cursor_sprite_gbm_bo (cursor_sprite);
+      dirty = gbm_bo_get_user_data (bo);
+      if (!force && bo == crtc->cursor_renderer_private && !*dirty)

What's to prevent:

 Old cursor sprite is freed, including the bo's
 New cursor sprite is allocated with the *same address*

This may seem rather unlikely - and it is - but it could happen, and we should
in general not be writing code that compares to potentially freed pointers.

====

I think if you switch when you call drmModeSetCursor2 you don't need the dirty
flag business - you just need a flag in the cursor_sprite saying whether you
have a pending new buffer you need to set.

@@ +210,3 @@
 static gboolean
+should_have_hw_cursor (MetaCursorRenderer *renderer,
+                       MetaCursorSprite *cursor_sprite)

Mutter is supposed to have GTK+-style annoying alignment of function parameters
;-)

@@ +222,3 @@
+
+  if (meta_cursor_sprite_get_texture_scale (cursor_sprite) != 1)
+    return FALSE;

Some day we can think about sticking cursor in planes on relevant hardware
(that's what happens internally in the kernel for Intel hardware anyways...)
and then we can get the hardware to scale cursors up/down when necessary.

::: src/core/boxes-private.h
@@ +41,3 @@
+gboolean meta_rectangle_contains_point  (const MetaRectangle *rect,
+                                         int                  x,
+                                         int                  y);

Seems to duplicate the POINT_IN_RECT() macro in common.h

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-bugs/attachments/20150903/17d50933/attachment.html>