Re: MS Windows double buffering

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Sat, 30 Apr 2022 13:41:25 +0800
> 
> I fixed a few minor problems with this, synchronized parts of the tool
> bar code with X to fix a bug, and installed the resulting changes, since
> they seems to be working fine for me and eliminates all of the dreadful
> flickering that was previously present on MS Windows.

Thanks!

However, I don't understand why you went ahead without waiting for the
review of the patches you posted.  What's the rush?  The result can be
only one: the changeset is spread on a larger set of commits, and
makes it harder to understand what changed and why, when later these
questions are asked.

Please don't act as if we are under some time pressure when we really
aren't.

My review is below.

> +static void
> +w32_set_inhibit_double_buffering (struct frame *f,
> +                               Lisp_Object new_value,
> +                               Lisp_Object old_value)

The old_value argument is unused, which should at least be mentioned,
if not marked with an explicit "unused" attribute.

Also, this function is a frame redisplay-interface method, so it
should have a comment describing its functionality.

>  static void
> +w32_show_back_buffer (struct frame *f)
> +{
> +  struct w32_output *output;
> +  HDC raw_dc;
> +
> +  output = FRAME_OUTPUT_DATA (f);
> +
> +  enter_crit ();
> +
> +  if (output->paint_buffer)
> +    {
> +      raw_dc = GetDC (output->window_desc);
> +
> +      if (!raw_dc)
> +     emacs_abort ();
> +
> +      BitBlt (raw_dc, 0, 0, FRAME_PIXEL_WIDTH (f),
> +           FRAME_PIXEL_HEIGHT (f),
> +           output->paint_dc, 0, 0, SRCCOPY);
> +      ReleaseDC (output->window_desc, raw_dc);
> +
> +      output->paint_buffer_dirty = 0;
> +    }
> +
> +  leave_crit ();
> +}
> +
> +void
> +w32_release_paint_buffer (struct frame *f)
> +{
> +  /* Delete the back buffer so it gets created
> +     again the next time we ask for the DC.  */
> +
> +  enter_crit ();
> +  if (FRAME_OUTPUT_DATA (f)->paint_buffer)
> +    {
> +      SelectObject (FRAME_OUTPUT_DATA (f)->paint_dc,
> +                 FRAME_OUTPUT_DATA (f)->paint_dc_object);
> +      ReleaseDC (FRAME_OUTPUT_DATA (f)->window_desc,
> +              FRAME_OUTPUT_DATA (f)->paint_buffer_handle);
> +      DeleteDC (FRAME_OUTPUT_DATA (f)->paint_dc);
> +      DeleteObject (FRAME_OUTPUT_DATA (f)->paint_buffer);
> +
> +      FRAME_OUTPUT_DATA (f)->paint_buffer = NULL;
> +      FRAME_OUTPUT_DATA (f)->paint_dc = NULL;
> +      FRAME_OUTPUT_DATA (f)->paint_buffer_handle = NULL;
> +    }
> +  leave_crit ();
> +}

These two functions should do nothing and return immediately if
double-buffering is disabled.  They currently will try to enter the
critical section before returning, which is an unnecessary slowdown.
At least w32_release_paint_buffer is called also when double-buffering
is disabled, AFAIU, so it should return immediately in that case.

Alternatively, make sure that these functions are called only if
double-buffering is in effect, but in that case the additional tests
inside the functions are redundant.

> @@ -4093,7 +4112,9 @@ w32_wnd_proc (HWND hwnd, UINT msg, WPARAM wParam, 
> LPARAM lParam)
>      {
>      case WM_ERASEBKGND:
>        f = w32_window_to_frame (dpyinfo, hwnd);
> -      if (f)
> +
> +      enter_crit ();
> +      if (f && !FRAME_OUTPUT_DATA (f)->paint_buffer)
>       {
>         HDC hdc = get_frame_dc (f);
>         GetUpdateRect (hwnd, &wmsg.rect, FALSE);
> @@ -4107,6 +4128,7 @@ w32_wnd_proc (HWND hwnd, UINT msg, WPARAM wParam, 
> LPARAM lParam)
>                    wmsg.rect.right, wmsg.rect.bottom));
>  #endif /* W32_DEBUG_DISPLAY */
>       }
> +      leave_crit ();
>        return 1;
>      case WM_PALETTECHANGED:
>        /* ignore our own changes */

Here and elsewhere in the modified code, I'd like to have all the
changes that affect the no-double-buffering code to be conditioned on
some variable exposed to Lisp.  That way, if and when someone reports
a problem that could be related to this feature, we could easily get
back to exactly the old code as it was before the changeset, and see
if the problem is indeed due to this change.  For example, entering
the critical section above is one such change.

We could then remove that variable in some future Emacs version, when
we are sure the new code is reasonably bug-free.

>  /* Draw truncation mark bitmaps, continuation mark bitmaps, overlay
>     arrow bitmaps, or clear the fringes if no bitmaps are required
> @@ -2872,8 +2932,7 @@ w32_scroll_run (struct window *w, struct run *run)
>  {
>    struct frame *f = XFRAME (w->frame);
>    int x, y, width, height, from_y, to_y, bottom_y;
> -  HWND hwnd = FRAME_W32_WINDOW (f);
> -  HRGN expect_dirty;
> +  HDC hdc;

This function, which is an important part of redisplay, was basically
rewritten from scratch.  As explained above, please add back the
deleted old code, conditioned under the variable I described there, so
that we could test whether the new code is responsible for some
problem that users may report.

> @@ -4809,6 +4838,9 @@ w32_scroll_bar_clear (struct frame *f)
>  {
>    Lisp_Object bar;
>  
> +  if (FRAME_OUTPUT_DATA (f)->paint_buffer)
> +    return;

There should be a comment here explaining why we return in that case.

> @@ -4969,24 +5003,33 @@ w32_read_socket (struct terminal *terminal,
>               }
>             else
>               {
> -               /* Erase background again for safety.  But don't do
> -                  that if the frame's 'garbaged' flag is set, since
> -                  in that case expose_frame will do nothing, and if
> -                  the various redisplay flags happen to be unset,
> -                  we are left with a blank frame.  */
> -               if (!FRAME_GARBAGED_P (f) || FRAME_PARENT_FRAME (f))
> +               enter_crit ();
> +               if (!FRAME_OUTPUT_DATA (f)->paint_buffer)
>                   {
> -                   HDC hdc = get_frame_dc (f);
> -
> -                   w32_clear_rect (f, hdc, &msg.rect);
> -                   release_frame_dc (f, hdc);
> +                   /* Erase background again for safety.  But don't do
> +                      that if the frame's 'garbaged' flag is set, since
> +                      in that case expose_frame will do nothing, and if
> +                      the various redisplay flags happen to be unset,
> +                      we are left with a blank frame.  */
> +
> +                   if (!FRAME_GARBAGED_P (f) || FRAME_PARENT_FRAME (f))
> +                     {
> +                       HDC hdc = get_frame_dc (f);
> +
> +                       w32_clear_rect (f, hdc, &msg.rect);
> +                       release_frame_dc (f, hdc);
> +                     }
> +
> +                   expose_frame (f,
> +                                 msg.rect.left,
> +                                 msg.rect.top,
> +                                 msg.rect.right - msg.rect.left,
> +                                 msg.rect.bottom - msg.rect.top);
> +                   w32_clear_under_internal_border (f);
>                   }
> -               expose_frame (f,
> -                             msg.rect.left,
> -                             msg.rect.top,
> -                             msg.rect.right - msg.rect.left,
> -                             msg.rect.bottom - msg.rect.top);
> -               w32_clear_under_internal_border (f);
> +               else
> +                 w32_show_back_buffer (f);
> +               leave_crit ();
>               }
>           }
>         break;

I don't understand why you enter the critical section here:
w32_show_back_buffer does that internally, and the old code didn't
need that.

> @@ -5840,6 +5885,17 @@ w32_read_socket (struct terminal *terminal,
>           }
>         count++;
>       }
> +
> +      /* Event processing might have drawn to F outside redisplay.  If
> +         that is the case, flush any changes that have been made to
> +         the front buffer.  */
> +
> +      if (f && FRAME_OUTPUT_DATA (f)->paint_buffer_dirty
> +       /* WM_WINDOWPOSCHANGED makes the buffer dirty, but there's
> +          no reason to flush it here, and that also causes
> +          flicker.  */
> +       && !f->garbaged && msg.msg.message != WM_WINDOWPOSCHANGED)
> +     w32_show_back_buffer (f);

This is not clean, since it references a WM_ message in a place other
than where its main processing happens.  Please change it so
processing of WM_WINDOWPOSCHANGED raises some flag (which is otherwise
always false), and have this code here test that flag.

> --- a/src/w32xfns.c
> +++ b/src/w32xfns.c
> @@ -136,13 +136,13 @@ select_palette (struct frame *f, HDC hdc)
>      f->output_data.w32->old_palette = NULL;
>  
>    if (RealizePalette (hdc) != GDI_ERROR)
> -  {
> -    Lisp_Object frame, framelist;
> -    FOR_EACH_FRAME (framelist, frame)
>      {
> -      SET_FRAME_GARBAGED (XFRAME (frame));
> +      Lisp_Object frame, framelist;
> +      FOR_EACH_FRAME (framelist, frame)
> +     {
> +       SET_FRAME_GARBAGED (XFRAME (frame));
> +     }
>      }
> -  }
>  }

This looks like whitespace change?  Or did I miss something?

And two more general issues:

 . w32term.c is also used in the Cygwin w32 build, (which produces a
   Cygwin Emacs that uses the native MS-Windows GUI functions instead
   of X).  Will this code work in that build?  If not, the new code
   should be ifdef'ed away on Cygwin.  Ken, can you please chime in
   and help us DTRT here?

 . how does one test this feature?  I rarely see any flickering on
   MS-Windows, so what should I try to see the effect of double
   buffering in action?

Thanks again for working on this.