> > 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@xxxxxxxxxx> > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx> > > --- > > 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@xxxxxxxxxx> > > Christophe > Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel