[Spice-devel] [PATCH spice-gtk] channel-display: add spice_frame_free() helper

[Christophe Fergeau]

On Tue, Jan 22, 2019 at 10:06:22AM -0500, Frediano Ziglio wrote:
> > 
> > On Mon, Jan 21, 2019 at 04:29:56PM +0000, Frediano Ziglio wrote:
> > > Heavily based on a former patch from Victor Toso removing some issues
> > > (https://lists.freedesktop.org/archives/spice-devel/2018-April/043168.html)
> > > 
> > > The SpiceFrame is created in channel-display.c but it is currently
> > > freed at each decoders' end. A helper function can reduce some code
> > > and makes it easier to check if the function is called, what time was
> > > called, etc.
> > 
> > "what time it was called"
> > 
> > > 
> > > In channel-display-mjpeg.c this means removing free_spice_frame()
> > > function.
> > > 
> > > In channel-display-gst.c, SpiceFrame is under SpiceGstFrame so we just
> > > need to be careful to call spice_frame_free() once by removing the
> > > unref function parameter to gst_buffer_new_wrapped_full();
> > > 
> > > The patch is using g_clear_pointer() everywhere that makes sense
> > > (checking for NULL before calling free and setting pointer to NULL
> > > afterwards)
> > 
> > Doing that in a preliminary commit + adding some static
> > spice_free_frame() functions in channel-display-gst.c would probably
> > have cut the diff size in half, making it easier to focus on the actual
> > changes during the review.
> > 
> > > 
> > > The ownedship management is more clear:
> > 
> > 'ownership'
> > 
> > > - SpiceFrame owns frame data (as it has a pointer to it);
> > > - spice_frame_free releases frame data;
> > > - SpiceFrame interface is simplified;
> > > - GstBuffer owns SpiceFrame (not only frame data);
> > > - SpiceGstFrame owns GstBuffer.
> > 
> > This last bullet point makes SpiceGstFrame a bit harder to understand
> > imo, since now you have frame, sample and buffer members, so you have
> > to understand the role of each of these.
> > 
> > 
> > > 
> > > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  src/channel-display-gst.c   | 25 +++++++++++--------------
> > >  src/channel-display-mjpeg.c | 28 +++++++---------------------
> > >  src/channel-display-priv.h  |  5 +----
> > >  src/channel-display.c       | 15 ++++++++++++---
> > >  4 files changed, 31 insertions(+), 42 deletions(-)
> > > 
> > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > > index 0b871a71..a6ad4f1c 100644
> > > --- a/src/channel-display-gst.c
> > > +++ b/src/channel-display-gst.c
> > > @@ -79,6 +79,7 @@ typedef enum {
> > >  
> > >  struct SpiceGstFrame {
> > >      GstClockTime timestamp;
> > > +    GstBuffer *buffer;
> > >      SpiceFrame *frame;
> > >      GstSample *sample;
> > 
> > Might be worth using 'encoded_buffer' and 'decoded_sample' as names?
> > 
> 
> For sample is not a regression, would be fine if I just rename "buffer" ?

I was thinking of a follow-up patch renaming both at once, I don't
consider 'buffer' naming to be a regression either.

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