From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: MS Windows double buffering Date: Sat, 30 Apr 2022 09:50:36 +0300 Message-ID: <835ymr9i9v.fsf@gnu.org> References: <877d791thh.fsf.ref@yahoo.com> <877d791thh.fsf@yahoo.com> <83ilqtbl3p.fsf@gnu.org> <87sfpxz8a7.fsf@yahoo.com> <87mtg4zhlg.fsf@yahoo.com> <87mtg3unzu.fsf@yahoo.com> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="8633"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel@gnu.org To: Po Lu , Ken Brown Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sat Apr 30 08:52:31 2022 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nkgxz-00026o-7X for ged-emacs-devel@m.gmane-mx.org; Sat, 30 Apr 2022 08:52:31 +0200 Original-Received: from localhost ([::1]:45308 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1nkgxx-0003Ks-Vl for ged-emacs-devel@m.gmane-mx.org; Sat, 30 Apr 2022 02:52:30 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:38656) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nkgw9-0002WW-9A for emacs-devel@gnu.org; Sat, 30 Apr 2022 02:50:37 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:40118) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nkgw7-000888-47; Sat, 30 Apr 2022 02:50:35 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=iek5ikdBWhIpFzvvB/Q6aITvm1NZqG9u4G2CzmKh3n4=; b=SSDxKEnqM7E0 ppjUQ46gsbGn3IkCW9tszys2aYw9rqkxz/293fatsiYsuHHA88el+DS9mIWRgeAXtrk9YJCfEHAiv UrQ3+hh3nlpOB5V7aEdogZLB3g/PMOBZk99G/j7tTAB1sjPTHFTFDpbLtoqPDRLHmKYRrV+iFkIsK vVsL9iC2v1tf8c0/c0UEodf9gUNly4jedS06rCXBzTGGzRJ8seKWUReHdi8IQgKnkOe6HpWPeCxKm b1YfShVO+50fUp4hqDkjrE290LUGLVqgQMwaBOv3TmxSjgeyy7pHijfvNgrok8OYmZuA0oW5EPwKJ Q7l1hNgwnHEFCXwUv7dcjg==; Original-Received: from [87.69.77.57] (port=3860 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nkgw5-0001rb-Tq; Sat, 30 Apr 2022 02:50:34 -0400 In-Reply-To: <87mtg3unzu.fsf@yahoo.com> (message from Po Lu on Sat, 30 Apr 2022 13:41:25 +0800) X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:289031 Archived-At: > From: Po Lu > 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.