GNOME Bugzilla – Bug 766970
aacparse: sticky event criticals with RTL HD mpeg-ts stream
Last modified: 2016-07-25 10:26:45 UTC
When playing Teleboy.ch-streams with gstreamer on enigma2, sometimes audio isn't played. Log shows: (enigma2:647): GStreamer-WARNING **: ../../git/gst/gstpad.c:5066:store_sticky_event:<aacparse23:src> Sticky event misordering, got 'segment' before 'caps' (enigma2:647): GStreamer-WARNING **: ../../git/gst/gstpad.c:5066:store_sticky_event:<'':decodepad111> Sticky event misordering, got 'segment' before 'caps' (enigma2:647): GStreamer-WARNING **: ../../git/gst/gstpad.c:5066:store_sticky_event:<src_1:proxypad370> Sticky event misordering, got 'segment' before 'caps' Example (recorded) file: http://www91.zippyshare.com/v/0UZHa7tY/file.html
Thanks for the bug report. I can confirm I get sticky event criticals and gst_structure_fixate_field_nearest_int criticals, but audio plays fine for me on the desktop with avdec_aac.
Created attachment 330665 [details] [review] keep event ordering
I got those. I fixed the fixate ones first, but I can't find the patch for some reason, and I'm not getting them anymore, so it might have got fixed upstream since I updated yesterday. The patch is for the event ordering. It's a bit iffy as delaying the segment till we know caps causes a gap to cause a clock wait in the sink, as the segment in this file starts from non-zero, so we have to delay that too. So I delayed everything but stream-start, because reasons. Delaying gap and segment works too, but seems a wee bit more iffy to me.
Comment on attachment 330665 [details] [review] keep event ordering If anything, this should be in baseparse. The problem here is that we have no CAPS event from upstream and invent our own caps based on buffers (and had a SEGMENT event before the buffers of course)? Shouldn't baseparse just delay all post-CAPS events (that it forwards) until caps are set (would need a new gst_base_parse_set_caps()) or a frame is finished?
Created attachment 330705 [details] [review] push caps first
It turns out to be even simpler, since baseparse already does event delaying.
Review of attachment 330705 [details] [review]: Erm that doesn't seem right. Why would you always push upstream caps downstream here? The subclass is responsible for creating caps, and usually they are not the same as the upstream ones. If the problem is that post-caps events are sent before the subclass has created the caps event, then the solution would be to delay the post-caps events until finish_frame() or a new gst_base_parse_set_caps().
I just ensure the order of push is caps first in what's already being pushed.
Caps should never ever be in that delayed events list as they are created by the subclass and just pushed downstream.
Ah, I see. They're pushed by default caps genrated when it's getting GAP event. So I guess that too has to be delayed, then. That delaying sounds like a huge hack in the first place tbh.
It's not a hack but required :) Why would the default caps generated by the GAP event need to be delayed? At the moment you have a GAP event, you can immediately send CAPS. And all delayed events (post-CAPS events) can then be sent.
I meant delaying the gap event. Since aacparse wants to nose about in the data to create caps, a gap event isn't enough for that. If the gap is not delayed, and if the segment (which must come after caps) would start at > 0, the sink will wait from 0 to the gap timestamp, as it doesn't yet know the real segment won't start at 0. So we miss that amount of audio.
The point of the default caps with the GAP event is that it should be as complete as possible and immediately go downstream. It's a fallback for the case where we don't have any actual data yet but need to produce caps and preroll. You need to send the SEGMENT event (which must come before GAP) after the default caps and before the GAP event in the default caps handling. Basically you must forward all delayed events (post-CAPS events) after the default caps were sent and before forwarding the GAP.
Created attachment 330828 [details] [review] baseparse: Make sure to not create an invalid event order when generating the default CAPS event because of a GAP event There must be a SEGMENT event before the GAP event, and SEGMENT events must come after any CAPS event. We however did not produce any CAPS yet, so we need to ensure to insert the CAPS event before the SEGMENT event into the pending events list.
Can someone who has the file in question check if this solves all the problems?
(In reply to Sebastian Dröge (slomo) from comment #15) > Can someone who has the file in question check if this solves all the > problems? It does :)
Created attachment 330829 [details] [review] fix for the fixate criticals And this is for the fixate criticals too.
Attachment 330828 [details] pushed as 8e8b8a8 - baseparse: Make sure to not create an invalid event order when generating the default CAPS event because of a GAP event
Review of attachment 330829 [details] [review]: ::: gst-libs/gst/audio/gstaudiodecoder.c @@ +2018,3 @@ + if (gst_structure_has_field (structure, "rate")) + gst_structure_fixate_field_nearest_int (structure, + "rate", GST_AUDIO_DEF_RATE); It will have to invent these fields if they are not there, i.e. if (has) fixate else set
Created attachment 330835 [details] [review] fix for the fixate criticals
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Jul 4 09:15:03 2016 +0100 audiodecoder: fix criticals fixating a non existent field https://bugzilla.gnome.org/show_bug.cgi?id=766970
commit e18a9d9b5fb535e52310189b794f787533c2880b Author: Sebastian Dröge <sebastian@centricular.com> Date: Mon Jul 4 11:16:55 2016 +0200 videodecoder: fix criticals fixating a non existent field https://bugzilla.gnome.org/show_bug.cgi?id=766970