GNOME Bugzilla – Bug 735626
multipartdemux: caps are NULL in pad-added callback (regression)
Last modified: 2014-08-29 09:51:19 UTC
In 1.4, the caps for the new pad are now NULL in the pad-added callback.
The commit that introduced the change was:
I confirm, the first solution I have in mind is to stick the two events and then add the pad but gst_pad_create_stream_id() require the pad to be parented. So I'm blocked ...
<jlitzinger> In Gstreamer 1.4, when a multipartdemux creates a pad, it sets the pad's caps after creating the pad, whereas in 1.2, the caps were set before the pad was created. The effect is that, in the pad-added callback, the caps are NULL where they weren't before. Would this be considered a bug, or was the change intentional?
<__tim> it sounds like a bug to me, but did you check the commit log? :)
<jlitzinger> yeah, I see where it happened, but no mention of this effect
<jlitzinger> I'll get the sha
<__tim> I don't think it was on purpose
<jlitzinger> I'll log a bug, and add a proposed patch to it
<__tim> stormer, ^^
<jlitzinger> Actually, it does look intentional...
<jlitzinger> The change was to ensure a stream-start event is sent prior to caps, to avoid the ""Got data flow before stream-start event" warning...
<jlitzinger> That begs the question, should an application ever expect a pad to have caps when the pad is added?
<__tim> well, it's not a hard requirement
<__tim> if you know the caps, you should make them available before adding the pad
<__tim> if those events get sent on the not-yet-added pad, that's fine, they will be stored in the pad as 'sticky events'
<__tim> and sent as soon as the pad gets linked
<jlitzinger> Looking at the multipartdemux code, it knows the caps, however, 1.4 change intentionally defers their transmission to avoid the "Sticky event misordering, got 'caps' before 'stream-start'" warning.
<stormer> __tim: jlitzinger: yes, sound like a bug/regression, some more code might have been needed
<stormer> jlitzinger: where does it fail ?
<__tim> jlitzinger, as I said, I don't think it was intentiontal to affect the way caps are handled, the point of the patch was to send a stream-start event before setting the caps
<__tim> jlitzinger, please just move the add_pad back to where it was (or I can do it if you like) :)
<jlitzinger> Heh, I did that, and that's what led to my second question.
<jlitzinger> If I add it back, the original warning message comes back
<stormer> jlitzinger: maybe I shouldn't have move where the pad is added ?
<jlitzinger> stormer: In the pad-added callback, the caps are NULL in 1.4 for multipartdemux
<__tim> jlitzinger, oh really? that's weird, could you open a bug then for starters, attachj your patch and then we'll investigate some more
<jlitzinger> working on it now, just wanted to test first
<stormer> __tim: I see the bug in the code, that would mean in general, we need to stick the stream-start, then caps, and then add the pad
<stormer> jlitzinger: are you setup to give it a try, remove gst_element_add_pad (GST_ELEMENT_CAST (demux), pad);, and put it back to it's original location
<stormer> ah wait
<stormer> __tim: hmm, I'm stuck I guess, gst_pad_create_stream_id() require me to be parented, and setting caps before setting stream-start throw a warning
<stormer> s/me/the pad
<jlitzinger> I did this
<jlitzinger> It works as previous, but I don't think it is what is desired
<jlitzinger> as you said, it seems like the events need to be added in the order: stream-start, caps
<jlitzinger> All I did was basically preserve the old order, and the old bug :)
<stormer> yes, that would undo the change ...
<stormer> slomo might know how to handle this
<jlitzinger> finishing the bug report..
<jlitzinger> I'll omit the patch
<stormer> if gst_pad_create_stream_id() didn't need a parented pad, it would all be easyer ...
<stormer> would simply have to stick the two events, and then add the pad ...
<stormer> thanks by the way
<jlitzinger> Should I copy/paste the relevant parts of this chat into the bug?
Created attachment 284748 [details] [review]
Proposed multipartdemux fix #1
Oops, my detailed comment was lost. If I understand the notion of parent correctly, the demux is the parent of the pad. This should allow the stream-start to be stored, and then the caps set, and then the pad added, preserving the old behavior.
Review of attachment 284748 [details] [review]:
Sound right to me, I was missing that store_sticky_event() method. Anyone seconding this change ?
Author: Jason Litzinger <email@example.com>
Date: Thu Aug 28 13:48:50 2014 -0600
multipartdemux: Ensure caps before pad added.
This stores the stream-start, sets caps, and then adds the pad,
which ensures that the caps are set for the "pad-added" callback.