[MPlayer-dev-eng] PVR Channel Navigation
Sven Gothel
sgothel at jausoft.com
Fri May 4 10:22:51 CEST 2007
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>
More information about the MPlayer-dev-eng
mailing list