[MPlayer-dev-eng] PVR Channel Navigation (2nd try)

[Sven Gothel]

On Sunday 29 April 2007 at 03:21, Reimar Döffinger wrote:
> Hello,
> On Sat, Apr 28, 2007 at 06:04:05PM -0600, Sven Gothel wrote:
> > Same as a diff attachment, which I prefere ;-)
> 
> Please don't top-post
> 

Ay ay .. usually I don't.

<snip>

> 
> How does this relate to the existing channel switching for other stuff
> like tv?

this patch actually enables most, if not all tv likeish behavior to pvr.

which is: 

	- pvr://<channel>

	- command bindings for 
		tv_step_channel
		tv_set_channel
		tv_set_freq 
		tv_step_freq
	  despite the tv implementation it works with the real offsets,
	  i.e. +5 means +5 not +1.

	- -tv channels=<>-<>,... supports

	- the actual proper v4l2 set/get frequency implementation

I guess .. that's it.
		
> 
> > +struct STATIONELEM {
> > +    char  name[8];
> > +    int   freq;
> > +    char  station[STATIONNAMESZ];
> > +    int   enabled;
> > +};
> > +
> > +struct STATIONLIST {
> > +    char                name[STATIONNAMESZ];
> > +    struct STATIONELEM *list;
> > +    int                 count;   /* total   number */
> > +    int                 used;    /* used    number */
> > +    int                 enabled; /* enabled number */
> > +};
> 
> It think everywhere else all-uppercase is only used for defines, structs
> are usually with _t or _s (IMO _s for the struct type and _t for the
> corresponding typedef makes most sense, but that is not consistent).
> Ok, correction, CHANLIST is also all uppercase, but I don't think that's
> a good idea.

However, I 'just' tried to copy the existing style ;-)

> 
> > +static int
> > +copycreate_stationlist(struct pvr_t *pvr, int count)
> 
> You always use pvr->stationlist, so why not just a pointer to that
> instead of the whole pvr struct?
> Also all functions are supposed to have doxygen-compatible comments.
> 
> > +  if ( count < chanlists[chantab].count )
> > +  {
> > +      count = chanlists[chantab].count ;
> > +  }
> 
> count = FFMIN(count, chanlists[chantab].count);
> IMO.

yep .. I am not yet aware of all these macros, thx.
that's why I wrote everything very straightforward ;-)
yeah .. it's a hard thing when you 'go back' to native C.

In this case, it's FFMAX(..).

> 
> [...]
> > +  pvr->stationlist.count = count;
> > +  strlcpy(pvr->stationlist.name, chanlists[chantab].name, STATIONNAMESZ);
> > +
> > +  for ( i=0; i<chanlists[chantab].count; i++ )
> > +  {
> > +    pvr->stationlist.list[i].station[0]= '\0' ; /* no station name yet */
> > +    strlcpy(pvr->stationlist.list[i].name, chanlists[chantab].list[i].name, STATIONNAMESZ);
> > +    pvr->stationlist.list[i].freq = chanlists[chantab].list[i].freq;
> > +    pvr->stationlist.list[i].enabled= 1 ; /* default enabled */
> > +    pvr->stationlist.enabled++;
> > +    pvr->stationlist.used++;
> > +  }
> > +  for ( ; i<pvr->stationlist.count; i++ )
> > +  {
> > +    pvr->stationlist.list[i].station[0]= '\0' ;
> > +    pvr->stationlist.list[i].name[0]= '\0' ;
> > +    pvr->stationlist.list[i].freq   = -1;
> > +    pvr->stationlist.list[i].enabled= 0;
> > +  }
> 
> pvr->stationlist.count already indicates them as unused, so why the need
> to initialize? Also, calloc already sets them all to 0, so at most
> setting freq to -1 is needed, though I don't know why -1 is used for
> invalid frequency, 0 is as obviously invalid and would need no extra
> initialization.

indeed, but since we have no constructors, I just wanted to make sure ..

but yes, we can remove that.

> 
> > +static void
> > +print_all_stations(struct pvr_t *pvr)
> > +{
> > +  if (!pvr) 
> > +    return;
> > +
> > +  if (NULL!=pvr->stationlist.list && pvr->stationlist.count>0)
> 
> Is pvr == NULL or pvr->stationlist.count>0 and pvr->stationlist.list ==
> NULL supposed to happen? The other code looks to me like you really try
> to avoid this, so it would be a bug if this happened, and at most an
> assert should be used to check for it.

first of all, I like to check all 'dirty' pointers per default when using C.
if I didn't, well, I was lazy ;-)

from an algorythm perspective, assuming writing prooven ones,
sure, assert is enough ..

assume somebody calls this function at some time, where the stations 
were not constructed yet, or ..

maybe just my embedded programming paranoia ;-)

> 
> > +  {
> > +      int i;
> > +
> > +      for (i = 0; i<pvr->stationlist.count ; i++)
> > +      {
> > +        mp_msg (MSGT_OPEN, MSGL_INFO,
> > +                "%s %3d: [%c] channel: %8s - freq: %8d - station: %s\n", LOG_LEVEL_V4L2, 
> > +                i, (pvr->stationlist.list[i].enabled)?'X':' ',
> > +                pvr->stationlist.list[i].name, pvr->stationlist.list[i].freq,
> > +                pvr->stationlist.list[i].station);
> > +      }
> > +  }
> 
> Also there is no point in checking for pvr->stationlist.count>0, the for
> loop won't be executed anyway if it isn't.

Indeed.
One could argue and calculate the difference of the extra conditional check 
with the stack calculations, but especially where the auto variable 'i'
will be optimized away .. it's chicken shit ;-)
So .. yes, agreed.

> 
> > +static int
> > +disable_all_stations(struct pvr_t *pvr)
> > +{
> > +  if (!pvr) 
> > +    return -1;
> > +
> > +  if (NULL!=pvr->stationlist.list && pvr->stationlist.count>0)
> > +  {
> > +      int i;
> > +
> > +      for (i = 0; i<pvr->stationlist.count ; i++)
> > +      {
> > +        pvr->stationlist.list[i].enabled= 0;
> > +      }
> > +      pvr->stationlist.enabled=0;
> 
> Worse, in this case pvr->stationlist.enabled=0; will not be done if
> pvr->stationlist.count <= 0, which does not fit my idea of what I think
> this function is supposed to do.

I see.

It's well described in the declaration, count is the total number of stations.
So count==0 means no station, the number is the exclusive upper border - count.

Ok, here is the 'logic',
which I agree is more a hack. (I would love to have the STL here ;-)

1.) setup stationlist with the internal channellist of the choosen tv norm,
i.e. 'ntsc-cable'.

2.) if the user specifies a channel-name -> station-name, 
    it frequency -> station-name mapping,
    we copy the information into the existing stationlist,
    and if it's a new entry, we add it.
    Because of the latter, there is the methodology of cached unused entries,
    i.e. the 'count' and 'used' metapher.
    Yep .. ugly C.

> 
> > +      return 0;
> > +  } 
> > +  return -1;
> 
> Also those two functions are very similar in structure but one returns
> int and one void, this seems inconsistent to me...

I tried to make it consistence as the other functions.
Maybe for some functions, where there is absolutly no way of error,
I skipped it. Or I was lazy ;-)

> 
> Didn't review most of the rest, maybe you could just remove most of the
> checks for things that should never happen unless there is a bug
> somewhere else? I don't think extra code to hide bugs is good, esp.
> when it makes the code so hard to read and more than doubles code size
> as in some cases here...

See above .. it's not about bugs, it's about testing pointers -> paranoia.

Ok .. I see, I may send out another revision a little later.

Now where I see, there is somehow a QA, 
how about automated test systems other than just compilation ?

Is there anything like this for MPlayer ?

Thank you.

Cheers, Sven
-- 
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/20070429/1ffa1892/attachment.pgp>