[MPlayer-dev-eng] [PATCH] Teletext support try3 (complete set of patches)

[Reimar Döffinger]

Hello,
On Sun, Jul 29, 2007 at 07:33:35PM +0700, Vladimir Voroshilov wrote:
> I want to commit patch in 5 hours.
> I anybody has objections or still wants to review it before commit
> just say "don't commit yet" here :).

I won't do a 100% full review anyway, so go ahead. I just have a few
nits:


> +/// page magazine entry structure
> +typedef struct _MAG{
> +    tt_page* pt;
> +    int      order;
> +    int      lang;
> +} MAG;

IMO it would be better not to use a all-uppercase name, but e.g. Mag
or whatever instead.
But more importantly you should not use names starting with _, so at
least
> typedef struct _MAG{
should be changed to
> typedef struct MAG{
And no, this does not collide in any way with the MAG typedef.

> +int tv_param_tpage=100;             ///< page number
> +int tv_param_tformat=0; ///< teletext display format

Align the comments

> +static tt_char tt_error={'?',1,0,0,0,0,'?'}; // Red '?' on black background

///<

> +#define LAT_UNI 0
> +#define RUS_UNI 1
> +
> +#define LANGS 2

I don't know how you use it, but

enum {
  LAT_UNI = 0,
  RUS_UNI,
  LANGS
}

Might be better also for extensibility.


> + * fill unicode member of the same struct with apropriate utf8 code.

"appropriate"

> +    priv->bpb =  (priv->ptsp->sampling_rate /6937500.0) * ONE_FIXP+ 0.5;
> +//FIXME: STUBS!!!!
> +    priv->soc=0;
> +    priv->eoc=92;
> +//END STUBS!!!
> +    priv->bp8bl = 0.97 * 8*priv->bpb/ONE_FIXP; // -3% tolerance
> +    priv->bp8bh = 1.03 * 8*priv->bpb/ONE_FIXP; // +3% tolerance

You do realize that you place your spaces in a extremely inconsistent
way? I only mention it because you seem to do this really in all of your
patches.
IMHO you should consider deciding on some style and sticking to it, it
makes the code seem more clean.

> +    //instead of copying enture page into cache, copy only undamaged

"entire" ?

> + * \return pointer to tt_page structure if requested page is found
> + * and NULL - otherwise

That " - " makes no sense to me.

> + * Frees all memory allocated for cache (incuding array of pointers).

"including"

> + * \brief converts raw teletext page into usefull format (1st rendering stage)

"useful", only one 'l', appears in some more places.

> +#define PRINT_HEX(dp,i,h) snprintf((char*)&(dp[i].unicode),2,"%X",(h)&0xf)

Hmm.. does this patch miss some part? I can't find the definition of
tt_char.
Though snprintf seems extremely like overkill.
Something in the spirit of
AV_WL16(&dp[i].unicode, (h)&0xf > 9 ? 'A' + (h) & 0xf - 10 : '0' + (h) & 0xf)
should do it as well.
And it would get much less ugly if you do the & 0xf where you use
PRINT_HEX.
Or actually least ugly is probably
static void print_hex(... *dp, int i, uint8_t h) {
  h &= 0xf;
  h = h > 9 ? 'A' + h - 10 : '0' + h;
  AV_WL16(&dp[i].unicode, h);
}

> + * Text will be UTF8 encoded
> + */
> +static void render2text(tt_page* pt,FILE* f,int colored){

> +    render2text(pt,f,1);//dump colored ascii page (UCS-2LE encoded)

These comments do not quite fit together.

> + * Signal phase is calculated using corellation between given samples data and

"correlation"

> +    //this is not teletest

"teletext"?

> +    //Decode Remainin 45-2(clock run-in)-1(framing code)=42 bytes

"remaining"

> +//        if(decode_raw_line_sine(priv,linep,data)<=0){
> +        if(decode_raw_line_runin(priv,linep,data)<=0){

Hmm.. if you leave the uncommented variant in, maybe add a comment what
the difference between those possibilities is.

> + * Then vbi_add_dec is sequentally called (through slave

"sequentially"

> + * +-----+------------+------------------+
> + * | dec | pagenumxec | displayed number |
> + * +-----+------------+------------------+

"pagenumdec" ?

Greetings,
Reimar Döffinger