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 757050 - liveadder: type change of "latency" property might cause crashes
liveadder: type change of "latency" property might cause crashes
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal blocker
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-10-24 11:32 UTC by Tim-Philipp Müller
Modified: 2015-10-28 22:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
aggregator: Expose functions to set the latency property (3.23 KB, patch)
2015-10-27 00:00 UTC, Olivier Crête
rejected Details | Review
liveadder: Make latency property be a uint in millisecs (2.76 KB, patch)
2015-10-27 00:00 UTC, Olivier Crête
needs-work Details | Review
liveadder: Make latency property be a uint in millisecs (3.41 KB, patch)
2015-10-27 23:05 UTC, Olivier Crête
accepted-commit_now Details | Review

Description Tim-Philipp Müller 2015-10-24 11:32:00 UTC
This commit

commit 305e5c7ac3c6cebc0280de25b9736bcf058f973a
Author: Olivier Crête <olivier.crete@collabora.com>
Date:   Mon Oct 5 00:55:36 2015 +0100

    liveadder: Remove plugin, replace by compat subclass of audiomixer
    
    New subclass with a similar behaviour as the old liveadder, but
    a slightly different API as the latency is in nanoseconds, not
    milliseconds. Also, the new liveadder has a effective latency that
    is latency + output-buffer-duration. In practice, just setting a non-zero
    latency with the new audiomixer gives you the right behavior in 99% of the
    cases.


changed the type and unit of the "latency" property from milliseconds (guint) to nanoseconds (guint64).

This can cause crashes in applications that use the liveadder element.

We should fix this to be really actually "compat", there's no good reason not to do that.
Comment 1 Olivier Crête 2015-10-27 00:00:00 UTC
Created attachment 314171 [details] [review]
aggregator: Expose functions to set the latency property

Allows subclasses to override the behaviour of the latency property.

API: gst_aggregator_set_latency_property
API: gst_aggregator_get_latency_property
Comment 2 Olivier Crête 2015-10-27 00:00:05 UTC
Created attachment 314172 [details] [review]
liveadder: Make latency property be a uint in millisecs

This restores roughly the same behaviour as the old liveadder element.
Except that the latency now also includes the output-buffer-duration.
Comment 3 Tim-Philipp Müller 2015-10-27 00:10:33 UTC
Thanks for the fix. Do we need the new public API? I'd just leave it as internal helper function unless something else needs it.
Comment 4 Olivier Crête 2015-10-27 04:07:36 UTC
GstAggregator is in a library, the liveadder is in a plugin...
Comment 5 Sebastian Dröge (slomo) 2015-10-27 07:10:14 UTC
Why do we need new API? You could make liveadder just a GstBin around audiomixer, and then expose whatever properties you want on it in whatever unit you want.
Comment 6 Nicolas Dufresne (ndufresne) 2015-10-27 12:56:09 UTC
I believe you can override a property without new machanism. See g_param_spec_override(). Careful, as you may need to translate the param_id.
Comment 7 Olivier Crête 2015-10-27 23:05:52 UTC
Created attachment 314264 [details] [review]
liveadder: Make latency property be a uint in millisecs

Now without exposing the setter/getter functions.
Comment 8 Olivier Crête 2015-10-27 23:06:42 UTC
Making it a bin would be quite painful because of the request pads, etc..
Comment 9 Olivier Crête 2015-10-28 22:59:48 UTC
Merged

commit 0e2880ac2eafc86c30056432d0cb84ba2bde65b2
Author: Olivier Crête <olivier.crete@collabora.com>
Date:   Mon Oct 26 19:58:04 2015 -0400

    liveadder: Make latency property be a uint in millisecs
    
    This restores roughly the same behaviour as the old liveadder element.
    Except that the latency now also includes the output-buffer-duration.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=757050