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

[Reimar Döffinger]

Hellom
On Fri, Jul 14, 2006 at 12:33:36AM +0700, Vladimir Voroshilov wrote:
> Reimar Doeffinger wrote: 
> > 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?

Well, there are the MP_CMD_SET_PROPERTY and MP_CMD_GET_PROPERTY which
are special commands to modify all kinds of "properties". I don't know
this stuff well, so maybe it isn't suitable, but you might want to have
a look at it anyway.

> > > +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. [...]

I don't think so, see comment further below.


> > > +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. [...]

I understand. Probably my solution in libmpdemux/Makefile is wrong. But
at least it should be consistent. So may Diego figure that out :-)

> > > +    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?

Actually... No, just return whatever data you have (maybe sleeping a bit
if it's really very little data, just test that it never uses and insane
amount). Well, actually you should also make sure it is aligned at a
sample boundary (i.e. len is divisible by number of channels multiplied
by number of bytes per sample), otherwise MPlayer might break thinks when e.g.
resampling.
I was mostly saying this because I think (and a closer look at
demux_rawaudio seems to confirm) that this is where your problem lies.
If you return data for less than one second of audio demux_rawaudio will
accept that happily and return for more much earlier.


> > > +        return STREAM_UNSUPORTED;
> > 
> > Ugh, is that really spelt like that?
> Yep. Look at stream.h :)

I did. Just wanted to check if my eyes were playing a joke ;-)
It's already used in quite a lot of places - shocking.

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

I guess the people doing binary build would be happy if they could build
in support for both. But that really is just a very minor
"nice-to-have".

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

Sure. I guess I've said it often enough, but when reviewing I don't try
to full understand everything, so if something I say doesn't make sense
to you that could be because it really doesn't make sense :-)
It just looked to me like you were implementing some kind of cache
(probably to avoid initial stuttering or so?), and in that case the -cache
option could probably provide the same functionality while being more
flexible. Disadvanteg is of course that the user has to know about it and
use it.

Greetings,
Reimar Döffinger