[MPlayer-dev-eng] PVR Channel Navigation

[Benjamin Zores]

Sven Gothel a écrit :
> Again, thx to Reimar for the review.
> 
> For now, I think this one is a good result
> and in balance of 'our styles'.
> 
> Now, I believe it's up to Benjamin to bring this code in !?

Hi,

Wait a bit for that.
It's only been 2 days and first time i read it by now on.

Few comments though:
- avoid cosmetic changes
- please keep the same indentation style as in the original file
- it's not related to you or your patch but we definitely need to do 
something about channels tuning in general, having code for V4L2/PVR/DVB 
sounds weird to me ...
- i don't have pvr card at hand any longer so you might be the only one 
to test the code atm.

Now, about the patch itself:

> +#define STATIONNAMESZ   256

use a better define name, STATION_NAME_SIZE at least but this one is 
unreadable to me.

> +struct STATIONELEM {
> +    char  name[8];
> +    int   freq;
> +    char  station[STATIONNAMESZ];
> +    int   enabled;
> +};

station_elem_t, no uppercase for structures.

> +typedef struct _stationlist_t {

typedef struct stationlist_s {

> +    char                name[STATIONNAMESZ];
> +    struct STATIONELEM *list;
> +    int                 total;   /* total   number */
> +    int                 used;    /* used    number */
> +    int                 enabled; /* enabled number */
> +} stationlist_t;

please use only one space between type and name, i really don't like 
multi spaces or tabs there.

 > +  int chanidx;
 > +  int chanidx_last;

chan_idx
chan_idx_last

> -  pvr = malloc (sizeof (struct pvr_t)); 
> +  pvr = calloc (1, sizeof (struct pvr_t));

not related to this patch i guess

> +  if ( pvr->stationlist.list )
> +    free(pvr->stationlist.list);
> +
> +  if( pvr->param_channel )
> +    free(pvr->param_channel);

no space before and after ( )
(same applies everywhere in the code)

> +  if ( stationlist->list != NULL )

if (stationlist->list)

adding != NULL is redundant to me
(same applies everywhere)

> +  stationlist->total= 0;
> +  stationlist->enabled=0;
> +  stationlist->used=0;

spaces before and after = in affectation

> +  for (i = 0; i<pvr->stationlist.total ; i++)
> +  {
> +    pvr->stationlist.list[i].enabled= 0;
> +  }

useless braces

> +    if (channel && !strcasecmp(pvr->stationlist.list[i].name, channel))
> +    {
> +        break; /* found existing channel entry */
> +    }
> +    if (freq>0 && pvr->stationlist.list[i].freq==freq)
> +    {
> +        break; /* found existing frequency entry */
> +    }

again

> +    if ( station )
> +    {
> +        strlcpy(pvr->stationlist.list[i].station, station, STATIONNAMESZ);
> +    } else if ( channel ) {
> +        strlcpy(pvr->stationlist.list[i].station, channel, STATIONNAMESZ);
> +    } else {
> +        snprintf(pvr->stationlist.list[i].station, STATIONNAMESZ, "F %d", freq);
> +    }

and again (in fact, in several parts of the code)

> +  if (i >= pvr->stationlist.total)
> +  {
> +      assert(i == pvr->stationlist.used);
> +      assert(i == pvr->stationlist.total);

i don't like assert() much, is it really needed ?

> +    if ( chanlists[i].name == NULL )

if (!chanlists[i].name)

> +  if(0>chantab)

if (chantab <= 0)

> +    (void) disable_all_stations(pvr);

(void) is useless.

> +        if (!sep) continue; // Wrong syntax, but mplayer should not crash

same in 2 lines to be consistent with code style.

> +        while ((sep=strchr(station, '_')))
> +        {
> +            sep[0] = ' ';
> +        }

useless braces

> +        // if channel number is a number and larger than 1000 treat it as frequency
> +        // tmp still contain pointer to null-terminated string with channel number here

C++ comments, please use /* ... */ only

> +        if ( ( freq = atoi(channel) ) <= 1000 )
> +        {
> +            freq=-1; 
> +        }

same here

> +  if (0>=pvr->freq)

if (pvr->freq < 0)

> +  if ( get_v4l2_freq (pvr) == pvr->freq )

if (pvr->freq == get_v4l2_freq (pvr))

My tests are always if (variable == value) not the opposite

> +      (void) set_station_by_channelname_or_freq(pvr, NULL, atoi(tv_param_freq),0);

useless (void)

>  static int
>  v4l2_list_capabilities (struct pvr_t *pvr)
>  {
> @@ -916,13 +1551,14 @@
>                "%s read (%d) bytes\n", LOG_LEVEL_PVR, pos);
>      }
>    }
> -		
> +   

as nico already spotted out, cosmetics

> +  if (NULL!=pvr->stationlist.list && pvr->stationlist.used>pvr->chanidx && pvr->chanidx>=0)

if (pvr->stationlist.list && ...)

avoid the NULL != ... test

> +  if (!stream || stream->type != STREAMTYPE_PVR)
> +    return -1;

you already test this in mplayer.c when you call the functions.
this is redundant, keep it here and remove test from mplayer.c or the 
opposite but don't test it twice.

> -  pvr_stream_open, 			
> +  pvr_stream_open,

cosmetic

>  #ifdef USE_TV
>  	case MP_CMD_TV_SET_FREQ:
>  	    if (mpctx->file_format == DEMUXER_TYPE_TV)
> -		tv_set_freq((tvi_handle_t *) (mpctx->demuxer->priv),
> -			    cmd->args[0].v.f * 16.0);
> +        {
> +            tv_set_freq((tvi_handle_t *) (mpctx->demuxer->priv),
> +                    cmd->args[0].v.f * 16.0);
> +        } 

useless cosmetic

> +#ifdef HAVE_PVR
> +        else if ( mpctx->stream!=NULL && STREAMTYPE_PVR == mpctx->stream->type )
> +        {
> +            pvr_set_freq (mpctx->stream, ROUND(cmd->args[0].v.f));
> +			set_osd_msg(OSD_MSG_TV_CHANNEL, 1, osd_duration, "%s: %s",
> +                pvr_get_current_channelname(mpctx->stream),
> +                pvr_get_current_stationname(mpctx->stream));
> +        }
> +#endif /* HAVE_PVR */

keep indentation coherent

>  	    break;
>  
>  	case MP_CMD_TV_STEP_FREQ:
>  	    if (mpctx->file_format == DEMUXER_TYPE_TV)
> -		tv_step_freq((tvi_handle_t *) (mpctx->demuxer->priv),
> -			    cmd->args[0].v.f * 16.0);
> +        {
> +            tv_step_freq((tvi_handle_t *) (mpctx->demuxer->priv),
> +                    cmd->args[0].v.f * 16.0);
> +        }

cosmetic

> +#ifdef HAVE_PVR
> +        else if ( mpctx->stream!=NULL && STREAMTYPE_PVR == mpctx->stream->type )
> +        {
> +            pvr_force_freq_step(mpctx->stream, 
> +                ROUND(cmd->args[0].v.f));
> +			set_osd_msg(OSD_MSG_TV_CHANNEL, 1, osd_duration, "%s: f %d",
> +                pvr_get_current_channelname(mpctx->stream),
> +                pvr_get_current_frequency(mpctx->stream));
> +        }
> +#endif /* HAVE_PVR */

indent


I hadn't listed all parts of the code but the errors spotted out above 
have been duplicated in many other parts.

Please fix those and I'll apply your patch.

Thx,

Ben