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

[Jonathon Jongsma]

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