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

[Vladimir Voroshilov]

Reimar Doeffinger wrote: 
> 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.
What does you mean?
> 
> > 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?)
Only one another way, i can see, is using threads. The reason is small ALSA's buffer (not enouph to
store 2 seconds of sound). ALSA's buffer size is controled by ALSA driver. If buffer will be 
smaller than 2 seconds we will have a lot of ALSA's xrun events (without using threads, of cause),
becase demux will call fill_buffer_s continiously untill fills  one second size demux_packet, 
after that it will be sleeping about one second, then call fill_buffer_s again and so on.
If anobody can suggest another way to avoid xrun events. i'll try it.
> 
> > +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?
ifneq does not have SAME effect. By default RADIO_CAPTURE=no (not empty. you can also set  
--disable-tv-v4l --disable-tv-v4l and check the behaviour of ifneq ($(TV_V4L)$(TV_V4L2),) - 
make will compile alsa1x.c when it souldnt due to configure options), 
so "ifneq ($(TV)$(RADIO_CAPTURE),)" will be true in this case.
"ifeq ($(findstring yes,$(TV)$(RADIO_CAPTURE)),yes)" will be true if and only if at least one
of given variables is equals to 'yes'.
> 
> I think having this as a doxygen comment for the corresponding variable
> would be nicer.
> 
Well, will be fixed.
> > +    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.
> 
Why? We just wait for at least len bytes of data as long as need. Does you mean to  check for
wait timeout here?
> > +#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.
Ok. AUDIO_IN_OSS and AUDIO_IS_ALSA is always defined now. so i'll fix it.
> 
> > +#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?
> 
Agree.
> > +        return STREAM_UNSUPORTED;
> 
> Ugh, is that really spelt like that?
Yep. Look at stream.h :)
> 
> > +#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?
Yes. autodetected (or manually set) by configure.
> 
> > +        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?
> 
Hmm. I'll look more carefull to -cache. If so, i'll remove this code.

-- 
Vladimir Voroshilov mailto:voroshli at univer.omsk.su
Omsk State University
JID: voroshil at jabber.ru
ICQ: 95587719