[gst-devel] Re: [gst-cvs] zeenix gst-plugins-bad: gst-plugins-bad/ gst-plugins-bad/ext/gsm/

[Andy Wingo]

Hi Zeeshan and Philippe,

A few questions about this one:

On Tue, 2005-10-25 at 09:12 -0700, Zeeshan Ali wrote:
> Over-writing Wim's gsm plugins (currently not working) with that from the farsight repo. Also made sure that they work with the RTP (de)payloader.
> --- gstgsm.c	23 Sep 2005 17:05:29 -0000	1.13
> +++ gstgsm.c	25 Oct 2005 16:12:03 -0000	1.14
> @@ -1,5 +1,7 @@
> -/* GStreamer
> - * Copyright (C) <1999> Erik Walthinsen <omega at cse.ogi.edu>
> +/*
> + * Farsight
> + * GStreamer GSM encoder/decoder (uses WAV49 compiled libgsm)
> + * Copyright (C) 2005 Philippe Khalaf <burger at speedy.org>

These files are clearly taken from gstreamer, modified, and then
returned. Thus you should not remove copyright lines. Also now that they
are in GStreamer, they should say GStreamer at the top.

>  GST_PLUGIN_DEFINE (GST_VERSION_MAJOR,
>      GST_VERSION_MINOR,
>      "gsm",
> -    "GSM Elements Plugin",
> -    plugin_init, VERSION, "LGPL", GST_PACKAGE, GST_ORIGIN)
> +    "GSM encoder/decoder",
> +    plugin_init, VERSION, "LGPL", "Farsight", "http://farsight.sf.net")

Likewise...

>  GstElementDetails gst_gsmdec_details = {
>    "GSM audio decoder",
>    "Codec/Decoder/Audio",
>    "Decodes GSM encoded audio",
> -  "Wim Taymans <wim.taymans at chello.be>",
> +  "Philippe Khalaf <burger at speedy.org>",

Normally we just add on names

>  static GstFlowReturn
>  gst_gsmdec_chain (GstPad * pad, GstBuffer * buf)
>    GstGSMDec *gsmdec;
> +  gsm_byte *data;
> +  g_return_val_if_fail (GST_IS_PAD (pad), GST_FLOW_ERROR);

this is a redundant check

>    gsmdec = GST_GSMDEC (gst_pad_get_parent (pad));

this will return a ref to gsmdec that you never drop

> +  g_return_val_if_fail (GST_IS_GSMDEC (gsmdec), GST_FLOW_ERROR);

this a bit redundant as well

> -  gsm_byte *data = (gsm_byte *) GST_BUFFER_DATA (buf);
> -  guint size = GST_BUFFER_SIZE (buf);
> +  g_return_val_if_fail (GST_PAD_IS_LINKED (gsmdec->srcpad), GST_FLOW_ERROR);

this is bogus, just check the return val of pad_push

>      gst_buffer_set_caps (outbuf, gst_pad_get_caps (gsmdec->srcpad));

leaks the return of get_caps()

> +    gst_pad_push (gsmdec->srcpad, outbuf);

ALWAYS check the return value of pad_push, and return it to the caller

>    return GST_FLOW_OK;

should be returning it here


Similar comments for gsmenc.

Please fix.

Regards,
-- 
Andy Wingo
http://wingolog.org/