[MPlayer-dev-eng] [PATCH] TV teletext
Vladimir Voroshilov
voroshil at gmail.com
Thu Mar 22 10:59:45 CET 2007
Hi, Ötvös
Havn't read carefully tvi_vbi.c yet.
I cant also review spu and osd related code.
Unfortunately, i also cant test functionality of this patch - broadcast
channels
in my area does not contain any teletext info.
Here is a result of first quick look into the patch.
>[skip]
>--- cfg-common.h (revision 22772)
>+++ cfg-common.h (working copy)
>@@ -460,6 +460,11 @@
> #endif
> {"adevice", &tv_param_adevice, CONF_TYPE_STRING, 0, 0, 0, NULL},
> #endif
>+#ifdef HAVE_TV_TELETEXT
>+ {"tdevice", &tv_param_tdevice, CONF_TYPE_STRING, 0, 0, 0, NULL},
>+ {"tformat", &tv_param_tformat, CONF_TYPE_STRING, 0, 0, 0, NULL},
>+ {"tpage", &tv_param_tpage, CONF_TYPE_STRING, 0, 0, 0, NULL},
Why does this is CONF_TYPE_STRING rather than CONF_TYPE_INT ?
There is a comment later in your patch which says that tpage is "first page
number".
>--- command.c (revision 22772)
>+++ command.c (working copy)
>@@ -1403,6 +1526,21 @@
> M_OPT_RANGE, -100, 100, (void *) TV_COLOR_HUE },
> #endif
>
>+#ifdef HAVE_TV_TELETEXT
>+ { "vbi_switch", mp_property_vbi_switch, CONF_TYPE_FLAG,
>+ M_OPT_RANGE, 0, 1, NULL },
>+ { "vbi_switch_alpha", mp_property_vbi_alpha_page, CONF_TYPE_FLAG,
>+ M_OPT_RANGE, 0, 1, NULL },
>+ { "vbi_half_page", mp_property_vbi_half_page, CONF_TYPE_FLAG,
>+ M_OPT_RANGE, 0, 2, NULL },
>+ { "vbi_flip_page", mp_property_vbi_flip_page, CONF_TYPE_FLAG,
>+ M_OPT_RANGE, 0, 1, NULL },
>+ { "vbi_step_page", mp_property_vbi_step_page, CONF_TYPE_INT,
>+ M_OPT_RANGE, -999, 999, NULL },
>+ { "vbi_set_page", mp_property_vbi_set_page, CONF_TYPE_INT,
>+ M_OPT_RANGE, 100, 999, NULL },
>+#endif
>+
>[skip]
>@@ -1500,6 +1638,14 @@
> { "tv_saturation", MP_CMD_TV_SET_SATURATION, 0, OSD_SATURATION, -1,
MSGTR_Saturation },
> { "tv_contrast", MP_CMD_TV_SET_CONTRAST, 0, OSD_CONTRAST, -1,
MSGTR_Contrast },
> #endif
>+#ifdef HAVE_TV_TELETEXT
>+ { "vbi_switch", MP_CMD_VBI_SWITCH, 1, 0, -1, NULL },
>+ { "vbi_switch_alpha", MP_CMD_VBI_ALPHA_PAGE, 1, 0, -1, NULL },
>+ { "vbi_half_page", MP_CMD_VBI_HALF_PAGE, 1, 0, -1, NULL },
>+ { "vbi_set_page", MP_CMD_VBI_SET_PAGE, 0, 0, -1, NULL },
>+ { "vbi_step_page", MP_CMD_VBI_STEP_PAGE, 1, 0, -1, NULL },
>+ { "vbi_flip_page", MP_CMD_VBI_FLIP_PAGE, 1, 0, -1, NULL },
>+#endif
> { NULL, 0, 0, 0, -1, NULL }
> };
What are these mp_property_* used for ?
Why do you define vbi_* as properties if you define vbi_* slave commands
later.
>@@ -158,6 +174,16 @@
> #define TVI_CONTROL_SPC_SET_INPUT 0x402 /* set input channel
(tv,s-video,composite..) */
> #define TVI_CONTROL_SPC_GET_NORMID 0x403 /* get normid from norm
name */
>
>+/* TELETEX controls */
>+#define TVI_CONTROL_VBI_SWITCH 0x501 /* on/off garb teletext */
>+#define TVI_CONTROL_VBI_SET_PAGE 0x502 /* set garb teletext page
number */
>+#define TVI_CONTROL_VBI_STEP_PAGE 0x503 /* set garb teletext page
number */
>+#define TVI_CONTROL_VBI_GET_PAGE 0x504 /* get garbbed teletext page
*/
>+#define TVI_CONTROL_VBI_ALPHA_PAGE 0x505 /* switch alpha */
>+#define TVI_CONTROL_VBI_HALF_PAGE 0x506 /* switch half page */
>+#define TVI_CONTROL_VBI_GET_TXTPAGE 0x507 /* get garbbed text
teletext page */
>+#define TVI_CONTROL_VBI_GET_IMGPAGE 0x508 /* get garbbed image
teletext page */
>+
s/garb/grab/
>+#ifdef HAVE_TV_TELETEXT
>+ case TVI_CONTROL_VBI_SWITCH:
>+ if (!priv->priv_vbi) return TVI_CONTROL_FALSE;
>+ vbi_switch(priv->priv_vbi, *(int *)arg);
>+ return TVI_CONTROL_TRUE;
>+ case TVI_CONTROL_VBI_ALPHA_PAGE:
>+ if (!priv->priv_vbi) return TVI_CONTROL_FALSE;
>+ vbi_switch_alpha(priv->priv_vbi, *(int *)arg);
>+ return TVI_CONTROL_TRUE;
>+ case TVI_CONTROL_VBI_HALF_PAGE:
>+ if (!priv->priv_vbi) return TVI_CONTROL_FALSE;
>+ vbi_switch_half(priv->priv_vbi, *(int *)arg);
>+ return TVI_CONTROL_TRUE;
>+ case TVI_CONTROL_VBI_SET_PAGE:
>+ if (!priv->priv_vbi) return TVI_CONTROL_FALSE;
>+ vbi_setpage(priv->priv_vbi, *(int *)arg, 0);
>+ return TVI_CONTROL_TRUE;
>+ case TVI_CONTROL_VBI_STEP_PAGE:
>+ if (!priv->priv_vbi) return TVI_CONTROL_FALSE;
>+ vbi_steppage(priv->priv_vbi, *(int *)arg);
>+ return TVI_CONTROL_TRUE;
>+ case TVI_CONTROL_VBI_GET_PAGE:
>+ if (!priv->priv_vbi) return TVI_CONTROL_FALSE;
>+ vbi_page *page = NULL;
>+ if (NULL==(page=vbi_getpage(priv->priv_vbi)))
>+ return TVI_CONTROL_FALSE;
>+ *(void **)arg = (void*)page;
>+ return TVI_CONTROL_TRUE;
>+ case TVI_CONTROL_VBI_GET_TXTPAGE:
>+ if (!priv->priv_vbi) return TVI_CONTROL_FALSE;
>+ char *txtpage = NULL;
>+ if (NULL==(txtpage=vbi_getpagetext(priv->priv_vbi)))
>+ return TVI_CONTROL_FALSE;
>+ *(void **)arg = (void*)txtpage;
>+ return TVI_CONTROL_TRUE;
>+ case TVI_CONTROL_VBI_GET_IMGPAGE:
>+ if (!priv->priv_vbi) return TVI_CONTROL_FALSE;
>+ tv_teletext_img_t *img = NULL;
>+ if (NULL==(img=vbi_getpageimg(priv->priv_vbi)))
>+ return TVI_CONTROL_FALSE;
>+ *(void **)arg = (void*)img;
>+ return TVI_CONTROL_TRUE;
>+#endif
Spaces and tabs are messed here.
Will below code be more easy to undestand (same for tvi_v4l2)?
This will hide all vbi-specific code in tvi_vbi.c
------------------begin-----------------
#ifdef HAVE_TV_TELETEXT
case TVI_CONTROL_VBI_SWITCH:
case TVI_CONTROL_VBI_ALPHA_PAGE:
case TVI_CONTROL_VBI_HALF_PAGE:
case TVI_CONTROL_VBI_SET_PAGE:
case TVI_CONTROL_VBI_STEP_PAGE:
case TVI_CONTROL_VBI_GET_PAGE:
case TVI_CONTROL_VBI_GET_TXTPAGE:
case TVI_CONTROL_VBI_GET_IMGPAGE:
if (!priv->priv_vbi) return TVI_CONTROL_FALSE;
return vbi_control(ptiv->priv_vbi,command,arg);
#endif
-------------------end-----------------
>--- stream/tvi_v4l2.c (revision 22772)
>+++ stream/tvi_v4l2.c (working copy)
>@@ -131,6 +134,12 @@
> volatile long audio_null_blocks_inserted;
> volatile long long dropped_frames_timeshift;
> long long dropped_frames_compensated;
>+#ifdef HAVE_TV_TELETEXT
>+ char *tdevice;
>+ char *tformat;
>+ char *tpage;
>+ priv_vbi_t *priv_vbi;
>+#endif
> } priv_t;
Why are you keeping this values here (in tvi's priv_t rather than vbi's) ??
Seems like it is used only in init.
>@@ -1116,7 +1189,10 @@
> set_control(priv, &control, 0);
> }
> }
>-
>+#ifdef HAVE_TV_TELETEXT
>+ if(priv->tdevice && priv->priv_vbi==NULL)
>+ priv->priv_vbi=vbi_init(priv->tdevice,priv->tformat,priv->tpage);
>+#endif
Hm. I thought, init should be called only once.
Why are you checking priv->priv_vbi here and doesn't do it in tvi_v4l.c ?
>--- 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;
1. Note, that this will cause tv subsystem to force mplayer exit
immediately.
2. (cosmetics, same for all code below) Routines with missed/wrong
indentation in body
are dificult to read, imho. (i'm prefer 4-space indentation ;) ).
>[skip]
> +// Close vbi capture, decodeand and free priv_vbi
s/decodeand/decode/
--
Regards,
Vladimir Voroshilov mailto:voroshil at gmail.com
JID: voroshil at jabber.ru
ICQ: 95587719
More information about the MPlayer-dev-eng
mailing list