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

[Daniel Colascione <dancol@dancol.org>]

On 10/21/2016 01:49 AM, Eli Zaretskii wrote:
 >> From: Daniel Colascione <dancol@dancol.org>
 >> 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!

Thanks for taking a look.

 >
 >> * 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 <dancol@dancol.org>
 >> 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.

Oh, of course. I figured it was overkill for the first version. I'll add 
the usual ceremonial bits for the next version

 >
 > 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?

Symmetry with the other extension clauses?

 > 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.

I thought about that, but didn't want to overwhelm users with noise. 
I'll add some more user visibility here.

The other question I had for myself is whether it makes sense to let 
users enable and disable this functionality at runtime: it's hard to 
think of a legitimate use case that doesn't involve a bug somewhere, and 
I'd rather fix the bugs.

 >
 >> 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.

Maybe I need to come up with a better name for what this hook does. It's 
specific to the XftDraw surface issue I mentioned in the preamble to the 
patch: without this code, rendering of text stops working (text 
rendering silently fails to happen) after a frame resize.

We're not flushing the caches implicated in bigs 24634 and 24565. I'll 
add some comments explaining what's going on.

I suspect it's a bad interaction between XRenderCreatePicture (which Xft 
uses internally) and the behind-the-scenes pixmap adjustment that DBE is 
doing that drives us to this hack.

 > 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.

We're testing whether the hook function is non-NULL before calling into 
it, so only that one backend gets the call. In any case, the overhead is 
trivial --- it's one indirect function call compared to all of 
redisplay. (Every call into a shared library is the same indirect jump.)

 > 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).

I blame my email client.

 >> +  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?

The hook only does something in the case where someone called update_end 
and we demurred on actually flipping the buffer because we knew we were 
in redisplay and would be getting redisplay_end_hook shortly. That is, 
if we update only one frame, we're only going to do one buffer flip.

Or are you worried about the function call overhead? That, as I 
mentioned above, is trivial.

 > Also, should
 > we distinguish between visible and iconified frames?

If we do, we should do it in the code that performs the updates, not the 
code (the snippet immediately above) that publishes the updates we've 
already done.

 >
 >> +#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?

supports_xdbe is whether XDBE is supported on a connection at all. What 
if XdbeAllocateBackBufferName fails transiently?


 >> +#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.

If we put xdbe_major and xdbe_minor at function level, the names leak 
into function scope. With braces, they exist only around the call to 
XdbeQueryExtension

 >
 >> 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.)

I was just reluctant to further bloat the structure. Making these 
members exist unconditionally is fine.

 >
 >> +/* 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.

The point is to make the build fail in the non-XDBE case if you try to 
use it as an lvalue. If draw_desc is going to actually exist in the 
structure in all cases, the strange definition is moot, since we'll 
always use the other one.