From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: RFC: flicker-free double-buffered Emacs under X11 Date: Fri, 21 Oct 2016 11:49:04 +0300 Message-ID: <83k2d2rssf.fsf@gnu.org> References: <9e8ad090-a6a0-c807-95ae-7ec7c3f391cb@dancol.org> Reply-To: Eli Zaretskii NNTP-Posting-Host: blaine.gmane.org X-Trace: blaine.gmane.org 1477039800 17531 195.159.176.226 (21 Oct 2016 08:50:00 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 21 Oct 2016 08:50:00 +0000 (UTC) Cc: emacs-devel@gnu.org To: Daniel Colascione Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Oct 21 10:49:55 2016 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bxVWT-0003iq-VN for ged-emacs-devel@m.gmane.org; Fri, 21 Oct 2016 10:49:54 +0200 Original-Received: from localhost ([::1]:59488 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxVWW-0007AR-7A for ged-emacs-devel@m.gmane.org; Fri, 21 Oct 2016 04:49:56 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:38089) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxVVu-0007AM-NM for emacs-devel@gnu.org; Fri, 21 Oct 2016 04:49:20 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bxVVr-0003MH-Co for emacs-devel@gnu.org; Fri, 21 Oct 2016 04:49:18 -0400 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:33986) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxVVr-0003MD-8o; Fri, 21 Oct 2016 04:49:15 -0400 Original-Received: from 84.94.185.246.cable.012.net.il ([84.94.185.246]:2744 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.82) (envelope-from ) id 1bxVVq-0000fX-A3; Fri, 21 Oct 2016 04:49:14 -0400 In-reply-to: <9e8ad090-a6a0-c807-95ae-7ec7c3f391cb@dancol.org> (message from Daniel Colascione on Thu, 20 Oct 2016 18:32:01 -0700) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2001:4830:134:3::e X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 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.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:208575 Archived-At: > From: Daniel Colascione > 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 > 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.