Re: MS Windows double buffering

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

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

I will keep that in mind in the future, sorry.

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

I will write one, thanks.

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

w32_release_paint_buffer releases the back buffer bitmap when a back
buffer exists.  It is only called if double buffering is enabled, or
when double buffering is being turned off, or during frame resize (to
resize the back buffer bitmap.)  But w32_show_back_buffer should indeed
work that way, yes.

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

Thanks.  That's not something we do on X, but I will add such a
variable.

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

[...]

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

Likewise, I will do that.

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

`paint_buffer' can only be accessed safely inside the critical section,
as long as get_frame_dc can be called from the message pump thread, but
maybe it isn't called there.

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

Will do, thanks.

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

Sorry for that, it snuck in somehow.

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

No idea, though I don't think double buffering uses anything that might
not work on Cygwin.

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

Emacs should flicker much less (or not at all) upon any of the test
cases in the last thread(s) on flicker, or when you run this:

  (while t (redraw-display) (redisplay))

Thanks.