From 11b83fb2fc513f94e42c1ffd4e3b57e845c5909a Mon Sep 17 00:00:00 2001 From: Thibault Saunier Date: Wed, 14 Sep 2022 16:39:48 -0300 Subject: [PATCH] videorate: Handle closing segment on EOS right after caps event The scenario is what we try in the tests: - we have a segment with .stop set - some frame(s) flow - we get a CAPS event - we get an EOS (before getting buffers after the CAPS event) in that case, without that patch, the segment is not properly closed which is not correct. In this patch we keep track of previous caps until a new buffer arrives, this way in that situation we set previous caps again, and close the segment with the previous buffer. Fixes: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/1352 in this specific case Part-of: --- .../gst-plugins-base/gst/videorate/gstvideorate.c | 136 ++++++++++++++++++--- .../gst-plugins-base/gst/videorate/gstvideorate.h | 7 ++ .../gst-plugins-base/tests/validate/meson.build | 1 + ...ment_after_caps_changed_before_eos.validatetest | 43 +++++++ .../flow-expectations/log-videorate-sink-expected | 12 ++ .../flow-expectations/log-videorate-src-expected | 13 ++ 6 files changed, 197 insertions(+), 15 deletions(-) create mode 100644 subprojects/gst-plugins-base/tests/validate/videorate/fill_segment_after_caps_changed_before_eos.validatetest create mode 100644 subprojects/gst-plugins-base/tests/validate/videorate/fill_segment_after_caps_changed_before_eos/flow-expectations/log-videorate-sink-expected create mode 100644 subprojects/gst-plugins-base/tests/validate/videorate/fill_segment_after_caps_changed_before_eos/flow-expectations/log-videorate-src-expected diff --git a/subprojects/gst-plugins-base/gst/videorate/gstvideorate.c b/subprojects/gst-plugins-base/gst/videorate/gstvideorate.c index c347b94..bf3fbb8 100644 --- a/subprojects/gst-plugins-base/gst/videorate/gstvideorate.c +++ b/subprojects/gst-plugins-base/gst/videorate/gstvideorate.c @@ -609,12 +609,9 @@ gst_video_rate_setcaps (GstBaseTransform * trans, GstCaps * in_caps, videorate->wanted_diff = 0; done: - /* After a setcaps, our caps may have changed. In that case, we can't use - * the old buffer, if there was one (it might have different dimensions) */ - GST_DEBUG_OBJECT (videorate, "swapping old buffers"); - gst_video_rate_swap_prev (videorate, NULL, GST_CLOCK_TIME_NONE); - videorate->last_ts = GST_CLOCK_TIME_NONE; - videorate->average = 0; + if (ret) { + gst_caps_replace (&videorate->in_caps, in_caps); + } return ret; @@ -627,7 +624,7 @@ no_framerate: } static void -gst_video_rate_reset (GstVideoRate * videorate) +gst_video_rate_reset (GstVideoRate * videorate, gboolean on_flush) { GST_DEBUG_OBJECT (videorate, "resetting internal variables"); @@ -642,6 +639,10 @@ gst_video_rate_reset (GstVideoRate * videorate) videorate->discont = TRUE; videorate->average = 0; videorate->force_variable_rate = FALSE; + if (!on_flush) { + /* Do not clear caps on flush events as those are still valid */ + gst_clear_caps (&videorate->in_caps); + } gst_video_rate_swap_prev (videorate, NULL, 0); gst_segment_init (&videorate->segment, GST_FORMAT_TIME); @@ -650,7 +651,7 @@ gst_video_rate_reset (GstVideoRate * videorate) static void gst_video_rate_init (GstVideoRate * videorate) { - gst_video_rate_reset (videorate); + gst_video_rate_reset (videorate, FALSE); videorate->silent = DEFAULT_SILENT; videorate->new_pref = DEFAULT_NEW_PREF; videorate->drop_only = DEFAULT_DROP_ONLY; @@ -789,9 +790,14 @@ gst_video_rate_swap_prev (GstVideoRate * videorate, GstBuffer * buffer, gint64 time) { GST_LOG_OBJECT (videorate, "swap_prev: storing buffer %p in prev", buffer); - if (videorate->prevbuf) - gst_buffer_unref (videorate->prevbuf); - videorate->prevbuf = buffer != NULL ? gst_buffer_ref (buffer) : NULL; + + gst_buffer_replace (&videorate->prevbuf, buffer); + /* Ensure that ->prev_caps always match ->prevbuf */ + if (!buffer) + gst_caps_replace (&videorate->prev_caps, NULL); + else if (videorate->prev_caps != videorate->in_caps) + gst_caps_replace (&videorate->prev_caps, videorate->in_caps); + videorate->prev_ts = time; } @@ -876,6 +882,8 @@ gst_video_rate_duplicate_to_close_segment (GstVideoRate * videorate) return count; } + GST_DEBUG_OBJECT (videorate, "Pushing buffers to close segment"); + res = GST_FLOW_OK; /* fill up to the end of current segment */ while (res == GST_FLOW_OK @@ -887,10 +895,54 @@ gst_video_rate_duplicate_to_close_segment (GstVideoRate * videorate) count++; } + GST_DEBUG_OBJECT (videorate, "----> Pushed %d buffers to close segment", + count); return count; } +/* WORKAROUND: This works around BaseTransform limitation as instead of rolling + * back caps, we should be able to push caps only when we are sure we are ready + * to do so. Right now, BaseTransform doesn't let us do anything like that + * so we rollback to previous caps when strictly required (though we now it + * might not be so safe). + * + * To be used only when wanting to 'close' a segment, this function will reset + * caps to previous caps, which will match the content of `prevbuf` in that case + * + * Returns: The previous GstCaps if we rolled back to previous buffers, NULL + * otherwise. + * + * NOTE: When some caps are returned, we should reset them back after + * closing the segment is done. + */ +static GstCaps * +gst_video_rate_rollback_to_prev_caps_if_needed (GstVideoRate * videorate) +{ + GstCaps *prev_caps = NULL; + + if (videorate->prev_caps && videorate->prev_caps != videorate->in_caps) { + if (videorate->in_caps) + prev_caps = gst_caps_ref (videorate->in_caps); + + if (!gst_pad_send_event (GST_BASE_TRANSFORM_SINK_PAD (videorate), + gst_event_new_caps (videorate->prev_caps) + )) { + + GST_WARNING_OBJECT (videorate, "Could not send previous caps to close " + " segment, not closing it"); + + gst_video_rate_swap_prev (videorate, NULL, GST_CLOCK_TIME_NONE); + videorate->last_ts = GST_CLOCK_TIME_NONE; + videorate->average = 0; + } + + gst_clear_caps (&videorate->prev_caps); + } + + return prev_caps; +} + static gboolean gst_video_rate_sink_event (GstBaseTransform * trans, GstEvent * event) { @@ -903,12 +955,14 @@ gst_video_rate_sink_event (GstBaseTransform * trans, GstEvent * event) { GstSegment segment; gint seqnum; + GstCaps *rolled_back_caps; gst_event_copy_segment (event, &segment); if (segment.format != GST_FORMAT_TIME) goto format_error; - GST_DEBUG_OBJECT (videorate, "handle NEWSEGMENT"); + rolled_back_caps = + gst_video_rate_rollback_to_prev_caps_if_needed (videorate); /* close up the previous segment, if appropriate */ if (videorate->prevbuf) { @@ -923,6 +977,26 @@ gst_video_rate_sink_event (GstBaseTransform * trans, GstEvent * event) gst_video_rate_swap_prev (videorate, NULL, 0); } + if (rolled_back_caps) { + GST_DEBUG_OBJECT (videorate, + "Resetting rolled back caps %" GST_PTR_FORMAT, rolled_back_caps); + if (!gst_pad_send_event (GST_BASE_TRANSFORM_SINK_PAD (videorate), + gst_event_new_caps (rolled_back_caps) + )) { + + GST_WARNING_OBJECT (videorate, "Could not resend caps after closing " + " segment"); + + GST_ELEMENT_ERROR (videorate, CORE, NEGOTIATION, + ("Could not resend caps after closing segment"), (NULL)); + gst_caps_unref (rolled_back_caps); + + return FALSE; + } + + gst_caps_unref (rolled_back_caps); + } + videorate->base_ts = 0; videorate->out_frame_count = 0; videorate->next_ts = GST_CLOCK_TIME_NONE; @@ -951,10 +1025,14 @@ gst_video_rate_sink_event (GstBaseTransform * trans, GstEvent * event) case GST_EVENT_EOS:{ gint count = 0; GstFlowReturn res = GST_FLOW_OK; + GstCaps *rolled_back_caps; GST_DEBUG_OBJECT (videorate, "Got %s", gst_event_type_get_name (GST_EVENT_TYPE (event))); + rolled_back_caps = + gst_video_rate_rollback_to_prev_caps_if_needed (videorate); + /* If the segment has a stop position, fill the segment */ if (GST_CLOCK_TIME_IS_VALID (videorate->segment.stop)) { /* fill up to the end of current segment */ @@ -993,6 +1071,22 @@ gst_video_rate_sink_event (GstBaseTransform * trans, GstEvent * event) } } + if (rolled_back_caps) { + GST_DEBUG_OBJECT (videorate, + "Resetting rolled back caps %" GST_PTR_FORMAT, rolled_back_caps); + + if (!gst_pad_send_event (GST_BASE_TRANSFORM_SINK_PAD (videorate), + gst_event_new_caps (rolled_back_caps) + )) { + + /* Not erroring out on EOS as it won't be too bad in any case */ + GST_WARNING_OBJECT (videorate, "Could not resend caps after closing " + " segment on EOS (ignoring the error)"); + } + + gst_caps_unref (rolled_back_caps); + } + if (count > 1) { videorate->dup += count - 1; if (!videorate->silent) @@ -1009,7 +1103,7 @@ gst_video_rate_sink_event (GstBaseTransform * trans, GstEvent * event) case GST_EVENT_FLUSH_STOP: /* also resets the segment */ GST_DEBUG_OBJECT (videorate, "Got FLUSH_STOP"); - gst_video_rate_reset (videorate); + gst_video_rate_reset (videorate, TRUE); break; case GST_EVENT_GAP: /* no gaps after videorate, ignore the event */ @@ -1545,6 +1639,18 @@ gst_video_rate_transform_ip (GstBaseTransform * trans, GstBuffer * buffer) videorate = GST_VIDEO_RATE (trans); + if (videorate->prev_caps != videorate->in_caps) { + /* After caps where set we didn't reset the state so we could close + * the segment from previous caps if necessary, we got a buffer after the + * new caps so we can reset now */ + GST_DEBUG_OBJECT (videorate, "Clearing old buffers now that we had a buffer" + " after receiving caps"); + gst_video_rate_swap_prev (videorate, NULL, GST_CLOCK_TIME_NONE); + gst_clear_caps (&videorate->prev_caps); + videorate->last_ts = GST_CLOCK_TIME_NONE; + videorate->average = 0; + } + /* make sure the denominators are not 0 */ if (videorate->from_rate_denominator == 0 || videorate->to_rate_denominator == 0) @@ -1853,14 +1959,14 @@ invalid_buffer: static gboolean gst_video_rate_start (GstBaseTransform * trans) { - gst_video_rate_reset (GST_VIDEO_RATE (trans)); + gst_video_rate_reset (GST_VIDEO_RATE (trans), FALSE); return TRUE; } static gboolean gst_video_rate_stop (GstBaseTransform * trans) { - gst_video_rate_reset (GST_VIDEO_RATE (trans)); + gst_video_rate_reset (GST_VIDEO_RATE (trans), FALSE); return TRUE; } diff --git a/subprojects/gst-plugins-base/gst/videorate/gstvideorate.h b/subprojects/gst-plugins-base/gst/videorate/gstvideorate.h index 02d26e3..11f260f 100644 --- a/subprojects/gst-plugins-base/gst/videorate/gstvideorate.h +++ b/subprojects/gst-plugins-base/gst/videorate/gstvideorate.h @@ -74,6 +74,13 @@ struct _GstVideoRate int max_rate; gdouble rate; gdouble pending_rate; + + GstCaps *in_caps; + /* Only set right after caps were set so that we still have a reference to + * the caps matching the content of `->prevbuf`, this way, if we get an EOS + * right after a CAPS, we can reset to those caps and close the segment with + * it */ + GstCaps *prev_caps; }; GST_ELEMENT_REGISTER_DECLARE (videorate); diff --git a/subprojects/gst-plugins-base/tests/validate/meson.build b/subprojects/gst-plugins-base/tests/validate/meson.build index abcd16e..6e2193c 100644 --- a/subprojects/gst-plugins-base/tests/validate/meson.build +++ b/subprojects/gst-plugins-base/tests/validate/meson.build @@ -20,6 +20,7 @@ tests = [ 'videorate/duplicate_on_eos', 'videorate/duplicate_on_eos_disbaled', 'videorate/duplicate_on_eos_half_sec', + 'videorate/fill_segment_after_caps_changed_before_eos', 'compositor/renogotiate_failing_unsupported_src_format', 'giosrc/read-growing-file', 'encodebin/set-encoder-properties', diff --git a/subprojects/gst-plugins-base/tests/validate/videorate/fill_segment_after_caps_changed_before_eos.validatetest b/subprojects/gst-plugins-base/tests/validate/videorate/fill_segment_after_caps_changed_before_eos.validatetest new file mode 100644 index 0000000..1e26f1d --- /dev/null +++ b/subprojects/gst-plugins-base/tests/validate/videorate/fill_segment_after_caps_changed_before_eos.validatetest @@ -0,0 +1,43 @@ +meta, + args = { + "appsrc name=src format=time handle-segment-change=true ! \ + videorate max-closing-segment-duplication-duration=999999999999999 name=videorate ! video/x-raw,framerate=1/1 ! fakesink sync=true", + }, + configs = { + "$(validateflow), pad=videorate:sink, buffers-checksum=as-id, ignored-event-types={ tag }", + "$(validateflow), pad=videorate:src, buffers-checksum=as-id, ignored-event-types={ tag }", + }, + handles-states=true, + ignore-eos=true + +# Generate the raw video frame that we will used in the appsrc. +run-command, argv={ + "gst-validate-1.0", "videotestsrc num-buffers=1 ! video/x-raw,format=I420,framerate=1/1,width=320,height=240 ! filesink location=$(TMPDIR)/tmp.i420", +} + + +appsrc-push, target-element-name=src, file-name="$(TMPDIR)/tmp.i420", pts=0, duration=1.0, + caps=(GstCaps)[video/x-raw,format=I420,framerate=1/1,width=320,height=240], + segment=[segment, stop=3.0, format=(GstFormat)time] + +appsrc-push, target-element-name=src, file-name="$(TMPDIR)/tmp.i420", pts=1., duration=1.0, + caps=(GstCaps)[video/x-raw,format=I420,framerate=1/1,width=320,height=240], + segment=[segment, stop=3.0, format=(GstFormat)time] +play + +crank-clock, repeat=1 + +checkpoint, text="Setting caps but the videorate element will roll the caps back to push buffers to close the configured segment on EOS" +set-properties, src::caps="video/x-raw,width=322,height=244,framerate=1/1" + +appsrc-eos, target-element-name=src +crank-clock, repeat=3 + + +wait, message-type=eos + +# check-position, expected-position=2.0 +stop + + + diff --git a/subprojects/gst-plugins-base/tests/validate/videorate/fill_segment_after_caps_changed_before_eos/flow-expectations/log-videorate-sink-expected b/subprojects/gst-plugins-base/tests/validate/videorate/fill_segment_after_caps_changed_before_eos/flow-expectations/log-videorate-sink-expected new file mode 100644 index 0000000..d6fbb27 --- /dev/null +++ b/subprojects/gst-plugins-base/tests/validate/videorate/fill_segment_after_caps_changed_before_eos/flow-expectations/log-videorate-sink-expected @@ -0,0 +1,12 @@ +event stream-start: GstEventStreamStart, flags=(GstStreamFlags)GST_STREAM_FLAG_NONE, group-id=(uint)1; +event caps: video/x-raw, format=(string)I420, framerate=(fraction)1/1, height=(int)240, width=(int)320; +event segment: format=TIME, start=0:00:00.000000000, offset=0:00:00.000000000, stop=0:00:03.000000000, time=0:00:00.000000000, base=0:00:00.000000000, position=0:00:00.000000000 +buffer: content-id=0, pts=0:00:00.000000000, dur=0:00:01.000000000, flags=discont +buffer: content-id=0, pts=0:00:01.000000000, dur=0:00:01.000000000 + +CHECKPOINT: Setting caps but the videorate element will roll the caps back to push buffers to close the configured segment on EOS + +event caps: video/x-raw, framerate=(fraction)1/1, height=(int)244, width=(int)322; +event eos: (no structure) +event caps: video/x-raw, format=(string)I420, framerate=(fraction)1/1, height=(int)240, width=(int)320; +event caps: video/x-raw, framerate=(fraction)1/1, height=(int)244, width=(int)322; diff --git a/subprojects/gst-plugins-base/tests/validate/videorate/fill_segment_after_caps_changed_before_eos/flow-expectations/log-videorate-src-expected b/subprojects/gst-plugins-base/tests/validate/videorate/fill_segment_after_caps_changed_before_eos/flow-expectations/log-videorate-src-expected new file mode 100644 index 0000000..10c9c27 --- /dev/null +++ b/subprojects/gst-plugins-base/tests/validate/videorate/fill_segment_after_caps_changed_before_eos/flow-expectations/log-videorate-src-expected @@ -0,0 +1,13 @@ +event stream-start: GstEventStreamStart, flags=(GstStreamFlags)GST_STREAM_FLAG_NONE, group-id=(uint)1; +event caps: video/x-raw, format=(string)I420, framerate=(fraction)1/1, height=(int)240, width=(int)320; +event segment: format=TIME, start=0:00:00.000000000, offset=0:00:00.000000000, stop=0:00:03.000000000, time=0:00:00.000000000, base=0:00:00.000000000, position=0:00:00.000000000 +buffer: content-id=0, pts=0:00:00.000000000, dur=0:00:01.000000000, flags=discont + +CHECKPOINT: Setting caps but the videorate element will roll the caps back to push buffers to close the configured segment on EOS + +event caps: video/x-raw, framerate=(fraction)1/1, height=(int)244, width=(int)322; +event caps: video/x-raw, format=(string)I420, framerate=(fraction)1/1, height=(int)240, width=(int)320; +buffer: content-id=0, pts=0:00:01.000000000, dur=0:00:01.000000000 +buffer: content-id=0, pts=0:00:02.000000000, dur=0:00:01.000000000, flags=gap +event caps: video/x-raw, framerate=(fraction)1/1, height=(int)244, width=(int)322; +event eos: (no structure) -- 2.7.4