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 735626 - multipartdemux: caps are NULL in pad-added callback (regression)
multipartdemux: caps are NULL in pad-added callback (regression)
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.4.0
Other Linux
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-08-28 18:28 UTC by Jason Litzinger
Modified: 2014-08-29 09:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed multipartdemux fix #1 (1.65 KB, patch)
2014-08-28 19:59 UTC, Jason Litzinger
committed Details | Review

Description Jason Litzinger 2014-08-28 18:28:34 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
Comment 1 Nicolas Dufresne (ndufresne) 2014-08-28 18:30:18 UTC
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 ...
Comment 2 Jason Litzinger 2014-08-28 18:32:21 UTC
<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
Comment 3 Jason Litzinger 2014-08-28 19:59:42 UTC
Created attachment 284748 [details] [review]
Proposed multipartdemux fix #1

Proposed fix.
Comment 4 Jason Litzinger 2014-08-28 20:00:45 UTC
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.
Comment 5 Nicolas Dufresne (ndufresne) 2014-08-28 20:18:47 UTC
Review of attachment 284748 [details] [review]:

Sound right to me, I was missing that store_sticky_event() method. Anyone seconding this change ?
Comment 6 Sebastian Dröge (slomo) 2014-08-29 08:38:35 UTC
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