[MPlayer-dev-eng] PVR Channel Navigation (3rd rev)

[Reimar Döffinger]

Hello,
On Sun, Apr 29, 2007 at 05:23:27AM -0600, Sven Gothel wrote:
> @@ -126,7 +151,14 @@
>    pvr->saturation = 0;
>    pvr->width = -1;
>    pvr->height = -1;
> -  pvr->freq = NULL;
> +  pvr->freq = -1;
> +  pvr->chanidx  = -1;
> +  pvr->chanidx_last  = -1;
> +  pvr->stationlist.name[0] = '\0';
> +  pvr->stationlist.list    = NULL;
> +  pvr->stationlist.total   = 0;
> +  pvr->stationlist.used    = 0;
> +  pvr->stationlist.enabled = 0;

I'd suggest either memset or a clear_/init_stationlist function.
The same in the copycreate_stationlist.

> +  if ( NULL!= pvr->stationlist.list )
> +    free(pvr->stationlist.list);

Don't you notice or don't you mind the very varying way you use spaces,
especially in if statements?
Also, you sometimes write "NULL != ptr", sometimes "ptr != NULL" and
sometimes "!ptr".
I slightly prefer the "if (!ptr)"/"if (ptr)" way and that's used in the
existing code alread.

> +/**
> + * Copy Constructor for stationlist
> + *
> + * @see parse_setup_stationlist
> + */

The first line should be marked with @brief (or \brief, whichever).
Parameters and return values should be documented as well.

> +static int
> +copycreate_stationlist(struct pvr_t *pvr, int num)


I still do not see a point in passing "struct pvr_t *" instead of only
"struct STATIONLIST *". Similarly also for print_all_stations,
disable_all_stations, set_station and after that also
parse_setup_stationlist

[...]
> +  if ( pvr->stationlist.list != NULL )
> +  {
> +    free ( pvr->stationlist.list );
> +    pvr->stationlist.list = NULL;
> +  }

note that this requires that stationlist _must_ be initialized before
calling this function. This needs to be documented.
Or IMO better just removed, because putting the free operation here
instead of a separate function makes it very likely that someone in the
future who adds more stuff that is alloced to the STATIONLIST struct
forgets to also adds free here.

> +    (void) disable_all_stations(pvr);

Please don't do do that (void).

> +    while (*channels) 
> +    {
> +        char* tmp = *(channels++);
> +        char* sep = strchr(tmp,'-');
> +        int  freq = -1;
> +        int i;

unused.

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

It should not ignore it either.

> +        strlcpy(station, sep + 1, STATIONNAMESZ);
> +
> +        sep[0] = '\0';

I doubt you're allowed to modify tv_param_channels.

> +        strlcpy(channel, tmp, STATIONNAMESZ);
> +
> +        while ((sep=strchr(station, '_')))
> +        {
> +            sep[0] = ' ';
> +        }
> +
> +        // if channel number is a number and larger than 1000 threat it as frequency

"treat", not "threat".

> +        // tmp still contain pointer to null-terminated string with channel number here
> +        if (atoi(channel)>1000)
> +        { 
> +            freq=atoi(channel);
> +        }

freq = atoi(channel);
if (freq <= 1000) freq = 0;
else channel = NULL;

> +  memset(&vf, 0, sizeof(vf));
[...]
> +  vf.type = 0;

That is really pointless unless you find a more descriptive name (e.g.
define) for this 0.

[...]
> +  if (pvr->freq>0)
> +  {
> +    if ( get_v4l2_freq (pvr) == pvr->freq )
> +    {
> +        mp_msg (MSGT_OPEN, MSGL_STATUS,
> +                "%s Frequency %d allready set.\n", LOG_LEVEL_V4L2, pvr->freq);
> +        return 0;
> +    }

s/allready/already/
Does this normally happen unless the user explicitly wants it? If not,
I'd say allow it, setting it twice might fix problems.

> +  }
> +
> +  if (pvr->dev_fd < 0)
> +    return -1;
> +
> +  if (0>=pvr->freq)

The second check for pvr->freq in that function. And by the time a
frequency value arrives here it has probably already been checked at 5 other
places anyway.

> +  if (!vt.signal)
> +  {
> +      mp_msg (MSGT_OPEN, MSGL_ERR,
> +              "%s NO SIGNAL at frequency %d (%d)\n", LOG_LEVEL_V4L2, pvr->freq, vf.frequency);
> +      /* still return 0, since this is 'just informal' */

"informal"? Do you mean informational?

> +static int
> +set_station_by_channeloffset(struct pvr_t *pvr, int offset, int v4lAction) 

Is an arbitrary offset instead of just +/-1 worth the extra complexity?

> +  if ( pvr->stationlist.enabled>=abs(offset) )
> +  {
> +      int gotcha=0;
> +      int chidx=pvr->chanidx + offset;
> +
> +      while(!gotcha)
> +      {
> +          if ( chidx  < 0 ) chidx += pvr->stationlist.used;
> +          if ( chidx >= pvr->stationlist.used ) chidx -= pvr->stationlist.used;

chidx = (chidx + pvr->stationlist.used) % pvr->stationlist.used;

[...]
> +            chidx += (offset>0)?1:-1;

FFSIGN(offset). Not sure that actually looks better though. Same about
abs() vs. FFABS().

> +static int
> +set_station_by_channelname(struct pvr_t *pvr, const char *channel, int v4lAction)
> +{
> +  if (!pvr || !pvr->stationlist.list) 
> +    return -1;
> +
> +  if ( pvr->stationlist.enabled>0 && channel )

Please check whether these checks really improve anything or actually
make a giant mess. At least the "pvr->stationlist.enabled>0" checks
leads to the weird inconsistency that switching to a disabled channel
causes only a warning if there is at least one enabled channel,
otherwise it fails without giving the user any clue why.

> +  {
> +      int i;
> +
> +      /* select channel */
> +      for (i = 0; i<pvr->stationlist.used ; i++)
> +      {
> +        if ( !strcasecmp(pvr->stationlist.list[i].name, channel))
> +        {
> +            if ( !pvr->stationlist.list[i].enabled )
> +            {
> +                mp_msg (MSGT_OPEN, MSGL_WARN,
> +                    "%s Switch disabled to user station channel: %8s - freq: %8d - station: %s\n", LOG_LEVEL_V4L2, 
> +                    pvr->stationlist.list[i].name, pvr->stationlist.list[i].freq,
> +                    pvr->stationlist.list[i].station);
> +
> +                return -1;
> +            }
> +            pvr->freq     = pvr->stationlist.list[i].freq;
> +            pvr->chanidx_last  = pvr->chanidx;
> +            pvr->chanidx  = i;
> +            break;
> +        }
> +      }
> +
> +      if (i >= pvr->stationlist.used)
> +      {
> +        mp_msg (MSGT_OPEN, MSGL_WARN,
> +                "%s unable to find channel %s\n", LOG_LEVEL_V4L2, channel);
> +      } else {
> +        mp_msg (MSGT_OPEN, MSGL_INFO,
> +            "%s Switch to user station channel: %8s - freq: %8d - station: %s\n", LOG_LEVEL_V4L2, 
> +            pvr->stationlist.list[i].name, pvr->stationlist.list[i].freq,
> +            pvr->stationlist.list[i].station);
> +
> +        if(v4lAction)
> +        {
> +            return set_v4l2_freq (pvr);
> +        }
> +        return (pvr->freq>0)?0:-1;

Why the "pvr->freq>0" only after set_v4l2_freq, and is set_v4l2_freq
being called when the channel was not found really intended?


> +static int
> +set_station_by_freq(struct pvr_t *pvr, int freq, int v4lAction)

This is almost the same code, only one comparison and one message are
different. Merge those two functions.

> +static int 
> +force_freq_offset (struct pvr_t *pvr, int offset)

This name is confusing. It steps the current frequency up or down, it
does not set an offset (an offset would remain when switching to a
different offset.
And I also think this could (and thus should) be put in a separate
patch since it is independent from the whole station stuff.

> +
>  static int
>  v4l2_list_capabilities (struct pvr_t *pvr)
>  {
> @@ -916,13 +1573,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;
>  }
>  
> +
>  static int
>  pvr_stream_open (stream_t *stream, int mode, void *opts, int *file_format)
>  {

cosmetics.

> @@ -933,9 +1591,13 @@
>    if (mode != STREAM_READ)
>      return STREAM_UNSUPORTED;
>    
> +  if (strlen (stream->url) > 6 && stream->url[6] != '\0')

wtf?!?!?

> @@ -1029,12 +1691,129 @@
>    return STREAM_OK;
>  }
>  
> +/* Public access */
> +
> +const char *
> +pvr_get_current_stationname (stream_t *stream)
> +{
> +  struct pvr_t *pvr;
> +
> +  if (!stream || stream->type != STREAMTYPE_PVR)
> +    return NULL;
> +  
> +  pvr = (struct pvr_t *) stream->priv;
> +
> +  if (NULL!=pvr->stationlist.list && pvr->stationlist.used>pvr->chanidx && pvr->chanidx>=0)
> +  {
> +        return pvr->stationlist.list[pvr->chanidx].station;
> +  }
> +  return NULL;
> +}

How long is the returned pointer valid?

> +const char *
> +pvr_get_current_channelname (stream_t *stream)

Same here. I don't think anything except returning a strdup'd string is
a good idea.

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

Can't see a reason for these changes.

> +#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 */
>  	    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);
> +        }

or these.

I think making that patch at least 20% smaller is a good and feasible
target.

Btw. I just noticed we have an official maintainer for stream_pvr.c
according to DOCS/tech/MAINTAINERS.

Greetings,
Reimar Döffinger