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

[Sven Gothel]

On Sunday 29 April 2007 at 09:40, Reimar Döffinger wrote:
> 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.
> 

Sure possible.

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

Actually - as long it is readable, sometimes I don't care.

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

Sure possible.

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

Just adapting to the old style. Right.

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

isn't it the overall free operation ?
I may double check ..

> 
> > +    (void) disable_all_stations(pvr);
> 
> Please don't do do that (void).

Ha !
Sure I do, and that's a good point to talk about '-Wall'.
From the 2nd -> 3rd version, I fixed stuff the compiler should have complained about.
Somebody should make sure to enable warnings during compilation.

My proposal - let's get module per module warning clean.

Here, it's a clear statement, that we discard the return value.

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

well...

> 
> > +        if (!sep) continue; // Wrong syntax, but mplayer should not crash
> 
> It should not ignore it either.
Maybe a message ..

> 
> > +        strlcpy(station, sep + 1, STATIONNAMESZ);
> > +
> > +        sep[0] = '\0';
> 
> I doubt you're allowed to modify tv_param_channels.
I am .. since it's not an array of const char's ;-),
and sematically it's for pvr here.

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

yep

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

yep, i thought so myself ;-)

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

yep .. i thought so myself ;-)

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

It will disturb the mpeg stream from the card,
that's why it's in.

The device can be read by others as well ..

The special use case is, where you call mplayer without 
giving a channel/frequency, so it just should stay tuned
without interrupting the stream.

I should add a comment here ;-)

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

well ..

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

same shit, 'just an informal message'
my bad grammar ;-)

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

Yes it is, now you can do channel hopping as you wish,
which is usefull for let's say 50 channels.

And .. the specification of tv_step_channel says so ;-)

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

of course .. me stupid, thanx ;-)

> 
> [...]
> > +            chidx += (offset>0)?1:-1;
> 
> FFSIGN(offset). Not sure that actually looks better though. Same about
> abs() vs. FFABS().

right, I agreed to have a look into libavutils.

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

Good point, such an informal message should be added.

Since this is a switch to a specific channel,
I don't want to traverse to the next available one.

<snip>

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

only 'pvr->freq>0' means the function's result is ok.

for the 2nd question: Yes, 
the function itself checks it and send a message.

and .. it actually reaches this point only if a freq is set ;-)
exception: the channellist.freq entry is invalid.
so yes, such checks will better remain in the code.

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

Since they are not public - I agree.
For public functions, i.e. outside the module, I would not.

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

Well, it sets a frequency offset - one time.

ok, I rename it.

> And I also think this could (and thus should) be put in a separate
> patch since it is independent from the whole station stuff.

Well, this patchset should implements all possible/feasible
tv functions to pvr.
IMHO - it should stay -> tv_step_frequency.

> 
> > +
> >  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?!?!?

hehe ;-)

I will add a comment here ;-)

it's the channel-name in the URL: 'pvr://7' for example.
see stream_tv.c.

Yes, I don't like the strdup later on either,
but it's not really a memory leak.

We may have a private string object for that channel parameter.

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

lifetime question, good one.

till 'pvr_stream_close', I will add a comment.

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

I prefere { } always, since not having them can cause huge pain,
especially if the statement is more than one line.
Call it a cosmetic change while I was there.

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

X for code
Y for comments
X+Y==0 ;-)

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

So I will add him to the CC list 
for the next revision .. nice.

My conclusion of this review is,
that you catched a few good points.
Encouraging one to do proper code.

Good attitude, thx.

Cheers, Sven

> 
> Greetings,
> Reimar Döffinger
> _______________________________________________
> 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/20070429/93cf0cf0/attachment.pgp>