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

[Christophe Fergeau]

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?

Rest of the patch looks good to me.

Acked-by: Christophe Fergeau <cfergeau at redhat.com>

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/aa3d62e2/attachment.sig>