[Bug 668847] [0.11] deinterleave port

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=668847
  GStreamer | gst-plugins-good | 0.11.x

Sebastian Dröge <slomo> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #206445|none                        |needs-work
             status|                            |

--- Comment #2 from Sebastian Dröge <slomo at circular-chaos.org> 2012-02-01 09:09:38 UTC ---
Review of attachment 206445:
 --> (https://bugzilla.gnome.org/review?bug=668847&attachment=206445)

::: 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

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.