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: 9966fdfa756ff2ef5c5dcf541cb9b9dfe3b3d878
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> 9966fdfa756ff2ef5c5dcf541cb9b9dfe3b3d878 <jlitzinger> ok <jlitzinger> I'll log a bug, and add a proposed patch to it <jlitzinger> thanks! <__tim> thanks <__tim> yup <__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> yep <__tim> thanks <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> http://pastebin.com/hY6U3AqH <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 ... <jlitzinger> yep <stormer> slomo might know how to handle this <stormer> hmm <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 ... <jlitzinger> https://bugzilla.gnome.org/show_bug.cgi?id=735626 <stormer> thanks by the way <jlitzinger> np <jlitzinger> Should I copy/paste the relevant parts of this chat into the bug? <__tim> sure
Created attachment 284748 [details] [review] Proposed multipartdemux fix #1 Proposed fix.
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 ?
commit bcbdcbf638f657c10c5fe1baebff072766fd22a5 Author: Jason Litzinger <jlitzinger@control4.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. https://bugzilla.gnome.org/show_bug.cgi?id=735626