[Spice-devel] [PATCH v1 09/10] DCC: change how fill_bits() marshalls data by reference

[Frediano Ziglio]

> 
> On Wed, 2016-12-14 at 16:03 -0500, Frediano Ziglio wrote:
> > > 
> > > 
> > > The fill_bits() function marshalls some data by reference. This
> > > data is
> > > owned by the RedDrawable that is owned by the Drawable that is
> > > owned by
> > > the RedDrawablePipeItem.  Instead of keeping the RedPipeItem alive
> > > by
> > > passing it to red_channel_client_init_send_data(), simply reference
> > > the
> > > Drawable and marshall it with _add_by_ref_full(). This means that
> > > we
> > > can't use  the _add_chunks_by_ref() convenience function since that
> > > function doesn't allow us to pass a free function to clean up the
> > > data
> > > after it is sent.
> > > 
> > > This change is not perfect since the fill_bits() function makes an
> > > assumption that 'simage' is owned by the 'drawable'. On the other
> > > hand,
> > > the previous code made a much bigger assumption: that the caller
> > > would
> > > ensure that the data would be kept alive
> > > ---
> > >  server/dcc-send.c | 55
> > >  ++++++++++++++++++++++++++++++++++++++-----------------
> > >  1 file changed, 38 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/server/dcc-send.c b/server/dcc-send.c
> > > index db42ab8..09a2bb4 100644
> > > --- a/server/dcc-send.c
> > > +++ b/server/dcc-send.c
> > > @@ -343,8 +343,17 @@ static void
> > > marshaller_add_compressed(SpiceMarshaller
> > > *m,
> > >      } while (max);
> > >  }
> > >  
> > > +static void marshaller_unref_drawable(uint8_t *data, void *opaque)
> > > +{
> > > +    Drawable *drawable = opaque;
> > > +    drawable_unref(drawable);
> > > +}
> > > +
> > 
> > This function is copied multiple times. Should be defined and reused.
> 
> I don't think this function is really copied multiple times. In other
> files, it's generally a RedPipeItem that is being referenced and
> unreferenced. In this file, it's a Drawable.
> 
> I thought about making the helper function for unreffing a RedPipeItem
> accessible from different files, but I couldn't decide on a good place
> to put it. The signature of the function is pretty marshaller-specific,
> so I'm not sure that I'd want to put that function in red-pipe-item.h.
> But maybe...
> 
> > 
> > > 
> > >  /* if the number of times fill_bits can be called per one
> > > qxl_drawable
> > >  increases -
> > >     MAX_LZ_DRAWABLE_INSTANCES must be increased as well */
> > > +/* NOTE: 'simage' should be owned by the drawable. The drawable
> > > will be kept
> > > + * alive until the marshalled message has been sent. See comments
> > > below for
> > > + * more information */
> > >  static FillBitsType fill_bits(DisplayChannelClient *dcc,
> > > SpiceMarshaller *m,
> > >                                SpiceImage *simage, Drawable
> > > *drawable, int
> > >                                can_lossy)
> > >  {
> > > @@ -449,7 +458,14 @@ static FillBitsType
> > > fill_bits(DisplayChannelClient *dcc,
> > > SpiceMarshaller *m,
> > >                  spice_marshall_Palette(bitmap_palette_out,
> > > palette);
> > >              }
> > >  
> > > -            spice_marshaller_add_chunks_by_ref(m, bitmap->data);
> > > +            /* 'drawable' owns this bitmap data, so it must be
> > > kept
> > > +             * alive until the message is sent. */
> > > +            for (unsigned int i = 0; i < bitmap->data->num_chunks;
> > > i++) {
> > > +                drawable->refs++;
> > > +                spice_marshaller_add_by_ref_full(m,
> > > bitmap->data->chunk[i].data,
> > > +                                                 bitmap->data-
> > > >chunk[i].len,
> > > +                                                 marshaller_unref_
> > > drawable,
> > > drawable);
> > > +            }
> > 
> > could this became a function? Or perhaps adding a
> > spice_marshaller_add_chunks_by_ref_full?
> > I think just adding a reference for the last chunk is enough (but
> > this is an optimization).
> 
> Yeah, I was initially going to add a _add_chunks_by_ref_full()
> function, but didn't because I was thinking I'd need to add a reference
> for each chunk and that wasn't really feasible within the
> _add_chunks_by_ref_full() function (since that function doesn't know
> what object to ref). I suppose that it would be possible if you only
> add a reference for the last chunk, but that feels a bit hacky to me.
> 
> You'd end up with something like:
> 
> void spice_marshaller_add_chunks_by_ref_full(SpiceMarshaller *m,
> SpiceChunks *chunks, free_func, opaque) {
>     unsigned int i;
> 
>     for (i = 0; i < chunks->num_chunks; i++) {
>     	if (last_chunk)
> 		spice_marshaller_add_by_ref_full(...)
> 	else
> 		spice_marshaller_add_by_ref(...)
>     }
> }
> 
> 
> 
> > 
> > > 
> > >              pthread_mutex_unlock(&dcc->priv->pixmap_cache->lock);
> > >              return FILL_BITS_TYPE_BITMAP;
> > >          } else {
> > > @@ -481,7 +497,14 @@ static FillBitsType
> > > fill_bits(DisplayChannelClient *dcc,
> > > SpiceMarshaller *m,
> > >                               &bitmap_palette_out,
> > > &lzplt_palette_out);
> > >          spice_assert(bitmap_palette_out == NULL);
> > >          spice_assert(lzplt_palette_out == NULL);
> > > -        spice_marshaller_add_chunks_by_ref(m, image.u.quic.data);
> > > +        /* 'drawable' owns this image data, so it must be kept
> > > +         * alive until the message is sent. */
> > > +        for (unsigned int i = 0; i < image.u.quic.data-
> > > >num_chunks; i++) {
> > > +            drawable->refs++;
> > > +            spice_marshaller_add_by_ref_full(m,
> > > image.u.quic.data->chunk[i].data,
> > > +
> > > image.u.quic.data->chunk[i].len,
> > > +                                             marshaller_unref_draw
> > > able,
> > > drawable);
> > > +        }
> > >          pthread_mutex_unlock(&dcc->priv->pixmap_cache->lock);
> > >          return FILL_BITS_TYPE_COMPRESS_LOSSLESS;
> > >      default:
> > > @@ -531,7 +554,7 @@ static void
> > > marshall_qxl_draw_fill(RedChannelClient *rcc,
> > >      SpiceMarshaller *mask_bitmap_out;
> > >      SpiceFill fill;
> > >  
> > > -    red_channel_client_init_send_data(rcc,
> > > SPICE_MSG_DISPLAY_DRAW_FILL,
> > > &dpi->dpi_pipe_item);
> > > +    red_channel_client_init_send_data(rcc,
> > > SPICE_MSG_DISPLAY_DRAW_FILL,
> > > NULL);
> > >      fill_base(base_marshaller, item);
> > >      fill = drawable->u.fill;
> > >      spice_marshall_Fill(base_marshaller,
> > 
> > Could this cause some problem destroying the primary surface?
> > Deleting a surface causes dpi to be deleted and a check on Drawables
> > (if I remember).
> 
> Hmm, I didn't notice any potential problems here. Can you be a little
> more specific about what you think the problem might be?
> 
> Jonathon
> 

This could be related :(

(process:9392): Spice-ERROR **: display-channel.c:1835:display_channel_destroy_surfaces: assertion `!display->priv->surfaces[i].context.canvas' failed

Didn't narrow down. I was just starting a Windows 7 machine with 4 monitors (I don't know if the monitor information is related).

Frediano