[MPlayer-dev-eng] [patch] loading libass subtitles from network

[Reimar Döffinger]

On Thu, Feb 04, 2010 at 01:24:18AM +0300, Yuriy Kaminskiy wrote:
> Currently libass doesn't use mplayer stream infrastructure, and fails to load
> subtitles from network.
> This patch fixes that.

I'm too tired to review properly, but please get rid of all the #ifdef NETWORK
stuff, it only means having two different code paths to test without any real
advantage (unless your proposed patch has architectural issues?).

> Name (re)displayed later anyway (SUB:...), and displayed correctly with
> -identify, so I considered that not important to bother (fixing would need
> changing (upstream) libass, or dirty hackery in libass msg hook).

IMO: who cares

> If #define'ing ass_read_file feels too hackerish, squash second patch in (while
> being out-of-tree, less changed files - better ;-)).

I expect getting rid of the network dependency should make this unnecessary.
Does your patch work with old libass versions btw?
If not, you should at least update the configure check.

> +ass_track_t* ass_read_stream(ass_library_t* library, char *fname, char *charset) {

I suspect fname and charset should be const.

> +	fd=open_stream (fname, NULL, &i);

extra space before (

> +	if (fd->end_pos > STREAM_BUFFER_SIZE)
> +		/* read entrie file at one if size is known */

? At least two typos I think

> +		if (buf_alloc >= 10*1024*1024) {
> +			mp_msg(MSGT_ASS, MSGL_INFO, MSGTR_LIBASS_RefusingToLoadSubtitlesLargerThan10M, fname);
> +			sz = 0;
> +			break;

That on the other hand is the kind of limit I do not really like, does the current code have that also?

> +		if (buf) free(buf);

Useless if

> +	buf[sz] = '\0';

Use just 0, '\0' is just pointless clutter

> +		if(track->name)
> +			free(track->name);

Also pointless if