[Bug 698927] LADSPA improved port to gstreamer 1.0

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=698927
  GStreamer | gst-plugins-bad | git

Stefan Sauer (gstreamer, gtkdoc dev) <ensonic> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #243063|none                        |reviewed
             status|                            |

--- Comment #15 from Stefan Sauer (gstreamer, gtkdoc dev) <ensonic at sonicpulse.de> 2013-05-03 07:46:39 UTC ---
Review of attachment 243063:
 --> (https://bugzilla.gnome.org/review?bug=698927&attachment=243063)

I think this is the last review and then we can commit it. Thanks for all the
work!
Please check all the functions in gstladspautils. If you only call them from
gstladspautils, please make those static and remove from the gstladspautils.h
to keep them private.

::: ext/ladspa/Makefile.am
@@ +3,3 @@
+libgstladspa_la_SOURCES = gstladspautils.c gstladspafilter.c gstladspasource.c
gstladspasink.c gstladspa.c
+libgstladspa_la_CFLAGS = -I$(top_srcdir)/gst-libs $(GST_PLUGINS_BASE_CFLAGS)
$(GST_BASE_CFLAGS) $(GST_CFLAGS) $(LRDF_CFLAGS) $(GST_PLUGINS_BAD_CFLAGS)
+libgstladspa_la_LIBADD = $(GST_PLUGINS_BASE_LIBS)
-lgstaudio-$(GST_API_VERSION) $(GST_BASE_LIBS) $(GST_LIBS) $(LIBM) $(LRDF_LIBS)
$(GST_LIBS) 

Please rewrap CFLAGS and LIBADD to avoid super long lines.

::: ext/ladspa/gstladspautils.c
@@ +73,3 @@
+    for (j = 0; j < samples; j++)
+      outdata[i * samples + j] =
+          ((LADSPA_Data *) indata)[j * ladspa->klass->count.audio.in + i];

I would move ladspa->klass->count.audio.in and ladspa->klass->count.audio.in
into two local vars. This allows the compiler to know that they won't change
during the nested loops. Also in the next function.

@@ +125,3 @@
+
+/*
+ * Do it.

:) Maybe write "Process a block of audio with the ladspa plugin".

@@ +142,3 @@
+  LADSPA_Data *in, *out;
+
+  //GST_DEBUG ("LADSPA transform for %i samples, %i->%i channels", samples,
ladspa->klass->count.audio.in, ladspa->klass->count.audio.out);

no // comments. Either uncomment or remove. You can use GST_LOG to lower the
prio.

@@ +337,3 @@
+}
+
+GParamSpec *

If you only call this from here make it static and make sure there is no
prototype in the .h file.

@@ +553,3 @@
+    longname = g_strdup ("no LADSPA description available");
+
+  //FIXME: no plugin author field different from element author field

please use /* */ style comments

@@ +836,3 @@
+}
+
+void

static ?

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