[MPlayer-dev-eng] [PATCH] Adding Radio support to MPlayer (update)

[Reimar Döffinger]

Hi,
On Fri, Jun 30, 2006 at 02:10:15PM +0700, Vladimir Voroshilov wrote:
>  
> +
> +#ifdef USE_RADIO
> +
> +#include "libmpdemux/stream_radio.h"

For consistency, IMHO move that next to the #include "libmpdemux/tv.h",
even though that means it is not under the #ifdef.

> Index: libmpdemux/demux_rawaudio.c
> ===================================================================
> --- libmpdemux/demux_rawaudio.c	(revision 18862)
> +++ libmpdemux/demux_rawaudio.c	(working copy)
> @@ -12,7 +12,6 @@
>  #include "demuxer.h"
>  #include "stheader.h"
>  
> -
>  extern int demuxer_type;
>  static int channels = 2;
>  static int samplerate = 44100;
> @@ -29,7 +28,6 @@
>    {NULL, NULL, 0, 0, 0, 0, NULL}
>  };
>  
> -
>  static demuxer_t* demux_rawaudio_open(demuxer_t* demuxer) {
>    sh_audio_t* sh_audio;
>    WAVEFORMATEX* w;

No real change here, just cosmetics.

> ===================================================================
> --- libmpdemux/Makefile	(revision 18862)
> +++ libmpdemux/Makefile	(working copy)
> @@ -40,6 +40,9 @@
>          stream_vcd.c \
>          stream_vstream.c \
>  
> +# Radio in
> +SRCS += stream_radio.c \
> +
>  # TV in
>  SRCS += tv.c \
>          frequencies.c \
> Index: libmpdemux/stream_radio.c
> ===================================================================
> --- libmpdemux/stream_radio.c	(revision 0)
> +++ libmpdemux/stream_radio.c	(revision 0)
> @@ -0,0 +1,776 @@
> +
> +#include "config.h"
> +
> +/*
> +    Radio support by Vladimir Voroshilov.
> +    
> +    Based on tv.c and tvi_v4l2.c code.
> +    
> +*/
> +#ifdef USE_RADIO

I personally prefered if the compilation was made conditional in the
Makefile, but on the other hand it's not done for the TV stuff either...

> +#include <pthread.h>

Forgot to have a look: Did your configure check make sure pthread is
available before enabling this feature?

> +    int		        audio_cnt;
> +    int                 audio_drop;

Please do not mix tabs and spaces (I personally prefer having no tabs at
all), there are some more places where this problem appears.


> +static int parse_channels(radio_priv_t* priv,float freq_channel,float* pfreq){

Please add doxygen comments to stuff, it makes it much easier to
understand later. At least for functions that are not static, doxygen
comments are an absolute must.
Didn't really check the remaining code in that file, I don't have a clue
about v4l anyway.

> Index: libmpdemux/stream_radio.h
> ===================================================================
> --- libmpdemux/stream_radio.h	(revision 0)
> +++ libmpdemux/stream_radio.h	(revision 0)
> @@ -0,0 +1,21 @@
> +#ifndef _H_STREAM_RADIO_
> +#define _H_STREAM_RADIO_
> +
> +#define RADIO_CHANNEL_LOWER 1
> +#define RADIO_CHANNEL_HIGHER 2
> +
> +
> +extern char *radio_param_device;
> +extern char **radio_param_channels;
> +extern int radio_param_volume;
> +char* radio_param_adevice;
> +int radio_param_arate;
> +int radio_param_achannels;
> +int radio_param_preload;

I really don't think these variable declarations without extern do what
you want...

Greetings,
Reimar Döffinger