Re: RFC: flicker-free double-buffered Emacs under X11

[Eli Zaretskii]

> From: Daniel Colascione <address@hidden>
> Date: Thu, 20 Oct 2016 18:32:01 -0700
> 
> This patch teaches Emacs how to use the X11 DOUBLE-BUFFER extension to 
> avoid showing the user incomplete drawing results. Without this patch, I 
> can make Emacs flicker like crazy by running isearch for a piece of text 
> unique in a buffer and holding down C-s. With this patch, Emacs does not 
> flicker no matter what I do to it.
> 
> The patch also stops flickering that occurs when using the "solid 
> resizing" feature of some window managers --- i.e., when the WM redraws 
> windows as the user drags their edges, as opposed to displaying some 
> kind of bounding-box in lieu of the actual window contents.

Thanks!

> * We do a buffer flip at the end of redisplay instead of in 
> x_update_end() so the user never sees the completely-cleared state that 
> we enter immediately after clear_garbaged_frames(). x_update_end() does 
> do a buffer flip if it's called outside redisplay. I've added a new 
> terminal hook to support this hack.

What happens in the case where redisplay_internal did its job, but
update_frame was interrupted by incoming input?  Won't that flip the
buffers unnecessarily?

I have some comments below.  Apologies if some of them stem from my
relative ignorance of the issues involved in this changeset.

> commit 15fdd8f63533201f05627ede634a8f5ae4757d7e
> Author: Daniel Colascione <address@hidden>
> Date:   Thu Oct 20 16:50:54 2016 -0700
> 
>      Add double-buffered output support to Emacs

Please add ChangeLog style commit log entries about each new and
modified function/macro.

Please also provide a NEWS entry about the new feature, in the
"Installation Changes" section.

> +AC_SUBST(XDBE_CFLAGS)
> +AC_SUBST(XDBE_LIBS)

Do we need XDBE_CFLAGS?  They seem to be unused?

Also, I think the fact that Xdbe extensions are used should be shown
in the summary displayed by the configure script when it finishes, and
XDBE should be added to the list in config_emacs_features, so that it
appears in EMACS_CONFIG_FEATURES when appropriate.

> diff --git a/src/dispnew.c b/src/dispnew.c
> index 70d4de0..8f81cee 100644
> --- a/src/dispnew.c
> +++ b/src/dispnew.c
> @@ -2999,6 +2999,7 @@ redraw_frame (struct frame *f)
>   {
>     /* Error if F has no glyphs.  */
>     eassert (f->glyphs_initialized_p);
> +  font_flush_frame_caches (f);
>     update_begin (f);
>     if (FRAME_MSDOS_P (f))
>       FRAME_TERMINAL (f)->set_terminal_modes_hook (FRAME_TERMINAL (f));
> diff --git a/src/font.c b/src/font.c
> index f8e6794..033995e 100644
> --- a/src/font.c
> +++ b/src/font.c
> @@ -5275,6 +5275,16 @@ font_deferred_log (const char *action, 
> Lisp_Object arg, Lisp_Object result)
>   }
> 
>   void
> +font_flush_frame_caches (struct frame *f)
> +{
> +  struct font_driver_list *list;
> +
> +  for (list = f->font_driver_list; list; list = list->next)
> +    if (list->on && list->driver->flush_frame_caches)
> +      list->driver->flush_frame_caches (f);
> +}

Why do we need this flushing?  Is it relevant to the patch, and if so
why?

I'm asking because we had some bad effects with some fonts due to
flushing the frame's font caches, see bugs 24634 and 24565, for
example, so my bother is that this will reintroduce those problems.

Also, the call to font_flush_frame_caches is unconditional, although
only one font back-end supports it.  That seems to incur a gratuitous
overhead of a function call for the other font back-ends.

In any case, please add a commentary to each function you add with a
summary of what it does.

> @@ -1233,6 +1237,7 @@ xg_create_frame_widgets (struct frame *f)
>        by callers of this function.  */
>     gtk_widget_realize (wfixed);
>     FRAME_X_WINDOW (f) = GTK_WIDGET_TO_X_WIN (wfixed);
> +  set_up_x_back_buffer (f);

There's some strange misalignment of indentation here (and in a few
other places).

> +  FOR_EACH_FRAME (tail, frame)
> +    {
> +      struct frame *f = XFRAME (frame);
> +      if (FRAME_TERMINAL (f)->redisplay_end_hook)
> +        (*FRAME_TERMINAL (f)->redisplay_end_hook) (f);
> +    }

This will call the hook for each frame, every redisplay cycle.  By
contrast, redisplay_internal many times updates only the selected
window of the selected frame.  Is calling the hook for all the frames
really needed, or should we only call it for the selected frame in the
latter case, or maybe just for frames that got updated?  Also, should
we distinguish between visible and iconified frames?

> +#ifdef HAVE_XDBE
> +  if (FRAME_DISPLAY_INFO (f)->supports_xdbe)
> +    {
> +      /* If allocating a back buffer fails, just use single-buffered
> +         rendering.  */
> +      x_sync (f);
> +      x_catch_errors (FRAME_X_DISPLAY (f));
> +      FRAME_X_DRAWABLE (f) = XdbeAllocateBackBufferName (
> +        FRAME_X_DISPLAY (f),
> +        FRAME_X_WINDOW (f),
> +        XdbeCopied);
> +      if (x_had_errors_p (FRAME_X_DISPLAY (f)))
> +        FRAME_X_DRAWABLE (f) = FRAME_X_WINDOW (f);

Shouldn't we turn off the supports_xdbe flag in the case of failure?

> +#ifdef HAVE_XDBE
> +  dpyinfo->supports_xdbe = false;
> +    {
> +      int xdbe_major;
> +      int xdbe_minor;
> +      if (XdbeQueryExtension (dpyinfo->display, &xdbe_major, &xdbe_minor))
> +        dpyinfo->supports_xdbe = true;
> +    }
> +#endif

No need for braces here, since we now require a C99 compiler.

> diff --git a/src/xterm.h b/src/xterm.h
> index 675a484..cb1aa1d 100644
> --- a/src/xterm.h
> +++ b/src/xterm.h
> @@ -475,6 +475,10 @@ struct x_display_info
>   #ifdef USE_XCB
>     xcb_connection_t *xcb_connection;
>   #endif
> +
> +#ifdef HAVE_XDBE
> +  bool supports_xdbe;
> +#endif
>   };
> 
>   #ifdef HAVE_X_I18N
> @@ -527,6 +531,17 @@ struct x_output
>        and the X window has not yet been created.  */
>     Window window_desc;
> 
> +#ifdef HAVE_XDBE
> +  /* The drawable to which we're rendering.  In the single-buffered
> +     base, the window itself.  In the double-buffered case, the
> +     window's back buffer.  */
> +  Drawable draw_desc;
> +
> +  /* Set to true when we need a buffer flip.  We do a buffer flip only
> +     at the end of redisplay in order to minimize flicker.  */
> +  bool need_buffer_flip;
> +#endif

We had bad experience with conditionally compiled struct members.  Is
it possible to have these members always defined, and set them to some
no-op values when XDBE is not supported?  (In general, run-time tests
should IMO always be preferred to compile-time tests, as the former
make extensions and future development easier.)

> +/* Return the drawable used for rendering to frame F.  */
> +#ifdef HAVE_XDBE
> +#define FRAME_X_DRAWABLE(f) ((f)->output_data.x->draw_desc)
> +#else
> +#define FRAME_X_DRAWABLE(f) (0,(FRAME_X_WINDOW (f)))
                               ^^^^^^^^^^^^^^^^^^^^^^^^

Why such a strange definition?  Won't it cause compiler warnings in
some cases, like when it's used as an lvalue?  We use some quite
paranoid warning switches on master.

If there are problems to have a simple definition for this macro in
the non-XDBE case, I'd prefer to have inline function(s) for the
problematic cases (e.g., comparison), rather than a tricky macro.

Thanks again for working on this.