Created attachment 370395 [details] [review]
csoundfilter patch

Review of attachment 370395 [details] [review]:

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?

(In reply to Sebastian Dröge (slomo) from comment #2)
> Review of attachment 370395 [details] [review] [review]:
> 
> This looks great, thanks :)

Thanks Sebastian for your comments. 

I followed your suggestions and modified the code according to that. I moved the fixate_caps code to transform_caps. I implemented the get_size  method and remove the prepare_output_buffer function, it is not necessary.

I did not known the g_strdup_vprintf() glib function, so now I use it, with better result.

Finally, modified the example, and remove the caps before and after of csoundfilter element because with the new implementation of transform_caps, csoundfilter figure out the caps to negotiate, according to defined by the user in the csd file

Created attachment 371122 [details] [review]
csoundfilter patch

Patch with several improvements suggested by Sebastian.

removed unnecessary methods like:

prepare_output_buffer
fixate_caps

Created attachment 371123 [details] [review]
csoundfilter patch

a little correction on test-csoundfilter.c example file , removed unused variables, all works fine

(In reply to Sebastian Dröge (slomo) from comment #2)
> Review of attachment 370395 [details] [review] [review]:


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


yeah, it leave us to remove unnecessary defines, and sizeof(float) is more intuitive


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

My first attempt was write a transform_caps and a fixed_caps, but follow you suggestion, we get a better caps negotiation and performance, joining both methods and adding a condition to verify if the csound instance has already created in the transform_caps method, removing the fixate_caps virtual method.


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

I have not known this virtual method. Thanks, better result using this. 
  
> @@ +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?

The filter will copy all data in chunks of a fixed size according to the csound's input buffer size, then will copy the data from the csound's output buffer, all the data in the same way

> @@ +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?

This function is used and assigned to a process function pointer (csoundfilter->process = gst_csoundfilter_trans;)

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

 I used available_fast based on the documentation in the GstAdapter class
"Gets the maximum number of bytes that are immediately available without requiring any expensive operations (like copying the data into a temporary buffer).",trying to avoid expensive ooperations. with the adapter I copy data from/to the csound buffers. This buffers are of a fixed size based on the settings in the csound file (*.csd), and copy or move data are expensive but, it is the only way to do it. because the csound API does not to leave us to pass a pointer where write the audio ouputs like another audio libraries like openal.

> @@ +499,3 @@
> +    va_list valist)
> +{
> +  char result[256];
> 
> Is is guaranteed that 256 is the maximum? Otherwise use g_strdup_vprintf()

thanks !!, works better and it is safe.

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

 with the transform_caps changed, the second caps and first caps are unnecessary, the pipeline figure out what are the best caps for both.

> see that sample rate and in/out channels at least is defined by the csd file?

the user will define the audio parameters inside of the csound file *.csd, So, they should be considerer during the caps negotiation. Bacause this file is load and compiled when the csound instance is created(in the start() method ) I separated the negotiation in two methods, transform_caps and fixated_caps.. But now, I followed your suggestion, removing the fixated_caps method, and make the negotiation in the fransform_caps, checking if the csound instance has alredy created for inmedialy considerer the user settings before the negotiation procedure finishes.

Created attachment 373116 [details] [review]
New patch

I made a patch using git format-patch master --stdout > filter.patch... I think it is better than using git diff.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/681.