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

[Daniel Colascione]

Eli Zaretskii <address@hidden> writes:

>> From: Daniel Colascione <address@hidden>
>> Cc: address@hidden
>> Date: Fri, 21 Oct 2016 11:27:29 -0700
>> 
>> Eli Zaretskii <address@hidden> writes:
>> 
>> >> Cc: address@hidden
>> >> From: Daniel Colascione <address@hidden>
>> >> Date: Fri, 21 Oct 2016 04:04:21 -0700
>> >> 
>> >>  > 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.)
>> >
>> > The code is more clear with the test before the call, otherwise the
>> > code reader needs to look in the hook to understand when it's a no-op.
>> 
>> But the test (of whether the hook exists) _is_ before the call. What are
>> you proposing?
>> 
>>     some_backend_needs_flush = False
>>     for backend in font_back_ends:
>>       if has_flush_hook(backend):
>>         some_back_end_needs_flush = True
>>     if some_back_end_needs_flush:
>>       for backend in font_back_ends:
>>         if has_flush_hook(backend):
>>           backend.flush_hook()
>>     
>> That's silly.
>
> Let's assume that neither of us makes silly proposals, okay?

Sorry --- I meant the comment in a more syllogistic way:

  1. I understand the proposal to be silly
  2. We don't make silly proposals

  Therefore, my understanding is flawed

>> Can you elaborate on what you consider a cleaner approach?
>
> I meant the test for the lone font backend that needs this kludge, not
> for the existence of the method.  And also a comment that tells it's a
> kludge whose need we understand only empirically.  I'd even prefer a
> direct call to the function when the font backend is the one which
> needs it, rather than having a method created for the benefit of a
> single backend, whose necessity we don't really understand.

So we want to write?

  for font_backend in frame.font_backends:
    if instanceof(font_backend, XftBackend):
      xft_refresh_hack()

That still strikes me as less clean, especially considering we'll have
to provide the xft_refresh_hack to general-purpose code.

> When I read the code which tests whether a backend has a method, I
> need to look up the methods of the backend(s) I'm interested in to
> know whether they have such a method.  When a method is defined for a
> single backend, most of those searches will produce a trivial result,
> i.e. a waste of my time.  I prefer that we make this evident at the
> place of the call instead.
>
>> >>  >> +  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.
>> >
>> > That might be so, but what you say above is in no way apparent from
>> > looking at the code.  IME, it is very important to have the idea when
>> > something is being done and when not just by looking at the code in
>> > redisplay_internal; having to find that out by realizing that some
>> > flag is set in update_end and then tested in the hook makes the code
>> > more subtle and its maintenance harder.  It's not like keeping this
>> > detail from redisplay_internal makes this detail local to some
>> > functions, or somesuch, so there's really no reason to conceal it,
>> > IMO.
>> 
>> This sentiment strikes me as being analogous to the old "no use of hooks
>> in Emacs internals" line --- yet we have facilities like syntax-ppss
>> that rely on hooks in core, and it's worked out fine.
>
> I'm not saying the code won't work.  I'm saying it could make its
> purpose more evident, which is better for long-term maintainability.

Longer-term, I think we need to move toward greater, not
lesser, modularity. With some explanatory comments,

Besides, there's no reason other window systems can't use this
hook. Looking briefly at ns_update_end, for example, I see that we call
flushWindow; we shouldn't have to do that until redisplay_end_hook.

>> We need to do a buffer flip at the end of redisplay for each frame on
>> which update_end was called during redisplay.
>
> Even if update_end was called for a frame whose update was "paused",
> i.e. whose redisplay was interrupted by incoming input?

Right now, we do a buffer flip unconditionally except in the case that
we `goto end_of_redisplay`. I think that's a bug actually --- but I have
to understand more of what this logic is actually doing.

Why _do_ we have a path that short-circuits the rest of the redisplay
code? What would happen if we just removed it? It appears to be some
kind of optimization, and I'm not sure it's actually necessary
(especially since, according to the comment, we disable it anyway in the
case of a blinking cursor).

I suppose you could make an argument for not doing a buffer flip if
redisplay is interrupted by input, but I'm honestly not sure what might
go wrong there, and my preference would be to act as much like the
single-buffered case as possible and default, if we're unsure, to
showing the results of our painting efforts to the user.

>> If someone calls update_end _outside_ redisplay, we should do a
>> buffer flip immediately. The code I've sent is the cleanest way of
>> implementing this model short of changing how update_begin and
>> update_end work.
>
> I'm asking whether flipping the buffer for a frame that wasn't updated
> or was updated partially can do some harm, like produce incorrect
> display.

Probably, but I don't think it can be worse than what we currently
have. I'm trying to be conservative and hold as few updates in the back
buffer as possible.  The only reason I added the end_of_redisplay stuff
is that the resulting flicker reduction in the window-resize case is so
dramatic that I think it's worth the extra complexity.

>> I think thhat what you're proposing is a layering violation. It will
>> make maintenance harder. The only facility that cares about the
>> has-a-frame-been-updated state is the X11 double-buffered back-end,
>
> My problems start with the name of the hook, which doesn't hint at all
> that only double-buffered X11 back-end cares about that.  If the hook
> was called something like double_buffer_flip_hook, or was compiled
> only if HAVE_XDBE is defined, this issue would go away.

So let's call it double_buffer_flip_hook.

>> making xdisp track this state makes everything more complicated,
>> especially because xdisp already has a "needs redisplay" flag and
>> shouldn't need to keep track of a separate "needs buffer flip" flag.
>> It shouldn't have to care. That's not its job.
>
> I don't know why you decided that it isn't xdisp.c's job.  It already
> has a lot of flags that other parts set to get xdisp.c do or not do
> certain parts of its job.  Why not let it test a flag here as well?
> Such a test will tell the reader that the call to the hook is only
> done when a flag (with hopefully some descriptive name) is set, so the
> reader can understand what the hook is doing and under what
> circumstances without actually looking at the hook, which involves
> first finding out which frame types support it, and what exactly does
> it do.
>
> IOW, if the code speaks for itself, it makes maintenance of this
> hyper-convoluted piece of Emacs easier, less time consuming, and less
> error prone.

This code is an adventure.

>> >> Or are you worried about the function call overhead? That, as I 
>> >> mentioned above, is trivial.
>> >
>> > No, I worry about maintainability of the display code and about
>> > lowering the risk of bugs introduced due to such subtleties.
>> >
>> 
>> I'm also worried about maintainability: that's why I don't want to make
>> redisplay_internal any more of a big ball of mud than it already is.
>
> It's too late for that.  It's already an extremely complex and hard to
> understand piece of code.  Hiding more information from its logic
> makes the code harder to maintain, not easier.  We are not talking
> about highly-modular package, where the logic is kept local to each
> module, and the interfaces between them are kept to the absolute
> minimum.  xdisp.c _knows_ a lot about how xterm.c and xfns.c work.
> You cannot disengage them without a complete rewrite.

Well, it also works with w32term and nsterm, so the coupling can't be
complete. :-)

> So the principles you are trying to apply, while a good idea in
> general, in this particular case make code harder to understand and
> develop.
>
> Of course, it's not a catastrophe, so if you are going to go to the
> barricades over this, I won't fight you.  I just hope you take my gray
> hair from many readings of redisplay_internal as some evidence that I
> know what I'm talking about.  The number of times I needed to go deep
> into the called functions just to realize that the code does something
> completely different from what it looked on the redisplay_internal
> level is beyond imagination.

I'm beginning to see your point. I just don't agree that the problem is
intractable. We're going to need to clean this stuff up, especially if
(in the distant future) we want to support multiple window systems in
the same Emacs build.

>
>> >>  >> +#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?
>> >
>> > How can it fail transiently?  And why turning off supports_xdbe in
>> > that case would mean trouble?
>> 
>> It's an allocation. Allocations can fail. And XDBE isn't guaranteed to
>> work for all visuals.
>
> How much memory is being allocated, and correspondingly what is the
> probability this allocation could fail on a healthy system?

I don't know --- I've never seen it fail. I'm just applying general
memory-allocation heuristics here.  DBE can also fail if the visual
we're using for a window 

>
> And what about my second question?
>
> The downside of your proposed code is that we will try to turn on XDBE
> each time hence, and each attempt means a potentially costly series of
> calls to catch X errors, and the XdbeAllocateBackBufferName itself.
> I'm asking whether it wouldn't be better to simply give up on XDBE in
> such a case for that frame.  It could be much cheaper, no?  Just a
> thought.

It's only on frame creation though, and frame creation is both rare and
already expensive.

By the way: aren't most calls to x_catch_errors already buggy? AFAICT,
they don't generally call x_sync first.  The purpose of this mechanism
is to run a section of code in such a way that Emacs doesn't die if
something goes wrong in Xlib, but since Xlib error reporting is
asynchronous anyway, use of x_catch_errors without a preceding x_sync
(as I have in my code) can silently swallow X errors that we _do_ want
to kill Emacs.

Anyway, the double-buffer code has a few kinks I need to work out. I'll
send another version of the patch after I figure out what's going on.

  1. [must fix] during frame creation, on one of my computers but not
  the other, I momentarily see an all-black frame with a small white
  square, and only a few hundred ms later does the normal frame content
  get draws

  2. [must fix] on the same system, and not the other, after resuming
  the system from sleep, Emacs frames momentarily display all white
  before the usual frame contents get filled in

  3. [should fix] on the same system, and not the other, resizing the a
  frame interactively still produces some flicking, particulary in the
  modeline, but much less than without the patch entirely.
  This flickering appears to have something to do with XRender, since if
  I force everything to use X11 core rendering instead, I don't see any
  flickering at all.