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 644284 - Suspicious max_latency computation in gstbaseaudiosink.c
Suspicious max_latency computation in gstbaseaudiosink.c
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
0.10.32
Other All
: Normal minor
: 0.10.36
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-03-09 07:21 UTC by Blaise Gassend
Modified: 2011-07-28 12:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
fix latency calculation for live elements (2.00 KB, patch)
2011-07-20 16:29 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
committed Details | Review

Description Blaise Gassend 2011-03-09 07:21:51 UTC
I have been reviewing latency computation code to try to understand exactly what the max_latency is (still working on that). While reviewing gstbaseaudiosink.c, I came across the following code in the GST_QUERY_LATENCY handling, which seems suspicious:

{{{
          min_latency =
              gst_util_uint64_scale_int (spec->seglatency * spec->segsize,
              GST_SECOND, spec->rate * spec->bytes_per_sample);
          GST_OBJECT_UNLOCK (basesink);

          /* we cannot go lower than the buffer size and the min peer latency */
          min_latency = min_latency + min_l;
          /* the max latency is the max of the peer, we can delay an infinite
           * amount of time. */
          max_latency = min_latency + (max_l == -1 ? 0 : max_l);
}}}

This code seems suspicious to me because in the case where max_l != 0, the final value of max_latency is:
{{{
min_latency_0 + min_l + max_l
}}}
(where min_latency_0 is the value of min_latency returned by gst_util_uint64_scale_int, and min_l and max_l are the latencies for the upstream pipeline)

I am still trying to grasp the exact meaning of max_latency, but it seems very suspicious to me that we would be adding min_l and max_l, so I thought I would flag this computation. 

It seems likely to me that the author intended to be using min_latency_0 in the max_latency computation, and forgot that min_latency had already been operated on.
Comment 1 Sebastian Dröge (slomo) 2011-03-15 19:09:52 UTC
Looks suspicious to me too... Wim?
Comment 2 Stefan Sauer (gstreamer, gtkdoc dev) 2011-07-20 16:29:25 UTC
Created attachment 192318 [details] [review]
fix latency calculation for live elements

Agree too. What about the attached patch? For some reason this test is failing already before making changes, the patch is neutral on it.

Running suite(s): baseaudiosrc
0%: Checks: 1, Failures: 1, Errors: 0
pipelines/basetime.c:49:F:general:test_basetime_calculation:0: Two buffers had same timestamp
FAIL: pipelines/basetime
Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2011-07-28 12:24:24 UTC
Okay the test is unrelated and got fixed.