[Bug 685249] add audio to tab-separated values encoder

[GStreamer (bugzilla.gnome.org)]

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

Sebastian Dröge <slomo> changed:

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

--- Comment #2 from Sebastian Dröge <slomo at circular-chaos.org> 2012-10-14 08:46:23 UTC ---
Review of attachment 225536:
 --> (https://bugzilla.gnome.org/review?bug=685249&attachment=225536)

* Could you run all this through gst-indent (gstreamer/tools)?
* Maybe the separator character could be made configurable via a property (and
be part of the caps then, defaulting to \t)
* Maybe add some docs there about how this output can be used with nxydump and
where nxydump is found

::: gst/debugutils/gsttsvenc.c
@@ +141,3 @@
+
+
+static int printsample_double(char *location, const void **sample)

All these printsample functions could be generalized with a macro that
generates the functions:
#define CREATE_PRINTFUNC(type, fmt) \
static int printsample_#name (char *location, const void ** sample) \
  return sprintf (location), "\t" fmt, (g#type) *(*(const g#type **)
sample)++); \
}

CREATE_PRINTFUNC(double, "%.16g");
CREATE_PRINTFUNC(uint32, "%u");

@@ +228,3 @@
+
+    for(channel = 0; channel < element->channels; channel++)
+      location += printsample(location, &samples);

Am I missing something or is this never incrementing samples, thus always
printing the same value (not incrementing for iterating over the channels and
also not incrementing for iterating the samples).

@@ +295,3 @@
+{
+  ARG_START_TIME = 1,
+  ARG_STOP_TIME

Rename to PROP_ from ARG_ please
Also shouldn't this be taken from the configured GstSegment?

@@ +327,3 @@
+
+    if(success)
+      *size = GST_AUDIO_INFO_WIDTH(&info) / 8 *
GST_AUDIO_INFO_CHANNELS(&info);

You can use BPF here

@@ +352,3 @@
+      caps =
+          gst_caps_copy(gst_pad_get_pad_template_caps
+          (GST_BASE_TRANSFORM_SINK_PAD(trans)));

You're leaking the caps here, get_pad_template_caps() already returns a new ref
in 1.0 (also no need to copy, returning a new ref is enough)

@@ +358,3 @@
+      caps =
+         
gst_caps_copy(gst_pad_get_pad_template_caps(GST_BASE_TRANSFORM_SRC_PAD
+              (trans)));

Same here

@@ +499,3 @@
+          && GST_BUFFER_OFFSET_END_IS_VALID(inbuf))) {
+    GST_ERROR_OBJECT(element,
+        "cannot compute number of input samples:  invalid offset and/or end
offset");

Don't depend on the offset to be set, just calculate the number of samples
yourself with the unit size

@@ +516,3 @@
+    stop =
+        timestamp_to_sample_clipped(GST_BUFFER_TIMESTAMP(inbuf), length,
+        element->rate, element->stop_time);

I don't think this is doing what you want, if I understand this correctly you
want to convert the timestamp to samples and clip to the [0, stop-start] and
offset by start. The function does not do that with these parameters

::: gst/debugutils/gsttsvenc.h
@@ +54,3 @@
+  gint rate;
+  gint channels;
+  gint unit_size;

Maybe use GstAudioInfo for this?

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