[vlc-devel] [PATCH] Improved support for USF subtitles
Christopher Gundler
c.gundler at mail.de
Fri Jun 16 10:51:47 CEST 2017
Hello Rémi,
unfortunately, I was unable to find the "glaring invalid free" you mentioned. Do you mean the usage of "video_format_Clean"?
It was introduced in response to the first review by Filip Roséen (https://mailman.videolan.org/pipermail/vlc-devel/2017-May/113005.html) exactly in the manner like in the already existing method "CreateTextRegion".
According to the "subpicture.c", the "video_format_t" get copied in case of "VLC_CODEC_YUVP" and should be therefore safe to use.
I hope that I'm not missing another bug you mean.
Christopher**********
On May 29, 2017 1:48:54 PM GMT+03:00, Christopher Gundler<c.gundler at mail.de> wrote:
> From: Julius Schoening<julius.schoening at uni-osnabrueck.de>
>
> Hello Filip,
> thank you for your review of the patch. I have improve the code (and
> change the author to Julius) and re-sent it now that you might have a
> second look on it.
>
> Best Regards,
> Christopher
>
> ---
> modules/codec/subsusf.c | 338
> ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 338 insertions(+)
>
> diff --git a/modules/codec/subsusf.c b/modules/codec/subsusf.c
> index 1a94b6b4fd..a2f3dbdd8f 100644
> --- a/modules/codec/subsusf.c
> +++ b/modules/codec/subsusf.c
> @@ -24,6 +24,7 @@
> # include "config.h"
> #endif
> #include <assert.h>
> +#include <limits.h>
>
> #include <vlc_common.h>
> #include <vlc_plugin.h>
> @@ -821,7 +822,166 @@ 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 *CreatePaintRegion( int x, int y, int
> width, int height )
> +{
> + 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 );
> + 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 );
> + video_format_Clean( &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 int ParseInteger(char *str)
> +{
> + long int value = strtol( str, NULL, 10 );
> + if ( value >= INT_MIN && value <= INT_MAX)
> + {
> + return (int) value;
> + }
> + else if( value < INT_MIN )
> + {
> + return INT_MIN;
> + }
> + else
> + {
> + return INT_MAX;
> + }
> +}
>
> static subpicture_region_t *ParseUSFString( decoder_t *p_dec,
> char *psz_subtitle )
> @@ -917,6 +1077,184 @@ 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 )))
> + {
> + psz_end = strstr ( psz_subtitle, "</polygon>" );
> +
> + polydata *p_root = NULL;
> + polydata **pp_next = &p_root;
> +
> + char *psz_point_start, *psz_point_end = NULL;
> +
> + // Iterate thought the points of the polygon by moving
> the pointer.
> + // For performance and readability reasons, the
> assignments are exceptionally made in the condition part.
> + for(char *data = psz_subtitle;
> + data < psz_end && ( psz_point_start = strstr( data,
> "<point " ) ) != NULL && ( psz_point_end = strstr( psz_point_start,
> "/>" ) ) != NULL;
> + data = psz_point_end + 2)
> + {
> + char *psz_point_x = GrabAttributeValue( "posx",
> psz_point_start );
> + char *psz_point_y = GrabAttributeValue( "posy",
> psz_point_start );
> + *pp_next = malloc( sizeof( polydata ) );
> +
> + if ( psz_point_x && psz_point_y && pp_next )
> + {
> + ( *pp_next )->x = ParseInteger( psz_point_x );
> + ( *pp_next )->y = ParseInteger( psz_point_y );
> + pp_next = &( ( *pp_next )->next );
> + }
> + else
> + {
> + pp_next = NULL;
> + }
> +
> + free( psz_point_x );
> + free( psz_point_y );
> + }
> +
> + if ( pp_next )
> + {
> + // create and draw picture region
> + subpicture_region_t *p_paint_region;
> + p_paint_region = CreatePaintRegion( 0, 0,
> p_sys->i_original_width + 1, p_sys->i_original_height + 1 );
> +
> + if( p_paint_region != NULL )
> + {
> + 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;
> + p_paint_region = CreatePaintRegion( 0, 0,
> p_sys->i_original_width + 1, p_sys->i_original_height + 1 );
> +
> + if( p_paint_region != NULL )
> + {
> + // convert to int
> + int x = ParseInteger( psz_rec_x );
> + int y = ParseInteger( psz_rec_y );
> + int w = ParseInteger( psz_rec_width );
> + int h = ParseInteger( psz_rec_height );
> +
> + 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;
> + }
> + }
> + }
> +
> + free( psz_rec_x );
> + free( psz_rec_y );
> + free( psz_rec_width );
> + free( psz_rec_height );
> + }
> + 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;
> + p_paint_region = CreatePaintRegion( 0, 0,
> p_sys->i_original_width + 1, p_sys->i_original_height + 1 );
> +
> + if( p_paint_region != NULL )
> + {
> + // convert to int
> + int x = ParseInteger( psz_point_x );
> + int y = ParseInteger( psz_point_y );
> + int d = ParseInteger( psz_point_d );
> +
> + 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;
> + }
> + }
> + }
> +
> + free( psz_point_x );
> + free( psz_point_y );
> + free( psz_point_d );
> + }
> + 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;
> --
> 2.12.2
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
> There is a glaring invalid free in the first few lines. I stopped reviewing there.
> -- > Rémi Denis-Courmont
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170616/69f5b621/attachment.html>
More information about the vlc-devel
mailing list