[MPlayer-dev-eng] PVR Channel Navigation

Benjamin Zores ben at geexbox.org
Mon Apr 30 14:32:10 CEST 2007


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



More information about the MPlayer-dev-eng mailing list