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 773093 - basetransform: Fix race when toggling passthrough
basetransform: Fix race when toggling passthrough
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
unspecified
Other All
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-10-17 12:17 UTC by Stian Selnes (stianse)
Modified: 2018-11-03 12:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
basetransform: Fix race when toggling passthrough (13.51 KB, patch)
2016-10-17 12:17 UTC, Stian Selnes (stianse)
reviewed Details | Review

Description Stian Selnes (stianse) 2016-10-17 12:17:29 UTC
This fixes a race where priv->passthrough is changed from TRUE to FALSE
while processing a buffer and we end up passing a non-writable buffer to
transform_ip(). More precicely if passthrough is changed just after
prepare_output_buffer() is finished.

Since priv->passthrough and other priv variables are accessed throughout
the chain function a lock is introduced and held while processing the
buffer, but released before pushing downstream. Since sub-classes may
call is_passthrough() and similar functions during for instance
transform_ip() a recursive lock is needed.
Comment 1 Stian Selnes (stianse) 2016-10-17 12:17:33 UTC
Created attachment 337841 [details] [review]
basetransform: Fix race when toggling passthrough
Comment 2 Sebastian Dröge (slomo) 2016-10-20 11:06:35 UTC
Review of attachment 337841 [details] [review]:

I'm worried of the performance impact of this here. Wouldn't it be better to remember passthrough throughout the processing of a single buffer at the very beginning, and not allow changing it on the way? This doesn't seem like something meaningful to do anyway. Or I'm misunderstanding the problem here :)

::: libs/gst/base/gstbasetransform.c
@@ +2053,3 @@
    */
+  if (!priv->negotiated && !priv->passthrough &&
+      (bclass->set_caps != NULL))

Shouldn't this here also take the lock?
Comment 3 GStreamer system administrator 2018-11-03 12:37:20 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/197.