[MPlayer-dev-eng] [PATCH] TV teletext

[Vladimir Voroshilov]

2007/3/22, Ötvös Attila <oattila at chello.hu>:
>
> Hi All!
>
> I created patch to TV teletext.
>
> Now review of tvi_vbi.*

>--- stream/tvi_vbi.h (none)
>+++ stream/tvi_vbi.h (working copy)
>@@ -0,0 +1,73 @@
>[skip]
>+ int act_subno; // stored page number
>+ int act_pgno; // stored subpage

comments are swapped.

>--- stream/tvi_vbi.c (none)
>+++ stream/tvi_vbi.c (working copy)
>@@ -0,0 +1,741 @@
>
>[skip]
>
>+/*
>+// Open vbi
>+*/
>+priv_vbi_t *open_vbi(char *device, unsigned int services, int tformat, int
startpage)
>+{
>+priv_vbi_t *priv_vbi;
>+if(inited) return NULL;
>+inited=1;
>+if(NULL==(priv_vbi=malloc(sizeof(priv_vbi_t)))) return NULL;
>+memset(priv_vbi,0,sizeof(priv_vbi_t));
>+priv_vbi->device=strdup(device);
>+priv_vbi->services=services; // capture services

Last line can be safely removed.

>[skip]
>+/*
>+// Close vbi capture, decodeand and free priv_vbi
>+*/
>+void close_vbi(priv_vbi_t *priv_vbi)
>+{
>
>[skip]
>
>+free(priv_vbi);
>+inited=0;
>+return;
>
"return;" at the end of returning void routine is not needed.

>[skip]
>+/*
>+// Decode event handler
>+*/
>+static void event_handler(vbi_event *ev, void *data)
>+{
>+priv_vbi_t *user_vbi = (priv_vbi_t*)data;
>+vbi_page pg;
>+char *s;
>+int i;
>+
>+switch (ev->type) {
>+ case VBI_EVENT_CAPTION:
>+ printf("caption\n");

should be mp_msg, if this msg is really needed.

>+ case VBI_EVENT_NETWORK:
>+ s=ev->ev.network.name;
>+ if(s) {
>+ pthread_mutex_lock(&(user_vbi->buffer_mutex));
>+ user_vbi->network_name = strdup(s);

If VBI_EVENT_NETWORK can occure more than once, there will be memory leak.

>+ pthread_mutex_unlock(&(user_vbi->buffer_mutex));
>+ }
>+ break;
>+ case VBI_EVENT_NETWORK_ID:
>+ s=ev->ev.network.name;
>+ if(s) {
>+ pthread_mutex_lock(&(user_vbi->buffer_mutex));
>+ user_vbi->network_id = strdup(s);

Same as previous.

>+ pthread_mutex_unlock(&(user_vbi->buffer_mutex));
>+ }
>+ break;
>+ case VBI_EVENT_TTX_PAGE:
>+ pthread_mutex_lock(&(user_vbi->buffer_mutex));
>+ user_vbi->curr_pgno=ev->ev.ttx_page.pgno; // page number
>+ user_vbi->curr_subno=ev->ev.ttx_page.subno; // subpage
>+ char out[VBI_TXT_PAGE_SIZE];

Mixed declaration and code. Will be rejected.

>[skip]
>+ case VBI_TFORMAT_COLOR: {
>+ if(user_vbi->redraw && user_vbi->on && user_vbi->canvas) { // redraw last
decoded page
>+ user_vbi->spudec_proc=1;
>+ user_vbi->redraw=0;
>+ } else if(user_vbi->act_pgno!=user_vbi->pgno && user_vbi->on) { // modify
header
>+ int csize = page.columns * page.rows * 12 * 10 * sizeof(vbi_rgba);
>+ void *canvas=malloc(csize);
>+ if(csize > user_vbi->canvas_size) { // test canvas size
>+ if(user_vbi->canvas) free(user_vbi->canvas);
>+ user_vbi->canvas=malloc(csize);
>+ memset(user_vbi->canvas,0,csize);
>+ user_vbi->canvas_size=0;
>+ if(user_vbi->canvas) user_vbi->canvas_size=csize;
>+ }
>+ vbi_draw_vt_page(&(page), // vbi_page to RGBA32_LB image
>+ user_vbi->fmt,
>+ canvas,
>+ user_vbi->reveal,
>+ user_vbi->flash_on);
>+ int hsize = page.columns * 12 * 10 * sizeof(vbi_rgba);

Mixed declaration and code.

>[skip]
>+/*
>+// init: device; text|bw|gray|color; first page number
>+*/
>+priv_vbi_t* vbi_init(char *tdevice, char *tformat, char *tpage)
>+{
>+if(inited) return NULL;
>+unsigned int services =
>+ VBI_SLICED_TELETEXT_B | \
>+ VBI_SLICED_CAPTION_525 | \
>+ VBI_SLICED_CAPTION_625 | \
>+ VBI_SLICED_VBI_525 | \
>+ VBI_SLICED_VBI_625 | \
>+ VBI_SLICED_WSS_625 | \
>+ VBI_SLICED_WSS_CPR1204 | \
>+ VBI_SLICED_VPS;
>+int formatid=VBI_TFORMAT_TEXT; // default
>+int startpage=0;

Mixed declaration and code.
Will be rejected.

>[skip]
>+/*
>+// Teletext on/off
>+*/
>+void vbi_switch(priv_vbi_t *priv_vbi, int flag)
>+{
>+pthread_mutex_lock(&(priv_vbi->buffer_mutex));
>+switch (flag) {
>+ case 0: priv_vbi->on=0; break;
>+ case 1: priv_vbi->on=1; break;

can be done as:
case 0:
case 1:
priv->vbi->on=flag;break;

But this does not realy matter.
Anyway possible flag values (including behaviour of "default" case
implementation above) should be documented.

>[skip]
>+/*
>+// Set opacity display mode (only in SPU mode)
>+*/
>+void vbi_switch_alpha(priv_vbi_t *priv_vbi, int flag)
>+{

Same as for vbi_switch.

>+/*
>+// Set half page mode (only in SPU mode)
>+// flag=0 : half mode off
>+// 1 : top half page
>+// 2 : bottom half page
>+*/
>+void vbi_switch_half(priv_vbi_t *priv_vbi, int flag)

Behaviour of "default" case section is not documented).

>+/*
>+// Set page number
>+*/
>+void vbi_setpage(priv_vbi_t *priv_vbi, int pgno, int subno)
>+{
>+pthread_mutex_lock(&(priv_vbi->buffer_mutex));
>+priv_vbi->pgno=steppage(0,pgno);
>+priv_vbi->subno=subno;
>+priv_vbi->act_subno=0;
>+memset(priv_vbi->pages,0,sizeof(priv_vbi->pages));
>+memset(priv_vbi->valid_pages,0,sizeof(priv_vbi->valid_pages));
>+pthread_mutex_unlock(&(priv_vbi->buffer_mutex));
>+}
>+
>+/*
>+// Increase or decrease page
>+*/
>+void vbi_steppage(priv_vbi_t *priv_vbi, int direction)
>+{
>+pthread_mutex_lock(&(priv_vbi->buffer_mutex));
>+priv_vbi->pgno=steppage(priv_vbi->pgno,direction);
>+priv_vbi->act_subno=0;
>+memset(priv_vbi->pages,0,sizeof(priv_vbi->pages));
>+memset(priv_vbi->valid_pages,0,sizeof(priv_vbi->valid_pages));
>+pthread_mutex_unlock(&(priv_vbi->buffer_mutex));
>+}

Why does clearing of all pages performed? Is changing current page index
not enough?
Each page switch cause rescan for all pages (~20 seconds for me).
IMHO, caching them will be much more preferred.
If background rescanning is hard to implement add slave command for
rescanning.
Of course, this is just IMHO.

>+/*
>+// Get current page text (only in text mode)
>+*/
>+char* vbi_getpagetext(priv_vbi_t *priv_vbi)
>+{
>+if(!priv_vbi->on) return NULL;
>+if(priv_vbi->tformat!=VBI_TFORMAT_TEXT && priv_vbi->canvas) return NULL;
>+char* page = NULL;

Mixed declaration and code. Will be rejected.

>+pthread_mutex_lock(&(priv_vbi->buffer_mutex));
>+if (priv_vbi->valid_pages[priv_vbi->act_subno])
>+ page=priv_vbi->txtpages+(priv_vbi->act_subno*VBI_TXT_PAGE_SIZE);

if VBI_TXT_PAGE_SIZE is a multiple of 4 (as it does, otherwise additional
pgragna will required) defining txtpages as
structure array can help a bit.
typedef struct{
char text[VBI_TXT_PAGE_SIZE];
} textpage_s;

in priv_vbi_t: textpage_s* txtpages;
page=(char*)priv_vbi->txtpages[priv_vbi->act_subno];
Just IMHO.

>[skip]
>+/*
>+// Get current page RGBA32 image (only in SPU mode)
>+*/
>+tv_teletext_img_t* vbi_getpageimg(priv_vbi_t *priv_vbi)
>+{
>+if(priv_vbi->tformat==VBI_TFORMAT_TEXT) return NULL;
>+if(priv_vbi->spudec_proc==0) return NULL;
>+tv_teletext_img_t* img = NULL;

Mixed declaration and code. Will be rejected.


Please, also make comments doxygen-compatible
(/** */ - multiline comment block with two stars at beginnning, or /// for
one-line comment).
And fix  mixed spaces and tabs in code indentation.


-- 
Regards,
Vladimir Voroshilov     mailto:voroshil at gmail.com
JID: voroshil at jabber.ru
ICQ: 95587719