Hi,
Many thanks for the reviews.
On Tue, Apr 17, 2018 at 07:08:13AM -0400, Frediano Ziglio wrote:
> >
> > From: Victor Toso <me@xxxxxxxxxxxxxx>
> >
> > 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.
> >
> > 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();
> >
> > Note that I'm using g_clear_pointer() everywhere that makes sense
> > (checking for NULL before calling free and setting pointer to NULL
> > afterwards)
> >
> > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx>
>
> Reading this patch I realize how this structure is weird and that
> due to it you are introducing a bug.
>
> The problem with SpiceFrame is that the "data" (so data, data_size and
> data_opaque fields) are valid until in VideoDecoder::queue_frame OR
> SpiceFrame::ref_data is called. The ownership of SpiceFrame is passed
> to VideoDecoder::queue_frame. Usually in VideoDecoder::queue_frame
> the pointer is retained but as soon of VideoDecoder::queue_frame exit
> if you didn't call SpiceFrame::ref_data all the above mentioned fields
> became invalid (dangling pointers!).
> That's the reason why SpiceFrame::free is just a g_free and not free
> the data message, because SpiceFrame does not own the data (but have
> a pointer to it!).
> I think would be a good way to have SpiceFrame::free to unreference
> data and obviously to own it (in spice_frame_new).
I agree, many thanks for this analyses is indeed quite important.
I'll change it for the next version.
>
> > ---
> > src/channel-display-gst.c | 8 +++-----
> > src/channel-display-mjpeg.c | 21 ++++-----------------
> > src/channel-display-priv.h | 1 +
> > src/channel-display.c | 11 +++++++++++
> > 4 files changed, 19 insertions(+), 22 deletions(-)
> >
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index 8b23036..d2847ec 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -91,10 +91,8 @@ static SpiceGstFrame *create_gst_frame(GstBuffer *buffer,
> > SpiceFrame *frame)
> >
> > static void free_gst_frame(SpiceGstFrame *gstframe)
> > {
> > - gstframe->frame->free(gstframe->frame);
> > - if (gstframe->sample) {
> > - gst_sample_unref(gstframe->sample);
> > - }
> > + g_clear_pointer(&gstframe->frame, spice_frame_free);
>
> Note that previously you didn't unreference the data.
>
> > + g_clear_pointer(&gstframe->sample, gst_sample_unref);
> > g_free(gstframe);
> > }
> >
> > @@ -553,7 +551,7 @@ static gboolean
> > spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> > frame->ref_data(frame->data_opaque);
> > GstBuffer *buffer =
> > gst_buffer_new_wrapped_full(GST_MEMORY_FLAG_PHYSICALLY_CONTIGUOUS,
> > frame->data,
> > frame->size, 0,
> > frame->size,
> > - frame->data_opaque,
> > frame->unref_data);
> > + frame->data_opaque,
> > NULL);
>
> Bug: you are retaining a pointer without ownership, looking for trouble!
>
> >
> > GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
> > GST_BUFFER_DTS(buffer) = GST_CLOCK_TIME_NONE;
> > diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> > index f0d55f6..66a6eb8 100644
> > --- a/src/channel-display-mjpeg.c
> > +++ b/src/channel-display-mjpeg.c
> > @@ -75,15 +75,6 @@ static void mjpeg_src_term(struct jpeg_decompress_struct
> > *cinfo)
> > }
> >
> >
> > -/* ---------- A SpiceFrame helper ---------- */
> > -
> > -static void free_spice_frame(SpiceFrame *frame)
> > -{
> > - frame->unref_data(frame->data_opaque);
> > - frame->free(frame);
> > -}
> > -
>
> This was correct as the code added the ownership calling ref_data, in previous
> file this was done differently.
>
> > -
> > /* ---------- Decoder proper ---------- */
> >
> > static void mjpeg_decoder_schedule(MJpegDecoder *decoder);
> > @@ -177,8 +168,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> > video_decoder)
> > /* Display the frame and dispose of it */
> > stream_display_frame(decoder->base.stream, decoder->cur_frame,
> > width, height, SPICE_UNKNOWN_STRIDE,
> > decoder->out_frame);
> > - free_spice_frame(decoder->cur_frame);
> > - decoder->cur_frame = NULL;
> > + g_clear_pointer(&decoder->cur_frame, spice_frame_free);
> > decoder->timer_id = 0;
> >
> > /* Schedule the next frame */
> > @@ -212,7 +202,7 @@ static void mjpeg_decoder_schedule(MJpegDecoder *decoder)
> > __FUNCTION__, time - frame->mm_time,
> > frame->mm_time, time);
> > stream_dropped_frame_on_playback(decoder->base.stream);
> > - free_spice_frame(frame);
> > + spice_frame_free(frame);
> > }
> > frame = g_queue_pop_head(decoder->msgq);
> > } while (frame);
> > @@ -222,7 +212,7 @@ static void mjpeg_decoder_schedule(MJpegDecoder *decoder)
> > /* mjpeg_decoder_drop_queue() helper */
> > static void _msg_in_unref_func(gpointer data, gpointer user_data)
> > {
> > - free_spice_frame((SpiceFrame*)data);
> > + spice_frame_free(data);
> > }
> >
> > static void mjpeg_decoder_drop_queue(MJpegDecoder *decoder)
> > @@ -231,10 +221,7 @@ static void mjpeg_decoder_drop_queue(MJpegDecoder
> > *decoder)
> > g_source_remove(decoder->timer_id);
> > decoder->timer_id = 0;
> > }
> > - if (decoder->cur_frame) {
> > - free_spice_frame(decoder->cur_frame);
> > - decoder->cur_frame = NULL;
> > - }
> > + g_clear_pointer(&decoder->cur_frame, spice_frame_free);
> > g_queue_foreach(decoder->msgq, _msg_in_unref_func, NULL);
> > g_queue_clear(decoder->msgq);
> > }
> > diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> > index 6a90a78..95ad7d8 100644
> > --- a/src/channel-display-priv.h
> > +++ b/src/channel-display-priv.h
> > @@ -196,6 +196,7 @@ void stream_dropped_frame_on_playback(display_stream
> > *st);
> > #define SPICE_UNKNOWN_STRIDE 0
> > void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t
> > width, uint32_t height, int stride, uint8_t* data);
> >
> > +void spice_frame_free(SpiceFrame *frame);
> >
> > G_END_DECLS
> >
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index ec94cf8..a134516 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -1485,6 +1485,17 @@ static void
> > display_session_mm_time_reset_cb(SpiceSession *session, gpointer dat
> >
> > #define STREAM_PLAYBACK_SYNC_DROP_SEQ_LEN_LIMIT 5
> >
> > +G_GNUC_INTERNAL
> > +void spice_frame_free(SpiceFrame *frame)
> > +{
> > + if (frame == NULL) {
> > + return;
> > + }
>
> not much sure about.
Might make sense to be a warning instead of silent return.
>
> > +
> > + frame->unref_data(frame->data_opaque);
> > + frame->free(frame);
> > +}
> > +
> > /* coroutine context */
> > static void display_handle_stream_data(SpiceChannel *channel, SpiceMsgIn
> > *in)
> > {
>
> Oh... note that with gstreamer code the spice_msg_in_unref is potentially
> called from a different thread leading to possible race conditions as
> the operation is not atomic. Maybe would be safer (even if I don't like
> the cost for just not much occurrences) to change in an atomic operation.
Really good catch. Let's discuss it on your proposal patch.
Cheers,
>
> Frediano
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel