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

[Vladimir Voroshilov]

2007/7/15, Michael Niedermayer <michaelni at gmx.at>:
> Hi
>
> On Sun, Jul 15, 2007 at 06:01:13PM +0700, Vladimir Voroshilov wrote:
> [...]
> > + *  optional libzvbi support
>
> no
TODO dropped out

> indention of these 2 structs is inconsistent
Fixed.

> please use consistent abbriviations (no vs. num)
Fixed.

> these could be enums
Done.

> it should by ///<
> and it could be vertically aligned
Fixed.


> this has trailing whitespace though its not forbidden in mplayer
> so this is just a note not a complaint
Fixed.

> no, remove this or the patch is rejected, converting from teletexts
> native encodings to some other encoding and then with iconv to unicode/utf8
> is unaccpetable, iconv is not needed!
>
> convert teletexts native encoding to unicode and store with PUT_UTF8() as utf8
Done. iconv is no more required.

> ohh my god is this a mess, please use something like the following
> (note, its untested)
>
I've kept two versions: my simplified and your's, provided in another mail.
See what is better.

> NOOOOOOOOOOOOOOOOOOOOOOOO!
>
> use utf8 with PUT_UTF8()

Done.

> > +void render2ascii(priv_vbi_t* priv,FILE* f,int pgno,int colored){
>
> and dont call it ascii if its not ascii
changed to render2text

> why should this be called with a pg==NULL ?
Related code was checked. This check is redudant. Removed.


> ?1:0 is superflous
Removed.

> dont mix tabs and spaces
All tab/spaces mess is fixed.

> p[i].unicode= p[i].raw - 0x20;
Fixed.

> strcpy(), memcpy() or something like that with "No teletext"
Done with 'for' loop over string

> priv_vbi->pgno & 0xf00
Done.

> s(n)printf with "%X.%X" or something like that
done with several 'snprintf'

> you should copy only _UNDAMAGED_ chars, that is ones with
> correct parity bit into the cache, the others should be left as they where
> before in the cache or '?' if the page was not in the cache
>
> also color/graphic char decoding MUST be done after the cache otherwise
> you cannot recover from damage in chars which affect the meaning of
> following chars
Done. Page now looks better.
Why did not i get such idea myself ?

> > + * flag:
> > + * 0 - off
> > + * 1 - on & opaque
> > + * 2 - on & transparent
> > + * 3 - on & transparent  with black foreground color (only in bw mode)
>
> use enum or define dont just document the meaning in some comment
Done with enums.

> > +    if(priv_vbi->on==0 && priv_vbi->input_section!=NULL) {
>
> the != NULL is superflous
Fixed.

> > +    case TV_VBI_CONTROL_SET_PAGE:
> > +    {
> > +        int val=*(int *) arg;
> > +        if(val<0 || val>0x799)
> > +            return TVI_CONTROL_FALSE;
> > +        pthread_mutex_lock(&(priv_vbi->buffer_mutex));
> > +        priv_vbi->pgno = steppage(0, val);
>
> this is wrong, if the user asks for page =0x7AB then you should return
> that page and not another page
Well. Fixed. to tell the truth my provider does'nt send any page from
outside standard region. But if client wants... :)


-- 
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: tt30_core.diff
Type: text/x-diff
Size: 46800 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20070717/d154fc00/attachment.diff>