[vlc-devel] [PATCH] This patch improves the support for the USF subtitle format.
Filip Roséen
filip at atch.se
Wed May 10 15:39:05 CEST 2017
Hi Christopher,
I am currently running a fever, so please let me know if something I
have written is not clear and warrants further explanation.
On 2017-04-20 15:14, Christopher Gundler wrote:
> This patch introduces the support for parsing rects, circles, and
> polygons from this type of subtitle format and was developed in the
> context of research ("Providing Video Annotations in Multimedia
> Containers for Visualization and Research.") by Julius Schoening,
> University of Osnabrueck, Germany.
> ---
> modules/codec/subsusf.c | 322 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 322 insertions(+)
>
> diff --git a/modules/codec/subsusf.c b/modules/codec/subsusf.c
> index 1a94b6b4fd..14acc884ad 100644
> --- a/modules/codec/subsusf.c
> +++ b/modules/codec/subsusf.c
> @@ -821,7 +821,147 @@ static void ParseUSFHeaderTags( decoder_t *p_dec, xml_reader_t *p_xml_reader )
> free( p_ssa_style );
> }
>
> +/**
> + * Create a region with a white transparent picture.
> + */
> +static subpicture_region_t *newPaintRegion( int x, int y, int width, int height )
As you can see, all other `static` functions in this translation-unit
use `CamelCase` (and not `camelCase`). If possible, as it certainly is
in this case, please follow the already present naming-scheme.
> +{
> + video_palette_t palette = {
> + .i_entries = 2,
> + .palette = {
> + [0] = { 0xff, 0x80, 0x80, 0x00 },
> + [1] = { 0xff, 0x80, 0x80, 0xff },
> + },
> + };
> +
> + video_format_t fmt;
> + video_format_Init( &fmt, VLC_CODEC_YUVP );
Usage of `video_format_Init` should preferrably have a corresponding
`video_format_Clean`. I am aware of the fact that it is currently not
documented, and strictly speaking not required (as `video_format_Init`
does not allocate any resource) - but better safe than sorry.
> + fmt.i_width =
> + fmt.i_visible_width = width;
> + fmt.i_height =
> + fmt.i_visible_height = height;
> + fmt.i_sar_num = 1;
> + fmt.i_sar_den = 1;
> + fmt.p_palette = &palette;
> +
> + subpicture_region_t *r = subpicture_region_New( &fmt );
> + if ( !r )
> + return NULL;
> + r->i_x = x;
> + r->i_y = y;
> + memset( r->p_picture->p->p_pixels, 0, r->p_picture->p->i_pitch * height );
> +
> + return r;
> +}
> +
> +/**
> + * Draw a rectangle into a paint region
> + */
> +static void DrawRect( subpicture_region_t *r,
> + int x1, int y1, int x2, int y2 )
> +{
> + uint8_t *p = r->p_picture->p->p_pixels;
> + int pitch = r->p_picture->p->i_pitch;
> +
> + for ( int y = y1; y <= y2; y++ )
> + {
> + p[x1 + pitch * y] = 1;
> + p[x2 + pitch * y] = 1;
> + }
> +
> + for ( int x = x1; x <= x2; x++ ) {
> + p[x + pitch * y1] = 1;
> + p[x + pitch * y2] = 1;
> + }
> +}
> +
> +struct polydata {
> + int x;
> + int y;
> + struct polydata *next;
> +};
> +typedef struct polydata polydata;
> +
> +static void DrawPoly( subpicture_region_t *r, polydata *root )
> +{
> + uint8_t *pY = r->p_picture->p[Y_PLANE].p_pixels;
> + int pitchY = r->p_picture->p[Y_PLANE].i_pitch;
> +
> + polydata *curr = root;
> +
> + do
> + {
> + // if end of list, set next to root for closed polygon
> + polydata *next = curr->next == NULL ? root : curr->next;
> +
> + int x0 = curr->x;
> + int y0 = curr->y;
> + int x1 = next->x;
> + int y1 = next->y;
> +
> + // bresenham's line algorithm
> + // from http://rosettacode.org/wiki/Bitmap/Bresenham%27s_line_algorithm#C
> + int dx = abs( x1 - x0 ), sx = x0 < x1 ? 1 : -1;
> + int dy = abs( y1 - y0 ), sy = y0 < y1 ? 1 : -1;
> + int err = ( dx > dy ? dx : -dy ) / 2, e2;
> +
> + for( ; ; )
> + {
> + pY[ x0 + pitchY * y0 ] = 1;
> + if ( x0 == x1 && y0 == y1 )
> + break;
> +
> + e2 = err;
> +
> + if ( e2 >- dx )
> + {
> + err -= dy;
> + x0 += sx;
> + }
> +
> + if ( e2 < dy )
> + {
> + err += dx;
> + y0 += sy;
> + }
> + }
> + }
> + while( ( curr = curr->next ) != NULL );
> +}
> +
> +static void DrawCircle( subpicture_region_t *r, int x0, int y0, int diameter )
> +{
> + uint8_t *pY = r->p_picture->p[Y_PLANE].p_pixels;
> + int pitchY = r->p_picture->p[Y_PLANE].i_pitch;
>
> + // bresenham's circle algorithm
> + // slightly optimized from https://de.wikipedia.org/wiki/Bresenham-Algorithmus#Kreisvariante_des_Algorithmus
> + int x = diameter / 2;
> + int y = 0;
> + int err = x;
> +
> + while( y <= x )
> + {
> + pY[ ( x0 + x ) + pitchY * ( y0 + y ) ] = 1;
> + pY[ ( x0 - x ) + pitchY * ( y0 + y ) ] = 1;
> + pY[ ( x0 + x ) + pitchY * ( y0 - y ) ] = 1;
> + pY[ ( x0 - x ) + pitchY * ( y0 - y ) ] = 1;
> + pY[ ( x0 + y ) + pitchY * ( y0 + x ) ] = 1;
> + pY[ ( x0 - y ) + pitchY * ( y0 + x ) ] = 1;
> + pY[ ( x0 + y ) + pitchY * ( y0 - x ) ] = 1;
> + pY[ ( x0 - y ) + pitchY * ( y0 - x ) ] = 1;
> + err -= 2 * y;
> + err++;
> + y++;
> +
> + if(err < 0)
> + {
> + err--;
> + err += 2*x;
> + x--;
> + }
> + }
> +}
>
> static subpicture_region_t *ParseUSFString( decoder_t *p_dec,
> char *psz_subtitle )
> @@ -917,6 +1057,188 @@ static subpicture_region_t *ParseUSFString( decoder_t *p_dec,
> p_region_upto = p_region_upto->p_next;
> }
> }
> + else if(( !strncasecmp( psz_subtitle, "<polygon ", 9 )) ||
> + ( !strncasecmp( psz_subtitle, "<polygon>", 9 )))
> + {
> + // get the end of the current polygon
> + psz_end = strstr ( psz_subtitle, "</polygon>" );
This is of course not the direct fault of this patch, but "parsing"
XML by simply looking for the closest end-tag, in this case
"</polygon>", will not reject malformed input (as no tag-verification
is done).
It is also worth noting that using relational-operators to compare two
pointers that are not part of the same array (including the element
one past the end), is *undefined-behavior*.
The below is, as an example, strictly speaking not ok:
int a1[3];
int a2[3];
int* p1 = &a1[1];
int* p2 = &a2[2];
int* p3 = NULL;
p1 > p2; /* undefined behavior */
p3 < p1; /* undefined behavior */
p1 == p2; /* ok */
p2 != p3; /* ok */
> +
> + // initialize stuff
> + polydata *p_root = NULL;
> + polydata **pp_next = &p_root;
> +
> + // starting position of next point
> + char *psz_point_start, *psz_point_end = NULL;
> +
> + // check if next point is found and in current polygon
> + for ( psz_point_start = strstr( psz_subtitle, "<point " );
> + pp_next &&
> + psz_point_start && psz_point_start < psz_end ;
> + psz_point_start = psz_point_end
> + ? strstr( psz_point_end, "<point " ) : NULL )
> + {
> + // read attributes of points
> + char *psz_point_x = GrabAttributeValue( "posx", psz_point_start );
> + char *psz_point_y = GrabAttributeValue( "posy", psz_point_start );
> +
> + if ( psz_point_x && psz_point_y &&
> + ( ( *pp_next = calloc( 1, sizeof( polydata ) ) ) != NULL ) )
We normally try to refrain from having assignments within
if-conditions, and the usage of `calloc` is unnecessary since you are
always writing to the relevant data-members of the object referred to
by`*pp_next`.
> + {
> + ( *pp_next )->x = (int)( strtol( psz_point_x, NULL, 10 ) );
> + ( *pp_next )->y = (int)( strtol( psz_point_y, NULL, 10 ) );
As `strtol` returns a `long int`, there is a potential signed integer
overflow in the above two expressions.
> + pp_next = &( ( *pp_next )->next );
> + }
> + else
> + {
> + pp_next = NULL;
> + }
> +
> + if ( psz_point_x ) free( psz_point_x );
> + if ( psz_point_y ) free( psz_point_y );
The equivalent of `free( NULL )` is well-defined.
> +
> + // get end of point and continue the search from there on
> + psz_point_end = strstr(psz_point_start, "/>") + 2;
Is it really guaranteed that you will find the relevant needle within
the buffer referred to by `psz_point_start`?
> + }
> +
> + if ( pp_next )
> + {
> + // create and draw picture region
> + subpicture_region_t *p_paint_region;
> + int video_width = p_sys->i_original_width;
> + int video_height = p_sys->i_original_height;
`video_width` and `video_height` were probably introduced to shorten
the line that follows, but since they are only used once I do not see
why they are required (as it does not aid code-readability).
> + p_paint_region = newPaintRegion( 0, 0, video_width + 1, video_height + 1 );
`newPaintRegion` might return `NULL`, leading to a null-pointer
dereference in `DrawPoly`.
> + DrawPoly( p_paint_region, p_root );
> +
> + // save picture region in list
> + if( !p_region_first )
> + {
> + p_region_first = p_region_upto = p_paint_region;
> + }
> + else if( p_paint_region )
> + {
> + p_region_upto->p_next = p_paint_region;
> + p_region_upto = p_region_upto->p_next;
> + }
> + }
> +
> + // free linked list
> + while( p_root )
> + {
> + polydata *p_tmp = p_root;
> + p_root = p_root->next;
> + free(p_tmp);
> + }
> + }
> + else if ( !strncasecmp( psz_subtitle, "<rectangle ", 11 ))
> + {
> + // get the end of the rectangle
> + psz_end = strstr ( psz_subtitle,"/>" );
> +
> + // read attributes of rectangle
> + char *psz_rec_x = GrabAttributeValue( "posx", psz_subtitle );
> + char *psz_rec_y = GrabAttributeValue( "posy", psz_subtitle );
> + char *psz_rec_width = GrabAttributeValue( "width", psz_subtitle );
> + char *psz_rec_height = GrabAttributeValue( "height", psz_subtitle );
> +
> + if ( psz_rec_x && psz_rec_y && psz_rec_width && psz_rec_height )
> + {
> + // create and draw picture region
> + subpicture_region_t *p_paint_region;
> + int video_width = p_sys->i_original_width;
> + int video_height = p_sys->i_original_height;
> + p_paint_region = newPaintRegion( 0, 0, video_width + 1, video_height + 1 );
> +
> + // convert to int
> + int x = (int)( strtol( psz_rec_x, NULL, 10 ) );
> + int y = (int)( strtol( psz_rec_y, NULL, 10 ) );
> + int w = (int)( strtol( psz_rec_width, NULL, 10 ) );
> + int h = (int)( strtol( psz_rec_height, NULL, 10 ) );
See previous remark regarding `strtol`.
> +
> + DrawRect( p_paint_region, x, y, x + w, y + h );
> +
> + // save picture region in list
> + if( !p_region_first )
> + {
> + p_region_first = p_region_upto = p_paint_region;
> + }
> + else if( p_paint_region )
> + {
> + p_region_upto->p_next = p_paint_region;
> + p_region_upto = p_region_upto->p_next;
> + }
> + }
> +
> + if ( psz_rec_x ) free( psz_rec_x );
> + if ( psz_rec_y ) free( psz_rec_y );
> + if ( psz_rec_width ) free( psz_rec_width );
> + if ( psz_rec_height ) free( psz_rec_height );
See previous remark regarding `free( NULL )`.
> + }
> + else if ( !strncasecmp( psz_subtitle, "<point ", 7 ) )
> + {
> + // get the end of the point
> + psz_end = strstr ( psz_subtitle, "/>" );
> +
> + // read attributes of point
> + char *psz_point_x = GrabAttributeValue( "posx", psz_subtitle );
> + char *psz_point_y = GrabAttributeValue( "posy", psz_subtitle );
> + char *psz_point_d = GrabAttributeValue( "diameter", psz_subtitle );
> +
> + if ( psz_point_x && psz_point_y && psz_point_d )
> + {
> + // create and draw picture region
> + subpicture_region_t *p_paint_region;
> + int video_width = p_sys->i_original_width;
> + int video_height = p_sys->i_original_height;
> + p_paint_region = newPaintRegion( 0, 0, video_width + 1, video_height + 1 );
See previous remark regarding `newPaintRegion` and its return-value.
> +
> + // convert to int
> + int x = (int)( strtol( psz_point_x, NULL, 10 ) );
> + int y = (int)( strtol( psz_point_y, NULL, 10 ) );
> + int d = (int)( strtol( psz_point_d, NULL, 10 ) );
> +
> + DrawCircle( p_paint_region, x, y, d );
> +
> + // save picture region in list
> + if( !p_region_first )
> + {
> + p_region_first = p_region_upto = p_paint_region;
> + }
> + else if( p_paint_region )
> + {
> + p_region_upto->p_next = p_paint_region;
> + p_region_upto = p_region_upto->p_next;
> + }
> + }
> +
> + if ( psz_point_x ) free( psz_point_x );
> + if ( psz_point_y ) free( psz_point_y );
> + if ( psz_point_d ) free( psz_point_d );
See previous remark regarding `free( NULL )`.
> + }
> + else if(( !strncasecmp( psz_subtitle, "<text ", 6 )) ||
> + ( !strncasecmp( psz_subtitle, "<text>", 6 )))
> + {
> + subpicture_region_t *p_text_region;
> + psz_end = strstr( psz_subtitle, "</text>" );
> + p_text_region = CreateTextRegion( p_dec,
> + psz_subtitle,
> + p_sys->i_align );
> +
> + if( p_text_region )
> + {
> + free( p_text_region->p_text->psz_text );
> + p_text_region->p_text->psz_text = CreatePlainText( psz_subtitle );
> + }
> +
> + if( !p_region_first )
> + {
> + p_region_first = p_region_upto = p_text_region;
> + }
> + else if( p_text_region )
> + {
> + p_region_upto->p_next = p_text_region;
> + p_region_upto = p_region_upto->p_next;
> + }
> + }
> else
> {
> subpicture_region_t *p_text_region;
Best Regards,\
Filip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170510/c2098d04/attachment.html>
More information about the vlc-devel
mailing list