[Spice-devel] [PATCH v2 spice-gtk] Update the codeflow description comment

[Jonathon Jongsma]

On Mon, 2019-02-11 at 16:14 +0200, Snir Sheriber wrote:
> ---
>  src/channel-display-gst.c | 39 ++++++++++++++++++++++---------------
> --
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 4272ade..c63592b 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -580,34 +580,39 @@ static void
> spice_gst_decoder_destroy(VideoDecoder *video_decoder)
>  /* spice_gst_decoder_queue_frame() queues the SpiceFrame for
> decoding and
>   * displaying. The steps it goes through are as follows:
>   *
> - * 1) A SpiceGstFrame is created to keep track of SpiceFrame and
> some additional
> - *    metadata. The SpiceGstFrame is then pushed to the
> decoding_queue.
> - * 2) frame->data, which contains the compressed frame data, is
> reffed and
> - *    wrapped in a GstBuffer which is pushed to the GStreamer
> pipeline for
> - *    decoding.
> - * 3) As soon as the GStreamer pipeline no longer needs the
> compressed frame it
> - *    will call frame->unref_data() to free it.
> + * 1) frame->data, which contains the compressed frame data, is
> wrapped in a GstBuffer
> + *    (encoded_buffer) which owns the SpiceFrame.
> + * 2) A SpiceGstFrame is created to keep track of SpiceFrame
> (encoded_frame), and some
> + *    additional metadata. The encoded_buffer is referenced and the
> SpiceGstFrame is then
> + *    pushed into the decoding_queue.
>   *
>   * If GstVideoOverlay is used (window handle was obtained
> successfully at the widget):
> - *   4) Decompressed frames will be renderd to widget directly from
> gstreamer's pipeline
> - *      using some gstreamer sink plugin which implements the
> GstVideoOverlay interface
> + *   3) Decompressed frames will be rendered to widget directly from
> GStreamer's pipeline

"to *the* widget" sounds more correct. Otherwise I'd say the rest of
this patch looks fine. I only reviewed it for language, though. I
didn't check that the description actually matches the code.

Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>



> + *      using some GStreamer sink plugin which implements the
> GstVideoOverlay interface
>   *      (last step).
> + *   4) As soon as GStreamer's pipeline no longer needs the
> compressed frame it will
> + *      unref the encoded_buffer
> + *   5) Once a decoded buffer arrives to the sink (notified by probe
> event) we will pop
> + *      its matching SpiceGstFrame from the decoding_queue and free
> it using
> + *      free_gst_frame, this will also unref the encoded_buffer
> which will allow
> + *      GStreamer to call spice_frame_free and free its
> encoded_frame.
>   *
>   * Otherwise appsink is used:
> - *   4) Once the decompressed frame is available the GStreamer
> pipeline calls
> + *   3) Once the decompressed frame is available the GStreamer
> pipeline calls
>   *      new_sample() in the GStreamer thread.
> - *   5) new_sample() then matches the decompressed frame to a
> SpiceGstFrame from
> + *   4) new_sample() then matches the decompressed frame to a
> SpiceGstFrame from
>   *      the decoding queue using the GStreamer timestamp information
> to deal with
>   *      dropped frames. The SpiceGstFrame is popped from the
> decoding_queue.
> - *   6) new_sample() then attaches the decompressed frame to the
> SpiceGstFrame,
> + *   5) new_sample() then attaches the decompressed frame to the
> SpiceGstFrame,
>   *      set into display_frame and calls schedule_frame().
> - *   7) schedule_frame() then uses gstframe->frame->mm_time to
> arrange for
> + *   6) schedule_frame() then uses gstframe->encoded_frame->mm_time
> to arrange for
>   *      display_frame() to be called, in the main thread, at the
> right time for
>   *      the next frame.
> - *   8) display_frame() use SpiceGstFrame from display_frame and
> - *      calls stream_display_frame().
> - *   9) display_frame() then frees the SpiceGstFrame, which frees
> the SpiceFrame
> - *      and decompressed frame with it.
> + *   7) display_frame() uses SpiceGstFrame from display_frame and
> calls
> + *      stream_display_frame().
> + *   8) display_frame() then calls to free_gst_frame to free the
> SpiceGstFrame and
> + *      unref the encoded_buffer which allows GStreamer to call
> spice_frame_free
> + *      and free its encoded_frame.
>   */
>  static gboolean spice_gst_decoder_queue_frame(VideoDecoder
> *video_decoder,
>                                                SpiceFrame *frame, int
> latency)