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

[Reimar Döffinger]

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

> On Saturday 28 April 2007 at 18:02, Sven Gothel wrote:
> > Well, I have fixed some issues:
> > 	- parse the stream url and use it, like the tv module, pvr://<channel>
> > 	- use the proper frequency divider depending of the tuner model
> > 	- reading the current frequency
> > 	- setting the frequency only, if it differs
> > 	- fixing the command parameter usage, using float for frequency
> > 	- some beautifying ..
> > 
> > Oh yes, TS=SPACE, 
> > and the review is inline now, as you do it here now.
> > [ even though I prefere diff attachments .. ]
> > 
> > Have fun ..

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

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

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

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

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

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

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

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

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

> +
>  static int
>  v4l2_list_capabilities (struct pvr_t *pvr)
>  {
> @@ -916,13 +1570,14 @@
>                "%s read (%d) bytes\n", LOG_LEVEL_PVR, pos);
>      }
>    }
> -		
> +        
>    if (!pos)
>      mp_msg (MSGT_OPEN, MSGL_ERR, "%s read %d bytes\n", LOG_LEVEL_PVR, pos);
>  
>    return pos;
>  }
>  
> +

Some cosmetics here and in some other places. I think
DOCS/tech/svn-howto.txt and/or DOCS/tech/patches.txt gives diff commands
to get a without most of them very quickly.

Greetings,
Reimar Döffinger