From 654ca283a71849ad5c63425083a9a0c2646b2219 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Alicia=20Boya=20Garc=C3=ADa?= Date: Wed, 12 Jul 2023 12:37:34 +0200 Subject: [PATCH] qtdemux: Fix premature EOS when some files are played in push mode Fixes https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/2771 This EOS branch exists so that if a seek with a stop is made, qtdemux stops accepting bytes from the sink after the entire requested playback range is demuxed, as otherwise we could keep download content that is not being used. This patch fixes two flaws that were present in that EOS check: 1) A comparison was made between track time and movie time without conversion. This made the check trigger early in files with edit lists. This patch fixes this by converting the track PTS to movie PTS (stream time) for the check. 2) To avoid sending a EOS prematurely when the segment stop is within a GOP and B-frames are present, the check for EOS should only be done for keyframes. I gather this was already the intention with the existing code, but because it used `stream->on_keyframe` instead of the local variable `keyframe` the old code was checking if the *previous* frame was a keyframe. It's interesting to note that these two flaws in the old code mask each other in most cases: the track PTS will have reached the movie end PTS, but EOS would only be sent if the previous frame was a keyframe. A simple case where they wouldn't mask each other, reproducing the bug, is a sequence of 3 frame GOPs with structure I-B-P. The following validateflow tests have been added to future-proof the fix: * validate.test.mp4.qtdemux_ibpibp_non_frag_pull.default * validate.test.mp4.qtdemux_ibpibp_non_frag_push.default Part-of: --- subprojects/gst-integration-testsuites/medias | 2 +- .../testsuites/validate.testslist | 2 ++ ...demux_ibpibp_non_frag_pull.default.validatetest | 11 ++++++ .../flow-expectations/log-fakesink0-sink-expected | 7 ++++ ...demux_ibpibp_non_frag_push.default.validatetest | 15 ++++++++ .../flow-expectations/log-fakesink0-sink-expected | 8 +++++ subprojects/gst-plugins-good/gst/isomp4/qtdemux.c | 42 ++++++++++++++++++++-- 7 files changed, 84 insertions(+), 3 deletions(-) create mode 100644 subprojects/gst-integration-testsuites/testsuites/validate/mp4/qtdemux_ibpibp_non_frag_pull.default.validatetest create mode 100644 subprojects/gst-integration-testsuites/testsuites/validate/mp4/qtdemux_ibpibp_non_frag_pull.default/flow-expectations/log-fakesink0-sink-expected create mode 100644 subprojects/gst-integration-testsuites/testsuites/validate/mp4/qtdemux_ibpibp_non_frag_push.default.validatetest create mode 100644 subprojects/gst-integration-testsuites/testsuites/validate/mp4/qtdemux_ibpibp_non_frag_push.default/flow-expectations/log-fakesink0-sink-expected diff --git a/subprojects/gst-integration-testsuites/medias b/subprojects/gst-integration-testsuites/medias index 06f5a5a..b4dd49b 160000 --- a/subprojects/gst-integration-testsuites/medias +++ b/subprojects/gst-integration-testsuites/medias @@ -1 +1 @@ -Subproject commit 06f5a5a9bad01b5cf76184aee4f430c834a9dac3 +Subproject commit b4dd49b992efde30a049f9813d08b539edbcaccd diff --git a/subprojects/gst-integration-testsuites/testsuites/validate.testslist b/subprojects/gst-integration-testsuites/testsuites/validate.testslist index ad17ae3..c5e5438 100644 --- a/subprojects/gst-integration-testsuites/testsuites/validate.testslist +++ b/subprojects/gst-integration-testsuites/testsuites/validate.testslist @@ -940,6 +940,8 @@ validate.test.interlace.interlace_deinterlace validate.test.interlace.interlace_deinterlace_alternate validate.test.matroska.demux_flush_within_cluster.default validate.test.mp4.qtdemux_change_edit_list.default +validate.test.mp4.qtdemux_ibpibp_non_frag_pull.default +validate.test.mp4.qtdemux_ibpibp_non_frag_push.default validate.test.mp4.qtdemux_reverse_playback_full_gop.reverse_playback_full_gop validate.test.mp4.redirect.play_15s validate.test.mse.segment_future_past_mse.segment_future_past_mse diff --git a/subprojects/gst-integration-testsuites/testsuites/validate/mp4/qtdemux_ibpibp_non_frag_pull.default.validatetest b/subprojects/gst-integration-testsuites/testsuites/validate/mp4/qtdemux_ibpibp_non_frag_pull.default.validatetest new file mode 100644 index 0000000..bc81f59 --- /dev/null +++ b/subprojects/gst-integration-testsuites/testsuites/validate/mp4/qtdemux_ibpibp_non_frag_pull.default.validatetest @@ -0,0 +1,11 @@ +set-globals, media_dir="$(test_dir)/../../../medias/" +meta, + seek=false, + handles-states=false, + args = { + "filesrc location=\"$(media_dir)/edit-lists/simple/ibpibp-non-frag.mp4\" ! qtdemux ! fakesink async=false", + }, + configs = { + "$(validateflow), pad=fakesink0:sink, record-buffers=true, logged-event-types={segment}", + "change-issue-severity, issue-id=event::eos-has-wrong-seqnum, new-severity=ignore", + } diff --git a/subprojects/gst-integration-testsuites/testsuites/validate/mp4/qtdemux_ibpibp_non_frag_pull.default/flow-expectations/log-fakesink0-sink-expected b/subprojects/gst-integration-testsuites/testsuites/validate/mp4/qtdemux_ibpibp_non_frag_pull.default/flow-expectations/log-fakesink0-sink-expected new file mode 100644 index 0000000..629b74e --- /dev/null +++ b/subprojects/gst-integration-testsuites/testsuites/validate/mp4/qtdemux_ibpibp_non_frag_pull.default/flow-expectations/log-fakesink0-sink-expected @@ -0,0 +1,7 @@ +event segment: format=TIME, start=0:00:00.333333333, offset=0:00:00.000000000, stop=0:00:02.333333333, time=0:00:00.000000000, base=0:00:00.000000000, position=0:00:00.333333333 +buffer: dts=0:00:00.000000000, pts=0:00:00.333333333, dur=0:00:00.333333333, flags=discont +buffer: dts=0:00:00.333333333, pts=0:00:01.000000000, dur=0:00:00.333333333, flags=delta-unit +buffer: dts=0:00:00.666666666, pts=0:00:00.666666666, dur=0:00:00.333333334, flags=delta-unit +buffer: dts=0:00:01.000000000, pts=0:00:01.333333333, dur=0:00:00.333333333 +buffer: dts=0:00:01.333333333, pts=0:00:02.000000000, dur=0:00:00.333333333, flags=delta-unit +buffer: dts=0:00:01.666666666, pts=0:00:01.666666666, dur=0:00:00.333333334, flags=delta-unit diff --git a/subprojects/gst-integration-testsuites/testsuites/validate/mp4/qtdemux_ibpibp_non_frag_push.default.validatetest b/subprojects/gst-integration-testsuites/testsuites/validate/mp4/qtdemux_ibpibp_non_frag_push.default.validatetest new file mode 100644 index 0000000..69e77ff --- /dev/null +++ b/subprojects/gst-integration-testsuites/testsuites/validate/mp4/qtdemux_ibpibp_non_frag_push.default.validatetest @@ -0,0 +1,15 @@ +set-globals, media_dir="$(test_dir)/../../../medias/" +meta, + seek=false, + handles-states=false, + args = { + "appsrc ! qtdemux ! fakesink async=false", + }, + configs = { + "$(validateflow), pad=fakesink0:sink, record-buffers=true, logged-event-types={segment, eos}", + "change-issue-severity, issue-id=event::eos-has-wrong-seqnum, new-severity=ignore", + } + +# Note that sending the file this way ensures qtdemux doesn't receive an EOS +# from appsrc, so we can test that the EOS comes from qtdemux. +appsrc-push, target-element-name=appsrc0, file-name="$(media_dir)/edit-lists/simple/ibpibp-non-frag.mp4" diff --git a/subprojects/gst-integration-testsuites/testsuites/validate/mp4/qtdemux_ibpibp_non_frag_push.default/flow-expectations/log-fakesink0-sink-expected b/subprojects/gst-integration-testsuites/testsuites/validate/mp4/qtdemux_ibpibp_non_frag_push.default/flow-expectations/log-fakesink0-sink-expected new file mode 100644 index 0000000..5aef9e7 --- /dev/null +++ b/subprojects/gst-integration-testsuites/testsuites/validate/mp4/qtdemux_ibpibp_non_frag_push.default/flow-expectations/log-fakesink0-sink-expected @@ -0,0 +1,8 @@ +event segment: format=TIME, start=0:00:00.333333333, offset=0:00:00.000000000, stop=0:00:02.333333333, time=0:00:00.000000000, base=0:00:00.000000000, position=0:00:00.333333333 +buffer: dts=0:00:00.000000000, pts=0:00:00.333333333, dur=0:00:00.333333333, flags=discont tag-memory +buffer: dts=0:00:00.333333333, pts=0:00:01.000000000, dur=0:00:00.333333333, flags=delta-unit tag-memory +buffer: dts=0:00:00.666666666, pts=0:00:00.666666666, dur=0:00:00.333333334, flags=delta-unit tag-memory +buffer: dts=0:00:01.000000000, pts=0:00:01.333333333, dur=0:00:00.333333333, flags=tag-memory +buffer: dts=0:00:01.333333333, pts=0:00:02.000000000, dur=0:00:00.333333333, flags=delta-unit tag-memory +buffer: dts=0:00:01.666666666, pts=0:00:01.666666666, dur=0:00:00.333333334, flags=delta-unit tag-memory +event eos: (no structure) diff --git a/subprojects/gst-plugins-good/gst/isomp4/qtdemux.c b/subprojects/gst-plugins-good/gst/isomp4/qtdemux.c index 4c5e116..efd48b1 100644 --- a/subprojects/gst-plugins-good/gst/isomp4/qtdemux.c +++ b/subprojects/gst-plugins-good/gst/isomp4/qtdemux.c @@ -7321,6 +7321,42 @@ gst_qtdemux_chain (GstPad * sinkpad, GstObject * parent, GstBuffer * inbuf) return gst_qtdemux_process_adapter (demux, FALSE); } +static guint64 +gst_segment_to_stream_time_clamped (const GstSegment * segment, + guint64 position) +{ + guint64 segment_stream_time_start; + guint64 segment_stream_time_stop = GST_CLOCK_TIME_NONE; + guint64 stream_pts_unsigned; + int ret; + + g_return_val_if_fail (segment != NULL, GST_CLOCK_TIME_NONE); + g_return_val_if_fail (segment->format == GST_FORMAT_TIME, + GST_CLOCK_TIME_NONE); + + segment_stream_time_start = segment->time; + if (segment->stop != GST_CLOCK_TIME_NONE) + segment_stream_time_stop = + gst_segment_to_stream_time (segment, GST_FORMAT_TIME, segment->stop); + + ret = + gst_segment_to_stream_time_full (segment, GST_FORMAT_TIME, position, + &stream_pts_unsigned); + /* ret == 0 if the segment is invalid (either position, segment->time or the segment start are -1). */ + g_return_val_if_fail (ret != 0, GST_CLOCK_TIME_NONE); + + if (ret == -1 || stream_pts_unsigned < segment_stream_time_start) { + /* Negative or prior to segment start stream time, clamp to segment start. */ + return segment_stream_time_start; + } else if (segment_stream_time_stop != GST_CLOCK_TIME_NONE + && stream_pts_unsigned > segment_stream_time_stop) { + /* Clamp to segment end. */ + return segment_stream_time_stop; + } else { + return stream_pts_unsigned; + } +} + static GstFlowReturn gst_qtdemux_process_adapter (GstQTDemux * demux, gboolean force) { @@ -7729,7 +7765,7 @@ gst_qtdemux_process_adapter (GstQTDemux * demux, gboolean force) case QTDEMUX_STATE_MOVIE:{ QtDemuxStream *stream = NULL; QtDemuxSample *sample; - GstClockTime dts, pts, duration; + GstClockTime dts, pts, stream_pts, duration; gboolean keyframe; gint i; @@ -7841,12 +7877,14 @@ gst_qtdemux_process_adapter (GstQTDemux * demux, gboolean force) dts = QTSAMPLE_DTS (stream, sample); pts = QTSAMPLE_PTS (stream, sample); + stream_pts = + gst_segment_to_stream_time_clamped (&stream->segment, pts); duration = QTSAMPLE_DUR_DTS (stream, sample, dts); keyframe = QTSAMPLE_KEYFRAME (stream, sample); /* check for segment end */ if (G_UNLIKELY (demux->segment.stop != -1 - && demux->segment.stop <= pts && stream->on_keyframe) + && demux->segment.stop <= stream_pts && keyframe) && !(demux->upstream_format_is_time && demux->segment.rate < 0)) { GST_DEBUG_OBJECT (demux, "we reached the end of our segment."); stream->time_position = GST_CLOCK_TIME_NONE; /* this means EOS */ -- 2.7.4