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

[Frediano Ziglio]

> 
> 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" ?

> Rest of the patch looks good to me.
> 
> Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> 
> Christophe
> 

Frediano