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? Rest of the patch looks good to me. Acked-by: Christophe Fergeau <cfergeau@xxxxxxxxxx> Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel