[MPlayer-dev-eng] [PATCH] Teletext support try3 (1/5, core)

[Vladimir Voroshilov]

Hi, Michael

2007/7/21, Michael Niedermayer <michaelni at gmx.at>:
> Hi
>
> On Sat, Jul 21, 2007 at 07:51:47AM +0700, Vladimir Voroshilov wrote:
>
> > +static tt_page** ptt_cache;
> > +static unsigned char* ptt_cache_first_subpage;
>
> shouldnt these and other be part of the priv_vbi_t context?

Fixed.

> > +int lang2id (int lang){
>>[...]
> > +unsigned int conv2uni(unsigned int p,int lang)
>
> these functions and others should either get a sensible name
> (inlcuding teletext or such) or be static

Of course all of them should be static.

> > +static void put_to_cache(priv_vbi_t* priv,tt_page* pg){
> > +    int cache_idx;
> > +    tt_page* pgc; //page in cache
> > +    int i;
> > +
> > +    cache_idx=(pg->pagenum&0x7ff)+VBI_MAX_PAGES*pg->subpagenum;
> > +    pthread_mutex_lock(&(priv->buffer_mutex));
> > +    if(ptt_cache_first_subpage[pg->pagenum]>pg->subpagenum){
> > +        ptt_cache_first_subpage[pg->pagenum]=pg->subpagenum;
> > +    }
>
> indexing ptt_cache_first_subpage by 0x100-0x8FF wastes space
This is the bug (look at array size). Fixed.

> also why is the internal page number using such a odd range, wouldnt it
> make more sense to just do the convertion for display
> instead of filling the code with &0x7ff and if/else checks
>
> also
> ((p+0x700)&0x7FF)+0x100
> can be used for the convertion no need to have these if/else all over the code

>From this version range 0x000-0x7ff is used in tvi_vbi.c and
0x100-0x8ff outside it.
To tell the truth current cache implementation is ugly.
Unfortunately, i havn't neither enough knowledge nor ideas to make
really good one.

> > +        for(col=0;col<VBI_COLUMNS;col++){
> > +            i=row*VBI_COLUMNS+col;
> > +            c=raw[i];
> > +            if(c&0x80){ //damaged char
> > +              p[i]=tt_error;
> > +              p[i].raw=c;
> > +              continue;
> > +            }
>
> indention is inconsistant

Fixed.

> > +/**
> > + * \brief checks whether page is ready and copies it into cache array if so
> > + * \param priv private data structure
> > + * \param magAddr page's magazine address (0-7)
> > + *
> > + * Routine also calls decode_page to perform 1st stage of rendering
> > + */
> > +void store_in_cache(priv_vbi_t* priv, int magAddr){
> > +    if (priv->mag[magAddr].order!=23){ //is last row ?
> > +        mp_msg(MSGT_TV,MSGL_DBG2,"store_in_cache(%d): pagenum:%x\n",
> > +            priv->mag[magAddr].order,
> > +            priv->mag[magAddr].pt->pagenum);
> > +        return;
> > +    }
> > +
> > +    put_to_cache(priv,priv->mag[magAddr].pt);
> > +    priv->curr_pagenum=priv->mag[magAddr].pt->pagenum;
>
> wouldnt it be better to store each row in the cache as it is received
> instead of doing it only when the last row is encountere?
> think of cases where a row is so damaged that its not recognized properly

Fixed.

> [...]
> > +    flag%=4;
> > +    if(flag<0)
> > +        flag+=4;
>
> flag &=3;

Fixed.


-- 
Regards,
Vladimir Voroshilov     mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tt37_core.diff
Type: text/x-diff
Size: 42187 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20070722/d5e245d6/attachment.diff>