Agreed (I thought we fixed that ages ago..)

Created attachment 324147 [details] [review]
appsrc: If do-timestamp=TRUE, capture the time when the buffer was pushed to the source

... instead of the time when it was pushed further downstream.

This has the main problem that a buffer might be pushed to the source when it is not PLAYING yet (-> no clock). In that case, basesrc will timestamp the buffer just before pushing. But future buffers that are pushed to appsrc by the application might be timestamp by appsrc before basesrc timestamps the previous buffer, causing timestamps to be misordered.

Review of attachment 324147 [details] [review]:

::: gst-libs/gst/app/gstappsrc.c
@@ +1620,3 @@
+  if (GST_BUFFER_DTS (buffer) == GST_CLOCK_TIME_NONE &&
+      GST_BUFFER_PTS (buffer) == GST_CLOCK_TIME_NONE &&
+      gst_base_src_get_do_timestamp (GST_BASE_SRC_CAST (appsrc))) {

That makes do-timestamp conditional to not having timestamp on the buffers, is this consistent with other do-timestamp implementation ?

I also wonder if the behaviour should not vary depending if we are live or not. Being media agnostic does not help of course.

(In reply to Nicolas Dufresne (stormer) from comment #4)
> Review of attachment 324147 [details] [review] [review]:
> 
> ::: gst-libs/gst/app/gstappsrc.c
> @@ +1620,3 @@
> +  if (GST_BUFFER_DTS (buffer) == GST_CLOCK_TIME_NONE &&
> +      GST_BUFFER_PTS (buffer) == GST_CLOCK_TIME_NONE &&
> +      gst_base_src_get_do_timestamp (GST_BASE_SRC_CAST (appsrc))) {
> 
> That makes do-timestamp conditional to not having timestamp on the buffers,
> is this consistent with other do-timestamp implementation ?

https://cgit.freedesktop.org/gstreamer/gstreamer/tree/libs/gst/base/gstbasesrc.c#n2243

Any other comments, ideas?

Ok then, as it was like this before.

I was mostkyy wondering about what I wrote in comment 3 :)

Maybe we should only do this if the appsrc is configured to be live, and also warn if someone asks us to timestamp without having configured the source to be live (is there a good use case for that?)

Even if the source is live, nothing is preventing the application from providing buffers before the pipeline is actually PLAYING.

That is true, but would be a bug in the application imho :)

(Meaning: we can warn, and then do the timestamp when outputting as now)

That sounds like an acceptable approach, yes. I don't think there's any point in do-timestamp on non-live streams and I would even consider a warning about that inside basesrc. What do you think?

Can't we just put "0" or GST_CLOCK_TIME_NONE on buffers that are pushed before starting? It could be used to push headers or maybe the first "not really preroll" frame.

NONE would make it use the basesrc timestamping mechanism, which leads to the problem mentioned above (newer buffer gets timestamped by appsrc, older buffer gets timestamped by basesrc => newer < older).

0 could lead to big gaps. Running time does not start at 0 always.


I think I prefer using NONE and warn about it.

Override the property, keep the value, gst_base_src_set_do_timestamp(FALSE), then the base class won't bother you. 

You should also do like GstBaseSrc and overwrite timestamps even if they are not NONE, no?

And it always starts at 0 as you're timestamping with the running time.

(In reply to Olivier Crête from comment #17)
> Override the property, keep the value, gst_base_src_set_do_timestamp(FALSE),
> then the base class won't bother you. 

But then we would push the first buffers possibly without any timestamp. Generally our code is very unhappy about first buffers having no timestamp

> You should also do like GstBaseSrc and overwrite timestamps even if they are
> not NONE, no?

Yes

> And it always starts at 0 as you're timestamping with the running time.

Not really, e.g. consider the case of the source being added to an already running pipeline or manual managing of the base time

Attachment 324147 [details] pushed as 1032f5c - appsrc: If do-timestamp=TRUE, capture the time when the buffer was pushed to the source

Pushed with the warning, as that seemed to be the result of the above discussion and all alternatives would have fundamental problems.