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

[Attila Kinali]

Heyo,

Do you plan to fix those flaws, so we could
commit your USF patch ?
It would be nice to have it in pre4

			Attila Kinali


On Sat, 24 Jan 2004 12:28:10 +0100
Attila Kinali <attila at kinali.ch> wrote:

> 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
> 
> _______________________________________________
> MPlayer-dev-eng mailing list
> MPlayer-dev-eng at mplayerhq.hu
> http://mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
> 


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