Created attachment 313196 [details] [review]
multiqueue: Use buffer DTS if present, else PTS

In order to accurately determine the amount (in time) of data
travelling in queues, we should use an increasing value.

If buffers are encoded and potentially reordered, we should be
using their DTS (increasing) and not PTS (reordered)

Makes sense to me and matches basesink behaviour. Should probably do the same in other queue-like elements for fill level.

What would this do if a stream sometimes has DTS and sometimes PTS? Might it causes confusion?

(In reply to Sebastian Dröge (slomo) from comment #2)
> Makes sense to me and matches basesink behaviour. Should probably do the
> same in other queue-like elements for fill level.

  Indeed. If this patch is ok, then the other queue-like elements should be fixed accordingly.

> 
> What would this do if a stream sometimes has DTS and sometimes PTS? Might it
> causes confusion?

  This could very well happen, like when PTS == DTS, some formats only set PTS. But that's fine.
  I'm more worried of decoders (maybe) not removing the DTS on the output buffer. Or any elements producing properly-ordered output. That could cause serious confusion.

When you decode to raw, the end result should have just PTS (PTS == DTS is also fine, but when I come across those, I usually fix it). Leaving trace of the DTS would be a bug.

Review of attachment 313196 [details] [review]:

::: plugins/elements/gstmultiqueue.c
@@ +1157,3 @@
   if (GST_IS_BUFFER (object)) {
     GstBuffer *buf = GST_BUFFER_CAST (object);
+    GstClockTime ptime = GST_BUFFER_DTS (buf);

Nitpick, but ptime sounds like presentation time, a confusing variable name when the first thing you do it set it to DTS. Maybe, timestamp could be better ?

@@ +1166,3 @@
+      if (ptime > segment->stop)
+        ptime = segment->stop;
+      time = gst_segment_to_running_time (segment, GST_FORMAT_TIME, ptime);

As we move toward DTS, I think we need negative TS support here. I suggest turning time into a gint64, and storing a signed version of the running time (optained using the _full method).

Created attachment 314189 [details] [review]
multiqueue: Use buffer DTS if present, else PTS

In order to accurately determine the amount (in time) of data
travelling in queues, we should use an increasing value.

If buffers are encoded and potentially reordered, we should be
using their DTS (increasing) and not PTS (reordered)

Created attachment 314190 [details] [review]
queue/queue2: Use GST_BUFFER_DTS_OR_PTS

The input of queue/queue2 might have DTS set, in which cas we want
to take that into account (instead of the PTS) to calculate position
and queue levels.

The switch to handling negative ts can be done in a later step. It still seems quite confusing.

commit a9c923d585affc85262e41b4790d378294706d7a
Author: Edward Hervey <bilboed@bilboed.com>
Date:   Tue Oct 27 08:48:07 2015 +0100

    queue/queue2: Use GST_BUFFER_DTS_OR_PTS
    
    The input of queue/queue2 might have DTS set, in which cas we want
    to take that into account (instead of the PTS) to calculate position
    and queue levels.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=756507

commit 692c0dc060807fd1ea813aa81e9c2656fd607b40
Author: Edward Hervey <edward@centricular.com>
Date:   Tue Oct 13 17:20:26 2015 +0200

    multiqueue: Use buffer DTS if present, else PTS
    
    In order to accurately determine the amount (in time) of data
    travelling in queues, we should use an increasing value.
    
    If buffers are encoded and potentially reordered, we should be
    using their DTS (increasing) and not PTS (reordered)
    
    https://bugzilla.gnome.org/show_bug.cgi?id=756507

Please leave open until it's fully implemented.

(In reply to Edward Hervey from comment #8)
> It still seems quite confusing.

I'm not sure what you are referring to.