GNOME Bugzilla – Bug 705993
aiffparse: fix sticky event warnings in push mode / add tests
Last modified: 2013-08-16 23:30:14 UTC
Using the aiff demuxer in push mode results in the following error: Unexpected critical/warning: gstpad.c:4506:store_sticky_event:<aiffparse0:src> Sticky event misordering, got 'segment' before 'caps' gstcheck.c:75:F:general:test_push:0: Unexpected critical/warning: gstpad.c:4506:store_sticky_event:<aiffparse0:src> Sticky event misordering, got 'segment' before 'caps As far as I understand it, a segment is received from upstream and passed through the demuxer before the header parsing has began (where the caps are declared). First patch fix this issue by handling sink events: CAPS and SEGMENT. Second patch a test for the aiff demuxer (both push and pull mode).
Created attachment 251614 [details] [review] aiffparse: fix push mode
Created attachment 251615 [details] [review] aiffparse: add tests
Review of attachment 251614 [details] [review]: ::: gst/aiff/aiffparse.c @@ +1713,3 @@ + /* some debug output */ + gst_event_copy_segment (event, &segment); + GST_DEBUG_OBJECT (aiff, "received newsegment %" GST_SEGMENT_FORMAT, It's not newsegment, it's segment :) Same in a few other places in this patch @@ +1717,3 @@ + + if (aiff->state != AIFF_PARSE_DATA) { + GST_DEBUG_OBJECT (aiff, "still starting, eating event"); The demuxer should still take into account the upstream segment when generating its initial segment event if upstream is in TIME format at least @@ +1776,3 @@ + gst_event_unref (aiff->start_segment); + GST_DEBUG_OBJECT (aiff, "Storing newseg %" GST_SEGMENT_FORMAT, &segment); + aiff->start_segment = gst_event_new_segment (&segment); You can just push the segment event here, if you get a segment event from upstream the demuxer is running in push mode and the event is received from the streaming thread
Review of attachment 251615 [details] [review]: ::: tests/check/elements/aiffparse.c @@ +54,3 @@ + NULL); + + fail_unless (gst_caps_is_always_compatible (caps, tcaps)); gst_caps_can_intersect() should be enough here :) @@ +147,3 @@ + GstBuffer ** buffer) +{ + if (offset + length > sizeof (aiff_file)) This could still be a short read, i.e. if offset < sizeof(aiff_file) ::: tests/check/elements/aiffparse.h @@ +2,3 @@ +#include <glib.h> + +static const guint8 aiff_file[] = { Maybe put this into a separate binary file instead of a large header?
Created attachment 251741 [details] [review] aiffparse: fix push mode
Created attachment 251742 [details] [review] aiffparse: add tests
Created attachment 251743 [details] [review] aiffparse: fix typo /newsegment/segment/
(In reply to comment #3) > Review of attachment 251614 [details] [review]: > > ::: gst/aiff/aiffparse.c > @@ +1713,3 @@ > + /* some debug output */ > + gst_event_copy_segment (event, &segment); > + GST_DEBUG_OBJECT (aiff, "received newsegment %" GST_SEGMENT_FORMAT, > > It's not newsegment, it's segment :) Same in a few other places in this patch Fixed in new patch. > > @@ +1717,3 @@ > + > + if (aiff->state != AIFF_PARSE_DATA) { > + GST_DEBUG_OBJECT (aiff, "still starting, eating event"); > > The demuxer should still take into account the upstream segment when generating > its initial segment event if upstream is in TIME format at least Fixed in new patch. > > @@ +1776,3 @@ > + gst_event_unref (aiff->start_segment); > + GST_DEBUG_OBJECT (aiff, "Storing newseg %" GST_SEGMENT_FORMAT, > &segment); > + aiff->start_segment = gst_event_new_segment (&segment); > > You can just push the segment event here, if you get a segment event from > upstream the demuxer is running in push mode and the event is received from the > streaming thread start_segment is automatically pushed in the parse_stream_data function which is called after the headers parsing function if aiff->state != AIFF_PARSE_DATA.
(In reply to comment #4) > Review of attachment 251615 [details] [review]: > > ::: tests/check/elements/aiffparse.c > @@ +54,3 @@ > + NULL); > + > + fail_unless (gst_caps_is_always_compatible (caps, tcaps)); > > gst_caps_can_intersect() should be enough here :) Fixed in new patch. > > @@ +147,3 @@ > + GstBuffer ** buffer) > +{ > + if (offset + length > sizeof (aiff_file)) > > This could still be a short read, i.e. if offset < sizeof(aiff_file) Fixed in new patch. > > ::: tests/check/elements/aiffparse.h > @@ +2,3 @@ > +#include <glib.h> > + > +static const guint8 aiff_file[] = { > > Maybe put this into a separate binary file instead of a large header? It felt more convenient to place into a large header. Anyway, should the file be placed under the tests/files/ directory ?
Could you squash the fix-up patch into the orginal one and update? > Anyway, should the file be placed under the tests/files/ directory ? Yes, please. And please try to make it as small as possible (couple of kB).
PS: _and _no _leading _underscores _for _internal _symbols _please :)
(In reply to comment #10) > Could you squash the fix-up patch into the orginal one and update? > > > Anyway, should the file be placed under the tests/files/ directory ? > > Yes, please. And please try to make it as small as possible (couple of kB). The fix-up patch is correcting typos in places non-related to the original patch. Should I squash it anyway ?
> The fix-up patch is correcting typos in places non-related to the original > patch. Should I squash it anyway ? Nah, it's fine, I only saw that afterwards. (They're not really "typos" btw, in 0.10 the event was called "new segment" event.)
Comment on attachment 251742 [details] [review] aiffparse: add tests Please put the file into a separate binary file instead of a extremely large header file
Created attachment 251833 [details] [review] aiffparse: add tests
(In reply to comment #14) > (From update of attachment 251742 [details] [review]) > Please put the file into a separate binary file instead of a extremely large > header file New patch attached. I used g_file_get_contents exclusively for convenience in order to compare input and output.
Thanks for the patches! Pushed them + some fixes to master: commit ef8557249623dd2a1ea5652e207347ca071d019b Author: Tim-Philipp Müller <tim@centricular.net> Date: Sat Aug 17 00:22:44 2013 +0100 tests: fix some leaks in aiffparse unit test commit 1d35549d6070b3aae5dd079831c82c6b4777bead Author: Tim-Philipp Müller <tim@centricular.net> Date: Sat Aug 17 00:09:18 2013 +0100 tests: fix state change order in aiffparse test Do state changes from sink to src. Fixes race condition in pull mode test where the source will start up and push buffers to queue/identity or aiffparse before the main thread has managed to set them to playing yet. commit 2bed61ee2ff5f1bb7c8ce068ae3dd992e074ffea Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Mon Aug 12 18:33:39 2013 +0100 aiffparse: add tests https://bugzilla.gnome.org/show_bug.cgi?id=705993 commit 63d629aba560512a97e36699be16f840f7603b97 Author: Tim-Philipp Müller <tim@centricular.net> Date: Sat Aug 17 00:23:08 2013 +0100 aiffparse: don't leak adapter commit ddcfe3ddf38660280cbd5d73a32bba5128a70685 Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Thu Aug 15 13:58:48 2013 +0100 aiffparse: s/newsegment/segment/ https://bugzilla.gnome.org/show_bug.cgi?id=705993 commit d69b6e53e463bc96b7457c3a3f99f1d07c891610 Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Tue Aug 13 18:42:55 2013 +0100 aiffparse: fix push mode Fix push mode by handling sink events (CAPS, SEGMENT) properly. https://bugzilla.gnome.org/show_bug.cgi?id=705993