GNOME Bugzilla – Bug 650313
ac3parse: Add support for iec61937 alignment
Last modified: 2011-07-27 15:59:15 UTC
Attaching a couple of patches that allow us to configure ac3parse to send buffers in units that are directly usable for iec61937 payloading (namely 6 audio blocks per substream). The first patch merely accepts configuring in caps at runtime, and the second adds the ability for downstream elements to request reconfiguration.
Created attachment 187911 [details] [review] ac3parse: Add support for IEC 61937 alignment When pushing out buffers over S/PDIF or HDMI, IEC 61937 payloading requires each buffer to contain 6 blocks from each substream. This adds code to collect all the frames needed to meet this requirement before pushing out a buffer.
Created attachment 187912 [details] [review] ac3parse: Support switching alignment on-the-fly This allows switching of alignment for E-AC3 streams at run-time. This is requested by downstream elements via a custom event.
Comment on attachment 187911 [details] [review] ac3parse: Add support for IEC 61937 alignment Looks good
Review of attachment 187912 [details] [review]: ::: gst/audioparsers/gstac3parse.c @@ +202,3 @@ + + klass->baseparse_src_event = parse_class->src_event; + parse_class->src_event = GST_DEBUG_FUNCPTR (gst_ac3_parse_src_event); Just override ->src_event and then chain up in the event function via GST_BASE_PARSE_CLASS(parent_class)->src_event(bparse, event); @@ +597,3 @@ gst_caps_set_simple (caps, "alignment", G_TYPE_STRING, + g_atomic_int_get (&ac3parse->align) == GST_AC3_PARSE_ALIGN_IEC61937 ? + "iec61937" : "frame", NULL); You should check if the alignment has changed here and only create new caps if the caps actually changed, otherwise just set GST_PAD_CAPS() on the buffers ::: gst/audioparsers/gstac3parse.h @@ +72,3 @@ struct _GstAc3ParseClass { GstBaseParseClass baseparse_class; + gboolean (*baseparse_src_event) (GstBaseParse * parse, GstEvent * event); Not needed
Thank you for the reviews, Sebastian! (and sorry for taking so long to get back) (In reply to comment #4) > Review of attachment 187912 [details] [review]: > > ::: gst/audioparsers/gstac3parse.c > @@ +202,3 @@ > + > + klass->baseparse_src_event = parse_class->src_event; > + parse_class->src_event = GST_DEBUG_FUNCPTR (gst_ac3_parse_src_event); > > Just override ->src_event and then chain up in the event function via > GST_BASE_PARSE_CLASS(parent_class)->src_event(bparse, event); Ack. > @@ +597,3 @@ > gst_caps_set_simple (caps, "alignment", G_TYPE_STRING, > + g_atomic_int_get (&ac3parse->align) == GST_AC3_PARSE_ALIGN_IEC61937 ? > + "iec61937" : "frame", NULL); > > You should check if the alignment has changed here and only create new caps if > the caps actually changed, otherwise just set GST_PAD_CAPS() on the buffers We're actually only setting all this if some other parameters changed, so new caps would be needed anyway. Or are you saying that the way it was originally done should be fixed (which makes more sense, I suppose). > ::: gst/audioparsers/gstac3parse.h > @@ +72,3 @@ > struct _GstAc3ParseClass { > GstBaseParseClass baseparse_class; > + gboolean (*baseparse_src_event) (GstBaseParse * parse, GstEvent * event); > > Not needed Ack.
Committed after making changes based on review -- thanks!
commit 89564fcb69b481050f8064f0ba232a86cd9c5f51 Author: Arun Raghavan <arun.raghavan@collabora.co.uk> Date: Tue Apr 12 17:01:47 2011 +0530 ac3parse: Support switching alignment on-the-fly This allows switching of alignment for E-AC3 streams at run-time. This is requested by downstream elements via a custom event. https://bugzilla.gnome.org/show_bug.cgi?id=650313 commit 96972eb462cc672d73c82cb2312f2116df1ced7c Author: Arun Raghavan <arun.raghavan@collabora.co.uk> Date: Sat Apr 9 12:26:56 2011 +0530 ac3parse: Add support for IEC 61937 alignment When pushing out buffers over S/PDIF or HDMI, IEC 61937 payloading requires each buffer to contain 6 blocks from each substream. This adds code to collect all the frames needed to meet this requirement before pushing out a buffer. https://bugzilla.gnome.org/show_bug.cgi?id=650313