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@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" ? I was thinking of a follow-up patch renaming both at once, I don't consider 'buffer' naming to be a regression either. Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel