[MPlayer-dev-eng] PVR Channel Navigation

[Sven Gothel]

On Monday 30 April 2007 at 06:32, Benjamin Zores wrote:
> 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.

done

> 
> > +struct STATIONELEM {
> > +    char  name[8];
> > +    int   freq;
> > +    char  station[STATIONNAMESZ];
> > +    int   enabled;
> > +};
> 
> station_elem_t, no uppercase for structures.

done

> 
> > +typedef struct _stationlist_t {
> 
> typedef struct stationlist_s {

done

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

I disagree, it betters the readability if it's written in proper columns.
So, I applied this here, but I have not changes it in the already existing structures
to respect your style. Hmm ..

Ok, changed, even though, I don't like it ;-)

> 
>  > +  int chanidx;
>  > +  int chanidx_last;
> 
> chan_idx
> chan_idx_last

done

> 
> > -  pvr = malloc (sizeof (struct pvr_t)); 
> > +  pvr = calloc (1, sizeof (struct pvr_t));
> 
> not related to this patch i guess

it is, calloc set's the content to zero,
which is now mandatory for the stationlist_t.

added a comment.

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

done, even though I like it because of the readability.

> 
> > +  if ( stationlist->list != NULL )
> 
> if (stationlist->list)
> 
> adding != NULL is redundant to me
> (same applies everywhere)

done

> 
> > +  stationlist->total= 0;
> > +  stationlist->enabled=0;
> > +  stationlist->used=0;
> 
> spaces before and after = in affectation

well, such a thing exists in the current code as well,
as other of your formating claims.
so this call is not very consistent.

done, almost.

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

I like 'em.

Indeed not using such braces may impact later changes,
i.e. there was a prominent example in Xorg a long time ago,
where one just added a statement, thinking it's in the block.
But since it was C, not python .. well ;-)

But I wasn't very consistent here either.

However, since you are the one .. 

done for one liners.

> 
> > +    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);
> 

done

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

well, I think it's a good practise to make a point with 'em.

anyway, done.

> 
> > +    if ( chanlists[i].name == NULL )
> 
> if (!chanlists[i].name)
> 
> > +  if(0>chantab)
> 
> if (chantab <= 0)
> 
> > +    (void) disable_all_stations(pvr);
> 
> (void) is useless.

no, it discards the return value -> no warning.

done - changed the semantics a bit.

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

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

done

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

done

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


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

it's good practise to have no l-value on the left side of an 
comparison operand to avoid an assignment, if you do a typo.

not done.

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

see above.

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

see above

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

right, but too much trouble true bring that extra space back ;-)

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

done

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

it doesn't comply with my idea of safe code, 
but however ..

done

> 
> > -  pvr_stream_open, 			
> > +  pvr_stream_open,
> 
> cosmetic

see above.

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

the new intendation is good, removed braces.
done

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

done

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

done

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

done

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

Ok .. thanks.

We don't have to agree in all those points, of course not.
So I just applied them all as far as I can see all those, but the l-value thing.

Cheers, Sven

> 
> Thx,
> 
> Ben
> _______________________________________________
> MPlayer-dev-eng mailing list
> MPlayer-dev-eng at mplayerhq.hu
> http://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
> 
> 



-- 
health & wealth
mailto:sgothel at jausoft.com ; www  : http://www.jausoft.ca ; pgp: http://www.jausoft.com/gpg/
land : +1 (780) 637 3842 ; cell: +1 (780) 952 4481
Timezone MST: EST-2, UTC-7, CET-8 ; MDT: EDT-2, UTC-6, CEDT-8
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20070504/e60c7116/attachment.pgp>