<html>
<head>
<base href="https://bugzilla.gnome.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - On Wayland, application containing GtkGLArea stops responding if it's not on current workspace"
href="https://bugzilla.gnome.org/show_bug.cgi?id=769835#c18">Comment # 18</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - On Wayland, application containing GtkGLArea stops responding if it's not on current workspace"
href="https://bugzilla.gnome.org/show_bug.cgi?id=769835">bug 769835</a>
from <span class="vcard"><a href="page.cgi?id=describeuser.html&login=carlosg%40gnome.org" title="Carlos Garnacho <carlosg@gnome.org>"> <span class="fn">Carlos Garnacho</span></a>
</span></b>
<pre>(In reply to Emmanuele Bassi (:ebassi) from <a href="show_bug.cgi?id=769835#c17">comment #17</a>)
<span class="quote">> Review of <span class=""><a href="attachment.cgi?id=342410&action=diff" name="attach_342410" title="wayland: Disable EGL swap interval">attachment 342410</a> <a href="attachment.cgi?id=342410&action=edit" title="wayland: Disable EGL swap interval">[details]</a></span> <a href='review?bug=769835&attachment=342410'>[review]</a> [review]:
>
> ::: gdk/wayland/gdkglcontext-wayland.c
> @@ +441,3 @@
> +static gboolean
> +egl_config_can_disable_swap_interval (GdkDisplay *display,
> + const EGLConfig *config)
>
> EGLConfig is already a pointer; I don't think you need to pass it as a
> pointer…</span >
Hmm, I see. I was confused by the weird memory management at:
<a href="https://git.gnome.org//browse/gtk+/tree/gdk/wayland/gdkglcontext-wayland.c#n410">https://git.gnome.org//browse/gtk+/tree/gdk/wayland/gdkglcontext-wayland.c#n410</a>
where it seems to return stuff from a freed block, knowing that EGLConfig is a
pointer itself makes sense then.
<span class="quote">>
> @@ +446,3 @@
> + EGLint interval;
> +
> + if (!eglGetConfigAttrib (display_wayland->egl_display, *config,
>
> … especially since you're just dereferencing it here.
>
> @@ +483,3 @@
> return NULL;
>
> + if (!egl_config_can_disable_swap_interval (display, &config))
>
> I'm not convinced this is an appropriate action to take.
>
> If swap interval cannot be disabled then we should fall back to the existing
> behaviour; it's not optimal, but saying "you can't have GL because your
> driver does not allow disabling swap interval" seems to me far more drastic
> than necessary.
>
> Just save the return value inside the GdkGLContext and emit a warning or a
> debug message.</span >
Sure, question is when it's more appropriate to warn then. Before it locks
presumably...
<span class="quote">>
> @@ +543,3 @@
> eglMakeCurrent(display_wayland->egl_display, EGL_NO_SURFACE,
> EGL_NO_SURFACE,
> EGL_NO_CONTEXT);
> + eglSwapInterval (dpy, 0);
>
> Not sure it's needed, here; we're unbinding everything, which means there's
> no context and no surface to swap.</span >
Right, I was unsure about this one.
Thanks for the review!</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are on the CC list for the bug.</li>
</ul>
</body>
</html>