<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<meta http-equiv="Content-Style-Type" content="text/css" />
<meta name="generator" content="pandoc" />
<title></title>
<style type="text/css">code{white-space: pre;}</style>
</head>
<body>
<p>Hi Christopher,</p>
<p>I am currently running a fever, so please let me know if something I have written is not clear and warrants further explanation.</p>
<p>On 2017-04-20 15:14, Christopher Gundler wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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 )</code></pre>
</blockquote>
<p>As you can see, all other <code>static</code> functions in this translation-unit use <code>CamelCase</code> (and not <code>camelCase</code>). If possible, as it certainly is in this case, please follow the already present naming-scheme.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +{
+ 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 );</code></pre>
</blockquote>
<p>Usage of <code>video_format_Init</code> should preferrably have a corresponding <code>video_format_Clean</code>. I am aware of the fact that it is currently not documented, and strictly speaking not required (as <code>video_format_Init</code> does not allocate any resource) - but better safe than sorry.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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>" );</code></pre>
</blockquote>
<p>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).</p>
<p>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 <em>undefined-behavior</em>.</p>
<p>The below is, as an example, strictly speaking not ok:</p>
<pre><code>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 */</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+ // 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 ) )</code></pre>
</blockquote>
<p>We normally try to refrain from having assignments within if-conditions, and the usage of <code>calloc</code> is unnecessary since you are always writing to the relevant data-members of the object referred to by<code>*pp_next</code>.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + {
+ ( *pp_next )->x = (int)( strtol( psz_point_x, NULL, 10 ) );
+ ( *pp_next )->y = (int)( strtol( psz_point_y, NULL, 10 ) );</code></pre>
</blockquote>
<p>As <code>strtol</code> returns a <code>long int</code>, there is a potential signed integer overflow in the above two expressions.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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 );</code></pre>
</blockquote>
<p>The equivalent of <code>free( NULL )</code> is well-defined.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+ // get end of point and continue the search from there on
+ psz_point_end = strstr(psz_point_start, "/>") + 2;</code></pre>
</blockquote>
<p>Is it really guaranteed that you will find the relevant needle within the buffer referred to by <code>psz_point_start</code>?</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + }
+
+ 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;</code></pre>
</blockquote>
<p><code>video_width</code> and <code>video_height</code> 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>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + p_paint_region = newPaintRegion( 0, 0, video_width + 1, video_height + 1 );</code></pre>
</blockquote>
<p><code>newPaintRegion</code> might return <code>NULL</code>, leading to a null-pointer dereference in <code>DrawPoly</code>.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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 ) );</code></pre>
</blockquote>
<p>See previous remark regarding <code>strtol</code>.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+ 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 );</code></pre>
</blockquote>
<p>See previous remark regarding <code>free( NULL )</code>.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + }
+ 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 );</code></pre>
</blockquote>
<p>See previous remark regarding <code>newPaintRegion</code> and its return-value.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+ // 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 );</code></pre>
</blockquote>
<p>See previous remark regarding <code>free( NULL )</code>.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + }
+ 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;</code></pre>
</blockquote>
<p>Best Regards,<br />
Filip</p>
</body>
</html>