audioaggregator: Fix mixup of running times and segment positions

We have to queue buffers based on their running time, not based on
the segment position.

Also return running time from GstAggregator::get_next_time() instead of
a segment position, as required by the API.

Also only update the segment position after we pushed a buffer, otherwise
we're going to push down a segment event with the next position already.

https://bugzilla.gnome.org/show_bug.cgi?id=753196

1 files changed, 100 insertions, 38 deletions

diff --git a/gst/audiomixer/gstaudioaggregator.c b/gst/audiomixer/gstaudioaggregator.c
index 3b59a4295..c30bc6486 100644
--- a/gst/audiomixer/gstaudioaggregator.c
+++ b/gst/audiomixer/gstaudioaggregator.c
@@ -53,11 +53,12 @@ struct _GstAudioAggregatorPadPrivate
                                    cached values. */
   guint position, size;
 
-  guint64 output_offset;        /* Offset in output segment that
-                                   collect.pos refers to in the
+  guint64 output_offset;        /* Sample offset in output segment relative to
+                                   segment.start that collect.pos refers to in the
                                    current buffer. */
 
-  guint64 next_offset;          /* Next expected offset in the input segment */
+  guint64 next_offset;          /* Next expected sample offset in the input segment
+                                   relative to segment.start */
 
   /* Last time we noticed a discont */
   GstClockTime discont_time;
@@ -145,7 +146,8 @@ struct _GstAudioAggregatorPrivate
 
   /* counters to keep track of timestamps */
   /* Readable with object lock, writable with both aag lock and object lock */
-  gint64 offset;
+
+  gint64 offset;                /* Sample offset starting from 0 at segment.start */
 };
 
 #define GST_AUDIO_AGGREGATOR_LOCK(self)   g_mutex_lock (&(self)->priv->mutex);
@@ -195,10 +197,16 @@ gst_audio_aggregator_get_next_time (GstAggregator * agg)
   GstClockTime next_time;
 
   GST_OBJECT_LOCK (agg);
-  if (agg->segment.position == -1)
+  if (agg->segment.position == -1 || agg->segment.position < agg->segment.start)
     next_time = agg->segment.start;
   else
     next_time = agg->segment.position;
+
+  if (agg->segment.stop != -1 && next_time > agg->segment.stop)
+    next_time = agg->segment.stop;
+
+  next_time =
+      gst_segment_to_running_time (&agg->segment, GST_FORMAT_TIME, next_time);
   GST_OBJECT_UNLOCK (agg);
 
   return next_time;
@@ -742,6 +750,7 @@ gst_audio_aggregator_fill_buffer (GstAudioAggregator * aagg,
   guint64 start_offset, end_offset;
   gint rate, bpf;
 
+  GstAggregator *agg = GST_AGGREGATOR (aagg);
   GstAggregatorPad *aggpad = GST_AGGREGATOR_PAD (pad);
 
   g_assert (pad->priv->buffer == NULL);
@@ -767,7 +776,12 @@ gst_audio_aggregator_fill_buffer (GstAudioAggregator * aagg,
       start_time + gst_util_uint64_scale_ceil (pad->priv->size, GST_SECOND,
       rate);
 
-  start_offset = gst_util_uint64_scale (start_time, rate, GST_SECOND);
+  /* Clipping should've ensured this */
+  g_assert (start_time >= aggpad->segment.start);
+
+  start_offset =
+      gst_util_uint64_scale (start_time - aggpad->segment.start, rate,
+      GST_SECOND);
   end_offset = start_offset + pad->priv->size;
 
   if (GST_BUFFER_IS_DISCONT (inbuf)
@@ -822,8 +836,8 @@ gst_audio_aggregator_fill_buffer (GstAudioAggregator * aagg,
   if (pad->priv->output_offset == -1) {
     GstClockTime start_running_time;
     GstClockTime end_running_time;
-    guint64 start_running_time_offset;
-    guint64 end_running_time_offset;
+    guint64 start_output_offset;
+    guint64 end_output_offset;
 
     start_running_time =
         gst_segment_to_running_time (&aggpad->segment,
@@ -831,12 +845,40 @@ gst_audio_aggregator_fill_buffer (GstAudioAggregator * aagg,
     end_running_time =
         gst_segment_to_running_time (&aggpad->segment,
         GST_FORMAT_TIME, end_time);
-    start_running_time_offset =
-        gst_util_uint64_scale (start_running_time, rate, GST_SECOND);
-    end_running_time_offset =
-        gst_util_uint64_scale (end_running_time, rate, GST_SECOND);
 
-    if (end_running_time_offset < aagg->priv->offset) {
+    /* Convert to position in the output segment */
+    start_output_offset =
+        gst_segment_to_position (&agg->segment, GST_FORMAT_TIME,
+        start_running_time);
+    if (start_output_offset != -1)
+      start_output_offset =
+          gst_util_uint64_scale (start_output_offset - agg->segment.start, rate,
+          GST_SECOND);
+
+    end_output_offset =
+        gst_segment_to_position (&agg->segment, GST_FORMAT_TIME,
+        end_running_time);
+    if (end_output_offset != -1)
+      end_output_offset =
+          gst_util_uint64_scale (end_output_offset - agg->segment.start, rate,
+          GST_SECOND);
+
+    if (start_output_offset == -1 && end_output_offset == -1) {
+      /* Outside output segment, drop */
+      gst_buffer_unref (inbuf);
+      pad->priv->buffer = NULL;
+      pad->priv->position = 0;
+      pad->priv->size = 0;
+      pad->priv->output_offset = -1;
+      GST_DEBUG_OBJECT (pad, "Buffer outside output segment");
+      return FALSE;
+    }
+
+    /* Calculate end_output_offset if it was outside the output segment */
+    if (end_output_offset == -1)
+      end_output_offset = start_output_offset + pad->priv->size;
+
+    if (end_output_offset < aagg->priv->offset) {
       /* Before output segment, drop */
       gst_buffer_unref (inbuf);
       pad->priv->buffer = NULL;
@@ -845,12 +887,25 @@ gst_audio_aggregator_fill_buffer (GstAudioAggregator * aagg,
       pad->priv->output_offset = -1;
       GST_DEBUG_OBJECT (pad,
           "Buffer before segment or current position: %" G_GUINT64_FORMAT " < %"
-          G_GUINT64_FORMAT, end_running_time_offset, aagg->priv->offset);
+          G_GUINT64_FORMAT, end_output_offset, aagg->priv->offset);
       return FALSE;
     }
 
-    if (start_running_time_offset < aagg->priv->offset) {
-      guint diff = aagg->priv->offset - start_running_time_offset;
+    if (start_output_offset == -1 || start_output_offset < aagg->priv->offset) {
+      guint diff;
+
+      if (start_output_offset == -1 && end_output_offset < pad->priv->size) {
+        diff = pad->priv->size - end_output_offset + aagg->priv->offset;
+      } else if (start_output_offset == -1) {
+        start_output_offset = end_output_offset - pad->priv->size;
+
+        if (start_output_offset < aagg->priv->offset)
+          diff = aagg->priv->offset - start_output_offset;
+        else
+          diff = 0;
+      } else {
+        diff = aagg->priv->offset - start_output_offset;
+      }
 
       pad->priv->position += diff;
       if (pad->priv->position >= pad->priv->size) {
@@ -862,14 +917,16 @@ gst_audio_aggregator_fill_buffer (GstAudioAggregator * aagg,
         pad->priv->output_offset = -1;
         GST_DEBUG_OBJECT (pad,
             "Buffer before segment or current position: %" G_GUINT64_FORMAT
-            " < %" G_GUINT64_FORMAT, end_running_time_offset,
-            aagg->priv->offset);
+            " < %" G_GUINT64_FORMAT, end_output_offset, aagg->priv->offset);
         return FALSE;
       }
     }
 
-    pad->priv->output_offset =
-        MAX (start_running_time_offset, aagg->priv->offset);
+    if (start_output_offset == -1 || start_output_offset < aagg->priv->offset)
+      pad->priv->output_offset = aagg->priv->offset;
+    else
+      pad->priv->output_offset = start_output_offset;
+
     GST_DEBUG_OBJECT (pad,
         "Buffer resynced: Pad offset %" G_GUINT64_FORMAT
         ", current audio aggregator offset %" G_GUINT64_FORMAT,
@@ -1066,13 +1123,10 @@ gst_audio_aggregator_aggregate (GstAggregator * agg, gboolean timeout)
     GST_OBJECT_UNLOCK (agg);
     gst_aggregator_set_src_caps (agg, aagg->current_caps);
     GST_OBJECT_LOCK (agg);
-    aagg->priv->offset = gst_util_uint64_scale (agg->segment.position,
-        GST_AUDIO_INFO_RATE (&aagg->info), GST_SECOND);
 
     aagg->priv->send_caps = FALSE;
   }
 
-
   rate = GST_AUDIO_INFO_RATE (&aagg->info);
   bpf = GST_AUDIO_INFO_BPF (&aagg->info);
 
@@ -1090,7 +1144,9 @@ gst_audio_aggregator_aggregate (GstAggregator * agg, gboolean timeout)
     next_offset = aagg->priv->offset - blocksize;
   }
 
-  next_timestamp = gst_util_uint64_scale (next_offset, GST_SECOND, rate);
+  next_timestamp =
+      agg->segment.start + gst_util_uint64_scale (next_offset, GST_SECOND,
+      rate);
 
   if (aagg->priv->current_buffer == NULL) {
     GST_OBJECT_UNLOCK (agg);
@@ -1248,7 +1304,9 @@ gst_audio_aggregator_aggregate (GstAggregator * agg, gboolean timeout)
           "Last buffer is incomplete: %" G_GUINT64_FORMAT " <= %"
           G_GUINT64_FORMAT, max_offset, next_offset);
       next_offset = max_offset;
-      next_timestamp = gst_util_uint64_scale (next_offset, GST_SECOND, rate);
+      next_timestamp =
+          agg->segment.start + gst_util_uint64_scale (next_offset, GST_SECOND,
+          rate);
 
       if (next_offset > aagg->priv->offset)
         gst_buffer_resize (outbuf, 0, (next_offset - aagg->priv->offset) * bpf);
@@ -1269,6 +1327,23 @@ gst_audio_aggregator_aggregate (GstAggregator * agg, gboolean timeout)
     GST_BUFFER_DURATION (outbuf) = agg->segment.position - next_timestamp;
   }
 
+  GST_OBJECT_UNLOCK (agg);
+
+  /* send it out */
+  GST_LOG_OBJECT (aagg,
+      "pushing outbuf %p, timestamp %" GST_TIME_FORMAT " offset %"
+      G_GINT64_FORMAT, outbuf, GST_TIME_ARGS (GST_BUFFER_PTS (outbuf)),
+      GST_BUFFER_OFFSET (outbuf));
+
+  GST_AUDIO_AGGREGATOR_UNLOCK (aagg);
+
+  ret = gst_aggregator_finish_buffer (agg, aagg->priv->current_buffer);
+  aagg->priv->current_buffer = NULL;
+
+  GST_LOG_OBJECT (aagg, "pushed outbuf, result = %s", gst_flow_get_name (ret));
+
+  GST_AUDIO_AGGREGATOR_LOCK (aagg);
+  GST_OBJECT_LOCK (agg);
   aagg->priv->offset = next_offset;
   agg->segment.position = next_timestamp;
 
@@ -1285,22 +1360,9 @@ gst_audio_aggregator_aggregate (GstAggregator * agg, gboolean timeout)
       GST_OBJECT_UNLOCK (pad);
     }
   }
-
   GST_OBJECT_UNLOCK (agg);
-
-  /* send it out */
-  GST_LOG_OBJECT (aagg,
-      "pushing outbuf %p, timestamp %" GST_TIME_FORMAT " offset %"
-      G_GINT64_FORMAT, outbuf, GST_TIME_ARGS (GST_BUFFER_PTS (outbuf)),
-      GST_BUFFER_OFFSET (outbuf));
-
   GST_AUDIO_AGGREGATOR_UNLOCK (aagg);
 
-  ret = gst_aggregator_finish_buffer (agg, aagg->priv->current_buffer);
-  aagg->priv->current_buffer = NULL;
-
-  GST_LOG_OBJECT (aagg, "pushed outbuf, result = %s", gst_flow_get_name (ret));
-
   return ret;
   /* ERRORS */
 not_negotiated: