[MPlayer-dev-eng] [PATCH] USF support - second time around NOVIRUS

[Attila Kinali]

On Sun, 14 Dec 2003 04:38:41 -0500
Avery Morrow <ashitaka-san at comcast.net> wrote:

> I have improved on my patch for USF support. It fixes
> everything that Moritz pointed out, and now supports XML more
> fully and displays multi-line subtitles properly. I tested it
> with a somewhat complex USF subtitle file which I've made, and
> it works perfectly. However, there still might be some
> Schroedinbug I didn't see.

diff -Naur 1/subreader.c 2/subreader.c
--- 1/subreader.c	2003-12-14 04:11:35.792026584 -0500
+++ 2/subreader.c	2003-12-14 04:11:23.621876728 -0500
@@ -262,6 +262,83 @@
     return current;
 }
 
+subtitle *sub_read_line_usf(FILE *fd,subtitle *current) {
+	char line[LINE_LEN+1];
+	int i, j, count, skip, start, a1, a2, a3, a4, b1, b2, b3, b4;
+	char *p=NULL;
+	count = 0;
+
+while (!current->text[0]) {
+	while (!strstr(line, "<subtitle ")) {
+		if(!fgets(line, LINE_LEN, fd))
+			return NULL;
+	}
+	if (sscanf(line, " <subtitle start=\"%d:%d:%d.%d\" stop=\"%d:%d:%d.%d\"", &a1, &a2, &a3, &a4, &b1, &b2, &b3, &b4) != 8) {
+		mp_msg(MSGT_SUBREADER,MSGL_WARN,"USF decoding doesn't parse XML correctly.\n"); //TODO: allow more than one line per time marker
+		break;
+	}
+	current->start = a1*360000+a2*6000+a3*100+a4/10;
+	current->end   = b1*360000+b2*6000+b3*100+b4/10;
+	if (current->end <= current->start)
+	  current->end = current->start+200;
+	while (!(p = strstr(line, "<text"))) {
+		if(!fgets(line, LINE_LEN, fd))
+			return NULL;
+	}
+	int len = 0, addline = 2;
+	// detects </text
+	p = &line[0];
+	for (;!(*p=='<' && *(p+1)=='/' && *(p+2)=='t' && *(p+3)=='e' && *(p+4)=='x' && *(p+5)=='t') && *p; p++,len++) {
+		if (*p=='\n' || *p=='\r') { 
+		char tmp[LINE_LEN+1];

This is imho a bad idea. This allocates 1KB on the stack 
and might lead to stack overflows.

[...]
+				char temp[LINE_LEN+1];

Another KB on the stack.

+				strncpy(temp, current->text[count], curptr-&(current->text[count][0]));
+				temp[curptr-&(current->text[count][0])] = '\0';
+				current->text[count]=(char *)malloc(j-1);

You should do a realloc here -> memleak

+				strcpy(current->text[count], temp);
+				count++;
+			        curptr=current->text[count]=(char *)malloc (len-j);
+		    	}
+			continue;
+		    }
+		    if(skip || eol(line[j]))
+			continue;

What's the intention of the check for eol ?
You know that eol also matches for '\0' ?

+		    if(line[j] == '\t') {
+		    	if(line[j-1] == ' ')
+				continue;
+			else
+		    		line[j] = ' ';
+			}
+		     *curptr=line[j];
+		    curptr++;
+		}
+		*curptr='\0';
+	count++;

There should be a check whether count reached SUB_MAX_TEXT somewhere in this loop.
-> sig11

+}
+    current->lines=count;
+    return current;
+}
+
 subtitle *sub_read_line_subviewer(FILE *fd,subtitle *current) {
     char line[LINE_LEN+1];
     int a1,a2,a3,a4,b1,b2,b3,b4;
@@ -923,10 +1000,14 @@
 		{*uses_time=1;return SUB_VPLAYER;}
 	if (sscanf (line, "%d:%d:%d ",     &i, &i, &i )==3)
 		{*uses_time=1;return SUB_VPLAYER;}
+	// If it's XML, it's USF... this is just as bad as the RT detection
+	if (strstr(line, "<?xml"))
+		{*uses_time=1;return SUB_USF;}

IMHO sub_autodetect() should be reworked to use multiple lines for
detection, but that's something that could be done later.

 	//TODO: just checking if first line of sub starts with "<" is WAY
 	// too weak test for RT
 	// Please someone who knows the format of RT... FIX IT!!!
 	// It may conflict with other sub formats in the future (actually it doesn't)
+	// ... it doesn't only if it comes last
 	if ( *line == '<' )
 		{*uses_time=1;return SUB_RT;}

------------------

Otherwise the patch looks ok and could be applied if those few things
are fixed.


				Attila Kinali




-- 
egp ist vergleichbar mit einem ikea bausatz fuer flugzeugtraeger
			-- reeler in +kaosu