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 723236 - caps: fix gst_caps_get_features return
caps: fix gst_caps_get_features return
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal blocker
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 721953
 
 
Reported: 2014-01-29 14:02 UTC by Matthieu Bouron
Modified: 2014-02-04 17:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gstcaps: fix gst_caps_get_features return (1.32 KB, patch)
2014-01-29 14:06 UTC, Matthieu Bouron
none Details | Review
gstcaps: fix gst_caps_get_features return (1.64 KB, patch)
2014-01-29 14:20 UTC, Matthieu Bouron
needs-work Details | Review
tests: add caps features unit tests (1.10 KB, patch)
2014-01-29 14:42 UTC, Matthieu Bouron
committed Details | Review
caps: ensure that each newly created caps has its own caps features (3.55 KB, patch)
2014-02-03 16:45 UTC, Matthieu Bouron
none Details | Review
caps: ensure that each newly created caps has its own caps features (3.60 KB, patch)
2014-02-04 00:04 UTC, Matthieu Bouron
none Details | Review
caps: ensure that each newly created caps has its own caps features (3.49 KB, patch)
2014-02-04 11:48 UTC, Matthieu Bouron
rejected Details | Review

Description Matthieu Bouron 2014-01-29 14:02:53 UTC
gst_caps_get_features returns a preallocated caps structures corresponding to memory:SystemMemory if the current caps features are NULL.

This behaviour leads to an issue when the user tries to modify the features using gst_caps_features + gst_caps_features_add since it will directly modifies the preallocated caps features.
Comment 1 Matthieu Bouron 2014-01-29 14:06:21 UTC
Created attachment 267522 [details] [review]
gstcaps: fix gst_caps_get_features return
Comment 2 Matthieu Bouron 2014-01-29 14:20:14 UTC
Created attachment 267523 [details] [review]
gstcaps: fix gst_caps_get_features return
Comment 3 Matthieu Bouron 2014-01-29 14:42:10 UTC
Created attachment 267532 [details] [review]
tests: add caps features unit tests
Comment 4 Sebastian Dröge (slomo) 2014-01-29 19:31:53 UTC
Review of attachment 267523 [details] [review]:

::: gst/gstcaps.c
@@ +857,3 @@
 
+  storage = &gst_caps_get_features_unchecked (caps, index);
+  features = *storage;

Why do you do this dance with & and the ** here? That does not seem to do anything useful at all other than being confusing :) Just add the capsfeatures with the normal API.

Also you can't just add the capsfeatures to the caps because the caps might not be writable. So many threads could call this at the same time and weird things could happen.

I don't have a better idea though, the way this function works now is not very useful.
Comment 5 Sebastian Dröge (slomo) 2014-01-29 19:32:27 UTC
Review of attachment 267532 [details] [review]:

Looks good
Comment 6 Matthieu Bouron 2014-01-29 22:26:34 UTC
(In reply to comment #4)
> Review of attachment 267523 [details] [review]:
> 
> ::: gst/gstcaps.c
> @@ +857,3 @@
> 
> +  storage = &gst_caps_get_features_unchecked (caps, index);
> +  features = *storage;
> 
> Why do you do this dance with & and the ** here? That does not seem to do
> anything useful at all other than being confusing :) Just add the capsfeatures
> with the normal API.

I reused the same method as in the gst_caps_set_features function since I couldn't use the regular API.

> 
> Also you can't just add the capsfeatures to the caps because the caps might not
> be writable. So many threads could call this at the same time and weird things
> could happen.
> 
> I don't have a better idea though, the way this function works now is not very
> useful.

I see two solutions: 
1) ensure at caps creation that a feature is set by default for each caps structure (ie: memory:SystemMemory).
2) use some sort of locking in the get_features function.
Comment 7 Matthieu Bouron 2014-02-03 16:45:56 UTC
Created attachment 267966 [details] [review]
caps: ensure that each newly created caps has its own caps features

Patch updated:
  * gst_caps_get_features returns the caps directly even if they are NULL (error message + assert). But it should not be the case.
  * gst_caps_set_features is now able to overwrite existing caps features as intended if the caps are writable. 
  * all newly created caps will own a gst caps features defaulting to memory:SystemMemory if nothing (NULL) is specified.

Obvisouly, this patch can be splitted.
Comments welcome.
Comment 8 Matthieu Bouron 2014-02-04 00:04:42 UTC
Created attachment 268023 [details] [review]
caps: ensure that each newly created caps has its own caps features

Patch updated:
  * fix gst_caps_append_structure_unchecked macro to prevent gst caps features from leaking.

Note: gst_caps_features_copy_conditional might be simplified in a future update.
Comment 9 Matthieu Bouron 2014-02-04 11:48:08 UTC
Created attachment 268056 [details] [review]
caps: ensure that each newly created caps has its own caps features

Patch updated:
  * simplified gst_caps_features_copy_conditional.
Comment 10 Sebastian Dröge (slomo) 2014-02-04 16:52:00 UTC
Related to this is the following commit. That makes your test fail in more useful ways now.

commit 23434848d3ddfe25f455d48793cf0438c9ec7966
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Tue Feb 4 17:48:54 2014 +0100

    capsfeatures: Make sure that the static ANY/EMPTY capsfeatures are never mutable
    
    See https://bugzilla.gnome.org/show_bug.cgi?id=723236
Comment 11 Sebastian Dröge (slomo) 2014-02-04 17:50:35 UTC
Should be all good now without the overhead of adding capsfeatures to every single caps out there.

commit 97103c90b0adadf695f02322ae440a123f7c706d
Author: Matthieu Bouron <matthieu.bouron@collabora.com>
Date:   Wed Jan 29 14:39:19 2014 +0000

    tests: add caps features unit tests
    
    https://bugzilla.gnome.org/show_bug.cgi?id=723236

commit 8d7e11e307dbd4a8a8a053f2a05af1098957a070
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Tue Feb 4 18:42:02 2014 +0100

    caps: When getting capsfeatures and none are there, store sysmem capsfeatures
    
    ... instead of returning a reference to a global instance. The caller might
    want to change the global instance otherwise, which causes funny effects like
    all global instances being changed and at the same time nothing in the caps
    being changed.
    
    As the caps might be immutable while we do this we have to do some magic
    with atomic operations.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=723236

commit e51e0cb0d3722f0d95128ad8ec7e79ae7b5e568a
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Tue Feb 4 18:03:47 2014 +0100

    caps: Don't get us sysmem capsfeatures if we just check for fixed caps