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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel