After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 788431 - tracers: Latency tracer should not include sink synchronization pause
tracers: Latency tracer should not include sink synchronization pause
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
1.13.x
Other Linux
: Normal normal
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-10-02 14:57 UTC by Nicolas Dufresne (ndufresne)
Modified: 2017-10-30 19:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
latency-tracer: Exclude synchronization time (3.25 KB, patch)
2017-10-11 15:17 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Nicolas Dufresne (ndufresne) 2017-10-02 14:57:41 UTC
As of today, we calculate the latency after the buffer has been pushed out of the peer of the sink sinkpad. As a side effect, if the sink is doing synchronization on the clock, we will wait for a variable about of time, time that gets included into the measure latency. This makes the result quite unreliable (specially on video).

The intention with this trace is to measure the effective latency (including processing time) inside the pipeline, ignoring the sources and sink latency, which cannot be measured effectively.

To illustrace better, let's take two camera sources. For both the latency have been estimated to 1 frame (let's assume 17ms). Then I build a supposedly zero-latency pipeline like this:

  videosource1 ! videosink
  videosource2 ! videosink

Both sources produce proper timestamps. If videosource1 effectively had 16ms of latency, the latency tracer would 1ms latency, while if the videosource2 only take 2ms of real latency, the latency tracer report 14ms. So basically, a lower latency camera reports more latency through the tracer. This is the case for v4l2src which currently just assume 1 frame latency since the API does not explicitly give back this latency. I'm working on adding a proper latency estimation in that element, but still, would like this to not affect the value from the latency tracer as it's counter intuitive.
Comment 1 Guillaume Desmottes 2017-10-11 13:43:21 UTC
What's the proper way to fix this?
Just switching to the "pad-push-(list)-pre" hooks on the sink's pad?
Comment 2 Nicolas Dufresne (ndufresne) 2017-10-11 14:00:24 UTC
I think so, but you also need to move some stuff to the sink sinkpad peer, otherwise you'll get a race and then it's worst, the result includes the last buffer duration (was the result of my quick and dirty attempt).
Comment 3 Nicolas Dufresne (ndufresne) 2017-10-11 15:17:29 UTC
Created attachment 361332 [details] [review]
latency-tracer: Exclude synchronization time

The goal of this tracer is to measure the processing latency between a
src and a sink. In push mode, the time was read after the chain function
have returned. As the amount of time we wait to get synched is reverse
to the amount of latency the source introduced, the result was quite
surprising.

This patch moves the latency calculation in the pre-push hook. When
there is no processing in a a pipeline (e.g. fakesrc ! fakesink), the
latency will now be 0 as it's supposed to. For pull mode, the code was
already correct. When GstBaseSink operate in pull mode, the processing
time is done durring the pull, so pull-post is the right hook. The
synchronization will happen after the pull has ended. Note that
GstBaseSink rarely operate in pull mode.
Comment 4 Guillaume Desmottes 2017-10-12 09:18:33 UTC
Review of attachment 361332 [details] [review]:

Typo in commit msg: "synched".

Looks good. Maybe add a comment in the code about the push/pull hooks distinction as it's not that trivial?
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2017-10-19 08:35:14 UTC
Review of attachment 361332 [details] [review]:

Sounds good to me. +1 for expanding the doc-comment to explain the reasoning about the hooks.
Comment 6 Nicolas Dufresne (ndufresne) 2017-10-30 19:33:31 UTC
Attachment 361332 [details] pushed as 41e35c3 - latency-tracer: Exclude synchronization time