[Spice-devel] [PATCH v5 07/20] server: Let the video encoder manage the compressed buffer and avoid copying it.

[Christophe Fergeau]

Hey,

On Wed, Sep 30, 2015 at 04:26:13PM +0200, Francois Gouget wrote:
> On Tue, 22 Sep 2015, Christophe Fergeau wrote:
> [...]
> > What I'm getting at is that VideoBuffer seems to only be used from red_marshall_stream_data(),
> > and that all that the non-VideoEncoder code needs is
> > - some data buffer
> > - the size of that data buffer
> > - some opaque pointer which manages this data
> > - a function to free the opaque pointer when it's no longer needed.
> > 
> > It seems to me that VideoBuffer could be stack-allocated (or even
> > removed), and that the gstreamer backend could just return a directly
> > GstBuffer with gst_buffer_unref() as its free function, rather than
> > what is done currently where you create a GstVideoBuffer wrapping a
> > GstBuffer, and then you have some more code to try and reuse existing
> > GstVideoBuffer instances rather than allocating a new one.
> > 
> > The GStreamer 1.0 situation is a bit more complicated as you need a
> > GstBuffer and a GstMapInfo in the free function, I think I would use
> > gst_mini_object_set_qdata() to attach the GstMapInfo data to the
> > GstBuffer.
> 
> I don't think getting rid of the VideoBuffer structure is a good idea. 
> Instead of encode_frame() returning one easily extended structure it 
> will have to return four values: a pointer to the data, the data size, 
> an opaque pointer and a callback. And the video encoder may still have 
> to allocate a structure to store all the information it needs to 
> properly release the frame buffer, as is the case for GStreamer 1.0.

Fine with me, removing was just an alternative suggestion.

> 
> > The needed data, data size, opaque pointer, free func could
> > alternatively be returned by a new encoder vfunc,
> 
> This would only work if we assume that encode_frame() cannot called 
> until the previous compressed frame has been released, or force adding a 
> way to track which compressed buffer we want the size of, which is 
> precisely what VideoBuffer* does for us.

ok.

> 
> 
> > or this could also be all hidden in an video_encoder_marshall(...) 
> > vfunc
> 
> I'd rather have the video encoder only care about video encoding and 
> leave the decision of what to do with the compressed buffer to the 
> caller, i.e. leave the marshalling to the rest of the Spice code.

Makes sense.

> 
> 
> > pull_compressed_buffer() could do (something like):
> > 
> >     buffer->opaque = gst_app_sink_pull_buffer(encoder->appsink);
> >     buffer->free_func = gst_buffer_unref;
> >     if (buffer->opaque) {
> >         buffer->data = GST_BUFFER_DATA(video_buffer->opaque);
> >         buffer->size = GST_BUFFER_SIZE(video_buffer->opaque);
> >         buffer->free_func = gst_buffer_unref;
> >         return VIDEO_ENCODER_FRAME_ENCODE_DONE;
> >     }
> > 
> > and red_marshall_stream_data() would do:
> > VideoBuffer buffer;
> > agent->video_encoder->encode_frame(..., &buffer);
> > spice_marshaller_add_ref_full(base_marshaller, buffer->data, buffer->size,
> >                               buffer->free_func, buffer->opaque);
> 
> I'm not sure I follow what you're getting at here.
> 
> gst_buffer_unref() and spice_marshaller_item_free_func() don't take the 
> same number of parameters. The above spice_marshaller_add_ref_full() 
> call would result in calling gst_buffer_unref(buffer->data, buffer->opaque).
> That's why red_release_video_encoder_buffer() is needed.

My point was mainly that the free function can be gst_buffer_unref (or
some adapter for it) rather than having an intermediate function freeing
the VideoBuffer.

> [...]
> > > +    /* The default video buffer */
> > > +    GstVideoBuffer *default_buffer;
> > 
> > Do we really need a default video buffer rather than always allocating a
> > new one? How much performance do we gain from that? Imo splitting this
> > in a separate commit (or removing it) would make the whole patch much
> > simpler (no need for refcounting in particular).
> 
> You're probably right that avoiding reallocations is probably overkill 
> (for both the mjpeg and gstreamer encoders). After all we do allocate 
> GstBuffer and GstMemory structures with every run (although my 
> understanding is that GStreamer tries to cache those and that Spice does 
> the same with its display_stream structures). So I'll separate that 
> part.

Thanks,

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20151001/ecd17e03/attachment.sig>