GNOME Bugzilla – Bug 668847
[0.11] deinterleave port
Last modified: 2012-02-08 09:41:16 UTC
The interleave plugin needs porting. I started working on the deinterleave element, will hopefully post a patch soon.
Created attachment 206445 [details] [review] deinterleave: port to 0.11 Port of the deinterleave element and its unittests. The interleave element will be ported as part of another patch, hence disabling it for now.
Review of attachment 206445 [details] [review]: ::: gst/interleave/deinterleave.c @@ +74,3 @@ +#else +#define FORMATS "{ S8BE, S16BE, S24BE, S32BE, F32BE } " +#endif deinterleave does not care about the byte order, it accepts any. It also handles the 24bit-in-4-bytes formats and all unsigned formats and the 64 bit float format. @@ +217,3 @@ self->width = 0; self->func = NULL; + self->audio_info = gst_audio_info_new (); Store it directly in the instance struct and use gst_audio_info_init() @@ +251,2 @@ if (self->pos) + channel_mask |= G_GUINT64_CONSTANT (1) << self->pos[i]; this would probably be channel_mask = G_GUINT... There is only one channel per output stream. @@ +253,2 @@ else + channel_mask = GST_AUDIO_CHANNEL_POSITION_NONE; this would be channel_mask=0 @@ +293,2 @@ else + channel_mask = GST_AUDIO_CHANNEL_POSITION_NONE; Same comments as above @@ +297,3 @@ + gst_caps_set_simple (srccaps, "channel-mask", GST_TYPE_BITMASK, + channel_mask, NULL); + Also I think you should use a separate GstAudioInfo for every srcpad and create the caps from the audio info instead of the manual fiddling with the channel_mask and other things @@ +361,3 @@ + + self->width = GST_AUDIO_INFO_WIDTH (self->audio_info); + self->pos = self->audio_info->position; Just use self->audio_info for everything instead of duplicating the fields @@ +833,3 @@ static gboolean +gst_deinterleave_sink_activate_mode (GstPad * sinkpad, + GstObject * parent, GstPadMode mode, gboolean active) Instead of doing this all in the activate_mode function, just add a state change function and do it in READY->PAUSED and PAUSED->READY. ::: gst/interleave/deinterleave.h @@ +54,3 @@ gint channels; GstAudioChannelPosition *pos; + GstAudioInfo *audio_info; Store directly in the instance struct and don't store channels/pos/width separately again
Created attachment 206872 [details] [review] deinterleave: port to 0.11 Port of the deinterleave element and its unittests. The interleave element will be ported as part of another patch, hence disabling it for now.
Review of attachment 206872 [details] [review]: Looks good in general but: ::: gst/interleave/deinterleave.c @@ -73,3 @@ GST_PAD_SRC, GST_PAD_SOMETIMES, - GST_STATIC_CAPS ("audio/x-raw-int, " There's no format in the src/sink template caps anymore. Please add that again @@ +334,3 @@ + if (!gst_audio_info_from_caps (&self->audio_info, caps)) + goto no_channels; no_channels is wrong here, failing to parse the caps means that the caps are invalid in general
Created attachment 206875 [details] [review] deinterleave: port to 0.11 Port of the deinterleave element and its unittests. The interleave element will be ported as part of another patch, hence disabling it for now.
commit 640be49e216880d745bb4b514037cbec47943541 Author: Philippe Normand <philn@igalia.com> Date: Mon Jan 30 16:40:19 2012 +0100 deinterleave: port to 0.11 Port of the deinterleave element and its unittests. The interleave element will be ported as part of another patch, hence disabling it for now. https://bugzilla.gnome.org/show_bug.cgi?id=668847