GNOME Bugzilla – Bug 723236
caps: fix gst_caps_get_features return
Last modified: 2014-02-04 17:50:48 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.
Created attachment 267522 [details] [review] gstcaps: fix gst_caps_get_features return
Created attachment 267523 [details] [review] gstcaps: fix gst_caps_get_features return
Created attachment 267532 [details] [review] tests: add caps features unit tests
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.
Review of attachment 267532 [details] [review]: Looks good
(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.
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.
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.
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.
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
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