After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 668847 - [0.11] deinterleave port
[0.11] deinterleave port
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
0.11.x
Other Linux
: Normal normal
: 0.11.x
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-01-27 15:10 UTC by Philippe Normand
Modified: 2012-02-08 09:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
deinterleave: port to 0.11 (41.74 KB, patch)
2012-01-30 15:46 UTC, Philippe Normand
needs-work Details | Review
deinterleave: port to 0.11 (41.63 KB, patch)
2012-02-06 10:56 UTC, Philippe Normand
needs-work Details | Review
deinterleave: port to 0.11 (41.76 KB, patch)
2012-02-06 11:57 UTC, Philippe Normand
committed Details | Review

Description Philippe Normand 2012-01-27 15:10:23 UTC
The interleave plugin needs porting. I started working on the deinterleave element, will hopefully post a patch soon.
Comment 1 Philippe Normand 2012-01-30 15:46:34 UTC
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.
Comment 2 Sebastian Dröge (slomo) 2012-02-01 09:09:38 UTC
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
Comment 3 Philippe Normand 2012-02-06 10:56:04 UTC
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.
Comment 4 Sebastian Dröge (slomo) 2012-02-06 11:29:42 UTC
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
Comment 5 Philippe Normand 2012-02-06 11:57:38 UTC
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.
Comment 6 Sebastian Dröge (slomo) 2012-02-08 09:41:10 UTC
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