[Bug 794852] New Audio Element based on Csound

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=794852

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

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

--- Comment #2 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 370395:
 --> (https://bugzilla.gnome.org/review?bug=794852&attachment=370395)

This looks great, thanks :)

::: ext/csound/gstcsoundfilter.c
@@ +160,3 @@
+      g_param_spec_string ("location", "Location",
+          "Location of the csd file used by csound", NULL,
+          G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

GST_PARAM_MUTABLE_READY for this one also

@@ +165,3 @@
+      g_param_spec_boolean ("loop", "Loop",
+          "do a loop on the score", DEFAULT_LOOP,
+          G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

GST_PARAM_MUTABLE_PLAYING

@@ +293,3 @@
+
+static GstCaps *
+gst_csoundfilter_transform_caps (GstBaseTransform * base,

In here also the in/out channels and sample rate of the csd file should be
considered, if loaded already. Loading the file in start() should be sufficient
to make negotiation work fine

@@ +330,3 @@
+          NULL);
+    }
+  } else if (csoundGetSizeOfMYFLT () == FLOAT_SAMPLES) {

Why not just use sizeof(float) here?

@@ +379,3 @@
+  } else if (direction == GST_PAD_SINK) {
+    gst_structure_set (structure, "channels", G_TYPE_INT,
+        csoundfilter->cs_ochannels, NULL);

These should all be fixed already if you move this to the transform_caps
function, and by doing it there negotiation would work correctly and you can
remove the fixate function

@@ +432,3 @@
+        gst_buffer_get_size (inbuf) / (csoundfilter->cs_ichannels *
+        sizeof (MYFLT));
+    new_size = num_samples * sizeof (MYFLT) * csoundfilter->cs_ochannels;

This could be done automatically if you implement the get_unit_size virtual
method. Then the allocation code of the base class should already figure out
how much to allocate

@@ +445,3 @@
+  }
+
+  *outbuf = gst_buffer_make_writable (*outbuf);

This should be unneeded, you just allocated the buffer above

@@ +465,3 @@
+  csoundfilter->process (csoundfilter, (MYFLT *) omap.data,
+      csoundfilter->in_bytes, csoundfilter->out_bytes);
+  gst_buffer_unmap (outbuf, &omap);

Will it always fill the complete output buffer or would we have to check here
how much it wrote?

@@ +480,3 @@
+static void
+gst_csoundfilter_trans (GstCsoundfilter * csoundfilter,
+    MYFLT * odata, guint in_bytes, guint out_bytes)

This function does not seem to be used anywhere?

@@ +483,3 @@
+{
+  gsize offset = 0;
+  while (gst_adapter_available_fast (csoundfilter->in_adapter) >=

Why fast? This might fail, without fast would be better

@@ +499,3 @@
+    va_list valist)
+{
+  char result[256];

Is is guaranteed that 256 is the maximum? Otherwise use g_strdup_vprintf()

::: tests/examples/csound/test-csoundfilter.c
@@ +82,3 @@
+  sink = gst_element_factory_make ("autoaudiosink", NULL);
+  caps = gst_caps_from_string ("audio/x-raw,channels=1,rate=44100");
+  caps2 = gst_caps_from_string ("audio/x-raw,channels=2,rate=44100");

The second caps filter here seems unneeded. audioconvert should figure out
something that works together with autoaudiosink

The first caps filter might be unneeded. Why exactly is it needed? csoundfilter
should also figure out something that works between audiotestsrc, itself and
downstream automatically already. From what I can see that sample rate and
in/out channels at least is defined by the csd file?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.