[MPlayer-dev-eng] [PATCH] Radio support for MPlayer

[Reimar Doeffinger]

Hello,
On Mon, Jul 10, 2006 at 01:12:00AM +0700, Vladimir Voroshilov wrote:
> +#ifdef USE_RADIO
> +    case MP_CMD_RADIO_STEP_CHANNEL :  {

Maybe this can/should be done via the property stuff.

> Index: libmpdemux/demux_rawaudio.c
> ===================================================================
> --- libmpdemux/demux_rawaudio.c	(revision 18984)
> +++ libmpdemux/demux_rawaudio.c	(working copy)
> @@ -20,6 +20,12 @@
>  static int bitrate = 0;
>  static int format = 0x1; // Raw PCM
>  
> +/*
> +    This variable will be set in stream_radio to be sure that    
> +    fill_buffer will be called at least with ALSA's period_time period
> +    (if ALSA was enabled, of course)
> +*/
> +int demux_rawaudio_packs_per_sec=1;


I really think this is not acceptable and must be solved in some other
way. If it's only an alsa problem, probably in the alsa code (what is the problem anyway?)

> +ifeq ($(findstring yes,$(TV)$(RADIO_CAPTURE)),yes)

I find that really ugly, and don't we use
ifneq ($(TV)$(RADIO_CAPTURE),)
for the same effect in other places?

> +/*****************************************************************
> + *
> + * global parameters:
> + *     variable (parameter name,type,default value):
> + *
> + *     radio_param_device (device,string, "/dev/radio0")
> + *         name of radio device file
> + *     radio_param_preload (preload,number,2)
> + *         number os second to preload in buffer
> + *     radio_param_channels (channels,string,NULL)
> + *         channels list (see man page)
> + *     radio_param_volume (volume,number,100)
> + *         initial volume for radio device
> + *     radio_param_adevice (adevice,string,NULL)
> + *         name of audio device file to grab data from
> + *     radio_param_arate (arate,number,44100)
> + *         audio framerate (please also set -rawaudio rate
> + *         parameter to the same value)
> + *     radio_param_achannels (achannels,number,2)
> + *         number of audio channels

I think having this as a doxygen comment for the corresponding variable
would be nicer.

> +    mp_msg(MSGT_RADIO, MSGL_DBG3, MSGTR_RADIO_BufferString,"grab_audio_frame",priv->audio_cnt,priv->audio_drop);
> +    while (1){
> +        //read_chunk fills exact priv->blocksize bytes
> +        if(read_chunk(&priv->audio_in, priv->audio_ringbuffer+priv->audio_tail) < 0){
> +            if (priv->audio_cnt<len){
> +                usleep(10000);
> +                continue;

I'm not sure if it's really a good idea to sleep here, at least not more
than once.

> +#if defined(HAVE_ALSA9) || defined(HAVE_ALSA1X)
> +    if(audio_in_init(&priv->audio_in, is_oss?AUDIO_IN_OSS:AUDIO_IN_ALSA)<0){
> +#else
> +    if(audio_in_init(&priv->audio_in, AUDIO_IN_OSS)<0){
> +#endif

IMO get rid of the #ifdef and e.g. make sure is_oss is always true when
we don't have alsa.

> +#if defined(HAVE_ALSA9) || defined(HAVE_ALSA1X)
> +    if(is_oss)
> +        ioctl(priv->audio_in.oss.audio_fd, SNDCTL_DSP_NONBLOCK, 0);
> +    else{
> +        snd_pcm_nonblock(priv->audio_in.alsa.handle,1);
> +        /* audio driver may create internal buffer less then uor request, so decreasing 
> +           fill_buffer_s call interval to apropriate value
> +        */
> +        demux_rawaudio_packs_per_sec=priv->audio_in.alsa.period_time>1000000?1:1000000/priv->audio_in.alsa.period_time;
> +    }
> +#else
> +    ioctl(priv->audio_in.oss.audio_fd, SNDCTL_DSP_NONBLOCK, 0);
> +#endif

Similarily here, you don't need to duplicate that line if you only put
the else part under #ifdef
But actually why isn't this done in audio_in_init?

> +        return STREAM_UNSUPORTED;

Ugh, is that really spelt like that?

> +#ifdef HAVE_RADIO_V4L2
> +    mp_msg(MSGT_RADIO, MSGL_INFO, MSGTR_RADIO_UsingV4L,2);
> +#else
> +    mp_msg(MSGT_RADIO, MSGL_INFO, MSGTR_RADIO_UsingV4L,1);
> +#endif

Ok, does that mean you have to decide between v4l and v4l2 at
compilation time?

> +        while (priv->audio_cnt<preload_bytes){
> +            if(read_chunk(&priv->audio_in, priv->audio_ringbuffer+priv->audio_tail)>0){
> +                priv->audio_cnt+=priv->audio_in.blocksize;
> +                priv->audio_tail = (priv->audio_tail+priv->audio_in.blocksize) % priv->audio_buffer_size;
> +            }
> +            usleep(10000);
> +        }

Not a big deal, but I'd be happier if this potential endless loop could
be "defused". What does this do anyway? Can't a similar effect be
achieved via -cache?

> +/* ****************************************************************
> + *
> + * global parameters:
> + *     variable (parameter name,type,default value):
> + *
> + *     radio_param_device (device,string, "/dev/radio0")
> + *         name of radio device file
> + *     radio_param_preload (preload,number,2)
> + *         number os second to preload in buffer
> + *     radio_param_channels (channels,string,NULL)
> + *         channels list (see man page)
> + *     radio_param_volume (volume,number,100)
> + *         initial volume for radio device
> + *     radio_param_adevice (adevice,string,NULL)
> + *         name of audio device file to grab data from
> + *     radio_param_arate (arate,number,44100)
> + *         audio framerate (please also set -rawaudio rate
> + *         parameter to the same value)
> + *     radio_param_achannels (achannels,number,2)
> + *         number of audio channels

IMHO comment them only in one place, duplicating documentation isn't
much better than duplicating code. I usually document where the
code/real declaration is, I think doxygen handles that better.
Same for any other place where this applies.

That's all for now.

Greetings,
Reimar D?ffinger