[Spice-devel] [PATCH x11spice] Use separate buffer for primary surface to fix graphical corruption

[Jeremy White]

Hi Brendan,

Nicely done; this solves the problem I had with repeated test runs.

I have one trivial change to request:

On 8/5/19 2:12 PM, Brendan Shanks wrote:
> The 'display->fullscreen' SHM segment was previously being used for both
> x11spice's internal change scanning and as the spice primary surface.
> I don't think spice wants anything else writing to the primary surface,
> and this caused sporadic test failures and graphical corruption.
> 
> Create a separate SHM segment 'display->primary' and use it only for the
> primary surface.
> 
> Signed-off-by: Brendan Shanks <bshanks at codeweavers.com>
> ---
>   src/display.c | 20 +++++++++++++++++++-
>   src/display.h |  1 +
>   src/main.c    |  2 +-
>   src/session.c |  2 +-
>   4 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/src/display.c b/src/display.c
> index 01e0e85..77b4d4e 100644
> --- a/src/display.c
> +++ b/src/display.c
> @@ -461,12 +461,25 @@ void destroy_shm_image(display_t *d, shm_image_t *shmi)
>   
>   int display_create_screen_images(display_t *d)
>   {
> +    /* 'primary' and 'fullscreen' don't need to be SHM, normal buffers would work
> +     * fine. Using SHM for all buffers is simpler though, and has no real downsides.
> +     */
> +    d->primary = create_shm_image(d, 0, 0);
> +    if (!d->primary) {
> +        return X11SPICE_ERR_NOSHM;
> +    }
> +
>       d->fullscreen = create_shm_image(d, 0, 0);
> -    if (!d->fullscreen)
> +    if (!d->fullscreen) {
> +        destroy_shm_image(d, d->primary);
> +        d->primary = NULL;
>           return X11SPICE_ERR_NOSHM;
> +    }
>   
>       d->scanline = create_shm_image(d, 0, 1);
>       if (!d->scanline) {
> +        destroy_shm_image(d, d->primary);
> +        d->primary = NULL;
>           destroy_shm_image(d, d->fullscreen);
>           d->fullscreen = NULL;
>           return X11SPICE_ERR_NOSHM;
> @@ -477,6 +490,11 @@ int display_create_screen_images(display_t *d)
>   
>   void display_destroy_screen_images(display_t *d)
>   {
> +    if (d->primary) {
> +        destroy_shm_image(d, d->primary);
> +        d->primary = NULL;
> +    }
> +
>       if (d->fullscreen) {
>           destroy_shm_image(d, d->fullscreen);
>           d->fullscreen = NULL;
> diff --git a/src/display.h b/src/display.h
> index dc4254b..27bc829 100644
> --- a/src/display.h
> +++ b/src/display.h
> @@ -55,6 +55,7 @@ typedef struct {
>   
>       const xcb_query_extension_reply_t *xfixes_ext;
>   
> +    shm_image_t *primary;
>       shm_image_t *fullscreen;
>       shm_image_t *scanline;
>   
> diff --git a/src/main.c b/src/main.c
> index cecadc8..71cbb46 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -108,7 +108,7 @@ int main(int argc, char *argv[])
>       /*------------------------------------------------------------------------
>       **  Start up a spice server
>       **----------------------------------------------------------------------*/
> -    rc = spice_start(&session.spice, &session.options, session.display.fullscreen);
> +    rc = spice_start(&session.spice, &session.options, session.display.primary);

Could you modify the local variables inside the spice_start function to 
use 'primary' instead of 'fullscreen' so we are not at a risk of 
conflating them in the future?

Cheers,

Jeremy

>       if (rc)
>           goto exit;
>       spice_started = 1;
> diff --git a/src/session.c b/src/session.c
> index 1e59415..4d72e14 100644
> --- a/src/session.c
> +++ b/src/session.c
> @@ -352,7 +352,7 @@ int session_recreate_primary(session_t *s)
>   
>       rc = display_create_screen_images(&s->display);
>       if (rc == 0) {
> -        shm_image_t *f = s->display.fullscreen;
> +        shm_image_t *f = s->display.primary;
>           rc = spice_create_primary(&s->spice, f->w, f->h, f->bytes_per_line, f->shmaddr);
>       }
>   
>