On 19.02.2016 14:31, Keith Packard wrote:
> Michel Dänzer <[email protected]> writes:
>
> Here's another review, now that I've more carefully looked at the other
> two patches.
>
>> + if (!flip_window || !flip_pixmap)
>> + return;
>
> In the original code, this didn't abort the operation entirely...
>
> This operation would have been skipped:
>
>> + present_set_tree_pixmap(flip_window, flip_pixmap, screen_pixmap);
>
> But this operation would not:
>
>> + present_set_tree_pixmap(screen->root, NULL, screen_pixmap);
BTW, it might be safer to make this
present_set_tree_pixmap(screen->root, flip_pixmap, screen_pixmap);
as well, so the root window pixmap is only changed to the screen pixmap
if it's currently the flip pixmap? Anyway, this can be done in a
followup, since the current call with NULL doesn't seem to cause any
problem in practice.
> Reading the rest of the code, I can't find a case which could call this
> with a null flip_window or flip_pixmap;
While I couldn't either just looking at present.c, I can get it called
with flip_window == NULL when pressing Escape while glxgears is flipping
in fullscreen, presumably due to present_clear_window_flip.
I'm not sure the present_copy_region or
present_set_tree_pixmap(screen->root, ...) calls will ever have any
actual effect in this case, but for now let's preserve the logic of the
refactored code wrt this.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
