basesink: Support position queries after non-resetting flushes

A flush is resetting or not depending on the reset_time argument in the
FLUSH_STOP event is set.

Resetting flushes reset the running time to zero and clear any existing
segment. These are the kind of flushes used by flushing seeks, and by far the
most common. Non-resetting flushes are much more niche, used for instance for
quality changes in adaptivedemux2 and MediaSource Extensions in WebKit.

A key difference between the seek use case and the quality change use case is
that the latter is much more removed from the player. Seeks generally occur
because an user request it, whereas quality changes can be automatic.

Currently, there are three notable cases where position queries fail:

(a) before pre-roll, as there is no segment yet. This is one is understandable,
as for at least some time before pre-roll, we cannot know if a media stream
would start at 0 or any other position, or the duration of the stream for that
matter.

(b) after a resetting flush caused by a seek. This kind of flush resets the
segment, so it's not surprising position queries fail. This is inconvenient for
applications, as it means they always need to handle position reporting (e.g.
in UI) separately every time they request a seek, e.g. by caching the seek
target and using it when the position query fail. I'm not fond of this
behavior, as it's unintuitive and makes GStreamer harder to use, but at this
point could be difficult to change and it's not within the scope of this
proposal.

(c) after a non-resetting flush, e.g. caused by a quality change. The segment
is not reset in this case. Position queries work until a FLUSH_STOP is sent.
Querying position after a FLUSH_START but before a FLUSH_STOP works, and
returns the position the sink was at the moment the FLUSH_START was received.
**This in fact the only reliable way (short of adding probes to the sink
element) to get this position**, as FLUSH_START receival is asynchronous with
playback.

In the case (c), as of currently, position queries fail once the FLUSH_STOP is
received. But unlike in (b), the application has no position to fall back to,
as the FLUSH_START was initiated by elements inside the pipeline that are in a
lower layer of abstraction. Specific applications that have control of both the
player and the internal element doing the flushing -- such as WebKit -- can
still work around this problem through layer violations (lucky!), but this
still puts in question this behavior in GStreamer.

This patch fixes this case by amending the position query handler of basesink,
which was previously erroneously returning early with "wrong state", even
though the flush occurs in PAUSED or PLAYING.

A unit test checking this behavior has also been added.

Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/3471>

subprojects/gstreamer/libs/gst/base/gstbasesink.c

@@ -5141,8 +5141,17 @@ gst_base_sink_get_position (GstBaseSink * basesink, GstFormat format,
 
   GST_OBJECT_LOCK (basesink);
   /* we can only get the segment when we are not NULL or READY */
-  if (!basesink->have_newsegment)
+  if (GST_STATE (basesink) <= GST_STATE_READY &&
+      GST_STATE_PENDING (basesink) <= GST_STATE_READY) {
     goto wrong_state;
+  }
+
+  segment = &basesink->segment;
+  /* get the format in the segment */
+  oformat = segment->format;
+
+  if (oformat == GST_FORMAT_UNDEFINED)
+    goto no_segment;
 
   in_paused = FALSE;
   /* when not in PLAYING or when we're busy with a state change, we
@@ -5153,11 +5162,6 @@ gst_base_sink_get_position (GstBaseSink * basesink, GstFormat format,
     in_paused = TRUE;
   }
 
-  segment = &basesink->segment;
-
-  /* get the format in the segment */
-  oformat = segment->format;
-
   /* report with last seen position when EOS */
   last_seen = basesink->eos;
 
@@ -5356,6 +5360,15 @@ wrong_state:
     GST_OBJECT_UNLOCK (basesink);
     goto done;
   }
+no_segment:
+  {
+    GST_DEBUG_OBJECT (basesink,
+        "haven't received a segment yet, can't anwser position, return -1");
+    res = FALSE;
+    *cur = -1;
+    GST_OBJECT_UNLOCK (basesink);
+    goto done;
+  }
 convert_failed:
   {
     GST_DEBUG_OBJECT (basesink, "convert failed, try upstream");

subprojects/gstreamer/tests/check/elements/fakesink.c

@@ -890,6 +890,175 @@ GST_START_TEST (test_position)
 
 GST_END_TEST;
 
+/* test position reporting still works after a FLUSH_STOP(reset_time=FALSE) */
+GST_START_TEST (test_position_non_resetting_flush)
+{
+  GstElement *pipeline, *sink;
+  GstPad *sinkpad;
+  GstStateChangeReturn ret;
+  gboolean qret;
+  gint64 qcur;
+  GstBuffer *buffer;
+  GstFlowReturn fret;
+  ChainData *data;
+  GstEvent *event;
+  gboolean eret;
+
+  /* create sink */
+  pipeline = gst_pipeline_new ("pipeline");
+  fail_if (pipeline == NULL);
+
+  sink = gst_element_factory_make ("fakesink", "sink");
+  fail_if (sink == NULL);
+  g_object_set (G_OBJECT (sink), "sync", TRUE, NULL);
+  g_object_set (G_OBJECT (sink), "num-buffers", 2, NULL);
+
+  gst_bin_add (GST_BIN (pipeline), sink);
+
+  sinkpad = gst_element_get_static_pad (sink, "sink");
+  fail_if (sinkpad == NULL);
+
+  /* make pipeline and element ready to accept data */
+  ret = gst_element_set_state (pipeline, GST_STATE_PAUSED);
+  fail_unless (ret == GST_STATE_CHANGE_ASYNC);
+
+  gst_pad_send_event (sinkpad, gst_event_new_stream_start ("test"));
+
+  /* send segment, this should work */
+  {
+    GstSegment segment;
+
+    GST_DEBUG ("sending segment");
+    gst_segment_init (&segment, GST_FORMAT_TIME);
+    segment.start = 1 * GST_SECOND;
+    segment.stop = 3 * GST_SECOND;
+    segment.time = 1 * GST_SECOND;
+    event = gst_event_new_segment (&segment);
+
+    eret = gst_pad_send_event (sinkpad, event);
+    fail_if (eret == FALSE);
+  }
+
+  /* do position query, this should succeed with the time value from the segment. */
+  qret = gst_element_query_position (sink, GST_FORMAT_TIME, &qcur);
+  fail_unless (qret == TRUE);
+  fail_unless (qcur == 1 * GST_SECOND);
+
+  /* send buffer that we will flush out */
+  buffer = gst_buffer_new ();
+  GST_BUFFER_PTS (buffer) = 2 * GST_SECOND;
+  GST_BUFFER_DURATION (buffer) = 1 * GST_SECOND;
+
+  GST_DEBUG ("sending buffer");
+
+  /* this buffer causes the sink to preroll */
+  data = chain_async (sinkpad, buffer);
+  fail_if (data == NULL);
+
+  /* wait for preroll */
+  ret = gst_element_get_state (pipeline, NULL, NULL, -1);
+
+  /* do position query, this should succeed with the time value from the
+   * segment. */
+  qret = gst_element_query_position (sink, GST_FORMAT_TIME, &qcur);
+  fail_unless (qret == TRUE);
+  fail_unless (qcur == 1 * GST_SECOND);
+
+  /* start flushing */
+  {
+    GST_DEBUG ("sending flush_start");
+    event = gst_event_new_flush_start ();
+
+    eret = gst_pad_send_event (sinkpad, event);
+    fail_if (eret == FALSE);
+  }
+
+  /* preroll buffer is flushed out */
+  fret = chain_async_return (data);
+  fail_unless (fret == GST_FLOW_FLUSHING);
+
+  /* do position query, this should succeed with the time value from the
+   * segment before the flush. */
+  qret = gst_element_query_position (sink, GST_FORMAT_TIME, &qcur);
+  fail_unless (qret == TRUE);
+  fail_unless (qcur == 1 * GST_SECOND);
+
+  /* send a FLUSH_STOP with reset_time=FALSE */
+  {
+    GST_DEBUG ("sending flush_stop");
+    event = gst_event_new_flush_stop (FALSE);
+
+    eret = gst_pad_send_event (sinkpad, event);
+    fail_if (eret == FALSE);
+  }
+
+  /* do position query again, this should again succeed with the time
+   * value from the segment before the flush. */
+  qret = gst_element_query_position (sink, GST_FORMAT_TIME, &qcur);
+  fail_unless (qret == TRUE);
+  fail_unless (qcur == 1 * GST_SECOND);
+
+  /* send a different segment, this should work */
+  {
+    GstSegment segment;
+
+    GST_DEBUG ("sending segment");
+    gst_segment_init (&segment, GST_FORMAT_TIME);
+    segment.start = 2 * GST_SECOND;
+    segment.stop = 4 * GST_SECOND;
+    segment.time = 10 * GST_SECOND;
+    event = gst_event_new_segment (&segment);
+
+    eret = gst_pad_send_event (sinkpad, event);
+    fail_if (eret == FALSE);
+  }
+
+  /* do position query again, the position should reflect the time
+   * value from the new segment. */
+  qret = gst_element_query_position (sink, GST_FORMAT_TIME, &qcur);
+  fail_unless (qret == TRUE);
+  fail_unless (qcur == 10 * GST_SECOND);
+
+  /* send buffer that we will flush out */
+  buffer = gst_buffer_new ();
+  GST_BUFFER_PTS (buffer) = 2 * GST_SECOND;
+  GST_BUFFER_DURATION (buffer) = 1 * GST_SECOND;
+
+  GST_DEBUG ("sending buffer");
+
+  /* this buffer causes the sink to preroll */
+  data = chain_async (sinkpad, buffer);
+  fail_if (data == NULL);
+
+  /* wait for preroll */
+  ret = gst_element_get_state (pipeline, NULL, NULL, -1);
+
+  /* do position query, this should succeed with the time value from the
+   * segment. */
+  qret = gst_element_query_position (sink, GST_FORMAT_TIME, &qcur);
+  fail_unless (qret == TRUE);
+  fail_unless (qcur == 10 * GST_SECOND);
+
+  ret = gst_element_set_state (pipeline, GST_STATE_PLAYING);
+  fail_unless (ret == GST_STATE_CHANGE_SUCCESS);
+
+  /* preroll buffer is rendered */
+  fret = chain_async_return (data);
+  fail_unless (fret == GST_FLOW_OK);
+
+  ret = gst_element_set_state (pipeline, GST_STATE_NULL);
+  fail_unless (ret == GST_STATE_CHANGE_SUCCESS);
+
+  /* fails because we are in the wrong state */
+  qret = gst_element_query_position (sink, GST_FORMAT_TIME, &qcur);
+  fail_unless (qret == FALSE);
+
+  gst_object_unref (sinkpad);
+  gst_object_unref (pipeline);
+}
+
+GST_END_TEST;
+
 /* like fakesrc, but also pushes an OOB event after each buffer */
 typedef GstPushSrc OOBSource;
 typedef GstPushSrcClass OOBSourceClass;
@@ -1190,6 +1359,7 @@ fakesink_suite (void)
   tcase_add_test (tc_chain, test_eos);
   tcase_add_test (tc_chain, test_eos2);
   tcase_add_test (tc_chain, test_position);
+  tcase_add_test (tc_chain, test_position_non_resetting_flush);
   tcase_add_test (tc_chain, test_notify_race);
   tcase_add_test (tc_chain, test_last_message_notify);
   tcase_skip_broken_test (tc_chain, test_last_message_deep_notify);