GNOME Bugzilla – Bug 755237
Caps Feature Negotiations have bad defaults
Last modified: 2015-09-20 03:08:42 UTC
I recently ran into an issue with the defaulting of Caps Features. We have an element we've written that can optionally accept a meta:GstTextOverlayComposition feature on input, so we set up our sink caps for the element as: Sink: video/x-raw(meta:GstTextOverlayComposition), ... ; video/x-raw, ... But it was failing to negotiate a connection when there was text overlay metadata. On examination of the negotiation, we saw that the element was being presented with: video/x-raw(memory:SystemMemory, meta:GstTextOverlayComposition), ... and when our element did an intersect, it got an EMPTY result. I checked the source code for gst_caps_intersect_full() and found that it only assumes a default feature of memory:SystemMemory if features are completely empty. Thus, when the negotiations presented video/x-raw(memory:SystemMemory),... negotiations succeeded, but when there was a text overlay, they failed. Wouldn't it make more sense to assume memory:SystemMemory in the absence of any other feature with a 'memory:' prefix, rather than requiring an empty feature list? This would result in the same backwards compatibility, while making it easier to work with the new Caps features.
I won't close it right way, but it seems like not a bug. If you don't set any caps feature it will default to (memory:SystemMemory) if you do specify a caps feature with no memory:, then it is assumed that your element don't support any type of memory. I don't think it's really a bug. A case where this could happen is with OMX Tunneling (not yet implemented though).
This seems inconsistent. Either memory:SystemMemory should be a default, or it shouldn't. Having conditions where it is and when it isn't is what causes confusion.
You are asking us to prevent having a valid case where memory should be NONE (reducing what this can do). The default here only exist for when you set no caps feature at all. mime/type,fields=... This defaults to: mime/type(memory:SystemMemory),fields=... There is no ambiguity when you do set a caps features. mime/type(meta:MyMeta),feilds=... Should not in my opinion match: mime/type(memory:SystemMemory, meta:MyMeta),feilds=... The first says, I want MyMeta and no memory shall be used (like a OMX tunnel), the second says I want MyMeta with memory compatible with SystemMemory.
NO, what I'm asking is for consistency. Either you should assume memory:SystemMemory in cases where its absent, or NOT. Having it depend on whether other Features exist or not is counter-intuitive. Since you've already decided you'll introduce a memory:SystemMemory feature, and then make it a default for backwards compatibility reasons, then you've ALREADY broken anything (like our code) that was written to use Features between the time you introduced Features, and the time you decided that memory:SystemMemory would be introduced to all negotiation. I think you made some bad design decisions, but if you really don't want to change them and keep what you have, you'd be better off introducing a memory:NONE option for the rare case where no memory is involved.
Can you bring a real arguments, accusation are no rationale. If you stick with rationales, we'll manage to get somewhere. Being polite and trying to understand why there is a debate about whether or not an existing (and deployed) implementation is correct is for me the way to go. Also, nothing of this will change before 1.8, so we have plenty of time to think about it. Caps features have worked this way since the day they where introduced. I have personally nothing to do with that, I'm only acting as Devil's Advocate here. memory:None (consistency requires camel case here) could be an option. Personally, I think not specifying memory:SystemMemory but specifying other features is creating confusion, and shall never be used. One known reason why adding a default memory:SytemMemory if there is not memory: already in place is inconvenient is that it requires parsing all the features already in place. This is a very costy operation during caps negotiation. Caps feature name are free-form. Do you have a solution to that, that would not require parsing every strings ? (a side note, Arnaud Vrac and I have manage to successfully implement OverlayComposition in many elements, even though some of it didn't make it for 1.6. We do believe the API works, hence it's not fundamental to change it, if it can be changed, as ABI and API compatibility is very important, even when we do mistake)
Memory transfer is not the only way to transfer data as negotiated through caps features. There are current use cases (meta:GstVideoGLTextureUploadMeta) that will break if we assume a lack of a memory caps feature means memory:SystemMemory which could be downright dangerous. The best option for you is to explicitly add the memory:SystemMemory caps feature to your negotations when dealing with other caps features. Think of it like this, no caps features == memory:SystemMemory. All other comparisons are based on normal equivalence so: meta:SomeMeta != memory:SystemMemory,meta:SomeMeta
I still contend that the current design is both confusing and counter-intuitive. I would hope that you will at least document how memory:SystemMemory defaults in the documentation. As far as I know its not currently documented anywhere. (My first indication it existed was when debugging this particular issue).
Improving the documentation is always welcome, like any of us, if you find something unclear, file a bug, if you have enough time, propose a patch.