diff --git a/subprojects/gst-plugins-base/gst/videorate/gstvideorate.c b/subprojects/gst-plugins-base/gst/videorate/gstvideorate.c index c347b94438..bf3fbb8b90 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 02d26e39d7..11f260f6b0 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 abcd16edbf..6e2193cbb3 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 0000000000..1e26f1dc38 --- /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 0000000000..d6fbb27331 --- /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 0000000000..10c9c27d7a --- /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)