[Bug 661137] Add support for Commodore tapes

[GStreamer (bugzilla.gnome.org)]

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

Sebastian Dröge (slomo) <slomo> changed:

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

--- Comment #5 from Sebastian Dröge (slomo) <slomo at circular-chaos.org> 2013-10-07 13:37:11 UTC ---
Review of attachment 255721:
 --> (https://bugzilla.gnome.org/review?bug=661137&attachment=255721)

The comments for the decoder almost all apply to the encoder too. Also run all
the .c files through gst-indent :)

::: configure.ac
@@ +2040,3 @@
+    AC_CHECK_HEADER(tapencoder.h,
+      [
+        AC_CHECK_LIB(tapencoder,tapenc_get_pulse,HAVE_TAPENC=yes)

They don't provide pkg-config files for this?

Anyway, please put both elements into the same plugin :)

::: ext/tapdec/gsttapdec.c
@@ +2,3 @@
+ * GStreamer
+ * Copyright (C) 2005 Thomas Vander Stichele <thomas at apestaart.org>
+ * Copyright (C) 2005 Ronald S. Bultje <rbultje at ronald.bitfreak.net>

Those two never touched this element, so probably remove them :)

@@ +85,3 @@
+struct _GstTapDec
+{
+  GstElement element;

Please let this use GstAudioDecoder as base class

@@ +123,3 @@
+    GST_PAD_ALWAYS,
+    GST_STATIC_CAPS ("audio/x-raw, "
+      "format=S32LE, "

I assume this is little endian on little endian systems, and big endian on big
endian? In that case you should use the GST_AUDIO_NE macro

@@ +229,3 @@
+          TRUE, G_PARAM_READWRITE);
+  obj_properties[PROP_WAVEFORM] =
+      g_param_spec_uint ("waveform", "Waveform", "0=square, 1=triangle, 2=sine
(if tapdecoder does not support sine, will fall back to square)", 0, 2,

Make this a GEnum and use g_param_spec_enum() here

@@ +230,3 @@
+  obj_properties[PROP_WAVEFORM] =
+      g_param_spec_uint ("waveform", "Waveform", "0=square, 1=triangle, 2=sine
(if tapdecoder does not support sine, will fall back to square)", 0, 2,
+          0, G_PARAM_READWRITE);

| G_PARAM_STATIC_STRING everywhere

@@ +379,3 @@
+    g_value_init (&default_value, G_PARAM_SPEC_VALUE_TYPE(obj_properties[i]));
+    g_param_value_set_default (obj_properties[i], &default_value);
+    g_object_set_property (G_OBJECT(filter), g_param_spec_get_name
(obj_properties[i]), &default_value);

Instead of this you could also just mark the properties as CONSTRUCT

@@ +404,3 @@
+    "LGPL",
+    PACKAGE,
+    "http://wav-prg.sourceforge.net/"

Please use the values here as in other plugins

::: ext/tapenc/gsttapenc.c
@@ +86,3 @@
+struct _GstTapEnc
+{
+  GstElement element;

Have this use GstAudioEncoder as base class

::: gst/tap/gstdmpdec.c
@@ +193,3 @@
+    srccaps = gst_caps_copy (gst_pad_template_get_caps
(gst_pad_get_pad_template (filter->srcpad)));
+    srcstructure = gst_caps_get_structure (srccaps, 0);
+    gst_structure_set_value (srcstructure, "rate", &rate);

Just use GstAudioInfo and gst_audio_info_to_caps() here :)

@@ +233,3 @@
+      out_pulse += inpulse;
+      if (inpulse > filter->overflow)
+        ret = GST_FLOW_ERROR;

Just error out immediately here? Then you don't need ret2 :)

@@ +244,3 @@
+    gst_adapter_unmap (filter->adapter);
+    gst_buffer_resize(newbuf, 0, pos_outpulse * sizeof(guint));
+    ret2 = gst_pad_push (filter->srcpad, newbuf);

Properly set timestamp and duration on the buffer, based on what you get as
input.

@@ +271,3 @@
+  gst_element_add_pad (GST_ELEMENT (filter), filter->srcpad);
+
+  filter->adapter = gst_adapter_new ();

You need to free the adapter in GstObject::finalize. Also you need to implement
GstElement::change_state to properly reset when going from PAUSED to READY so
the element can be re-used. Also you should implement a event handler on the
sinkpad to handle flush events properly, and also the segment event.

::: gst/tap/gsttapconvert.c
@@ +150,3 @@
+{
+  GstTapConvert *filter = GST_TAP_CONVERT (base);
+  guint buflen = gst_buffer_get_size (outbuf) / sizeof(guint);

Use the size of the GstMapInfo below, also don't use guint. But use a properly
sized type (e.g. guint32 if you want 32 bit values).

@@ +155,3 @@
+  GstMapInfo map;
+
+  gst_buffer_map (outbuf, &map, GST_MAP_READ);

mapping can fail

::: gst/tap/gsttapfiledec.c
@@ +118,3 @@
+    GST_PAD_ALWAYS,
+    GST_STATIC_CAPS ("audio/x-tap, "
+      "rate = (int) { 110840, 111860, 123156, 127840, 138550 }")

You don't use these values anywhere else, are they useful?

@@ +319,3 @@
+  gst_element_add_pad (GST_ELEMENT (filter), filter->srcpad);
+
+  filter->adapter = gst_adapter_new ();

Same as for the dmpdec element, including the handling of events and
GstElement::change_state.

::: gst/tap/gsttapfileenc.c
@@ +358,3 @@
+    gst_segment_init(&segment, GST_FORMAT_BYTES);
+    gst_pad_push_event (filter->srcpad,
+      gst_event_new_segment (&segment));

You should check if it worked, and also initially check if you can seek at all.
See the seeking query, some muxers are using that

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