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

[Yuriy Kaminskiy]

On 04.02.2010 01:48, Reimar Döffinger wrote:
> 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?).

Only tiny bit more overhead (only on startup, should not matter). Agree, not
worth it.

>> 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?

It works with external libass.git, from quick test it seems work with internal
libass too; likely will work with even with older internal libass (before recent
major internal libass update).

> 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.
I just copied existing code ass_read_file prototype. Tried to change, but...

libass/ass_mp.c:319: warning: passing argument 1 of 'open_stream' discards
qualifiers from pointer target type
libass/ass_mp.c:347: warning: passing argument 4 of 'ass_read_memory' discards
qualifiers from pointer target type

So no, won't change. Whoever fix open_stream and ass_read_memory prototype (and
external libass as well) should fix this prototype too then.

>> +	fd=open_stream (fname, NULL, &i);
> 
> extra space before (
Fixed.
> 
>> +	if (fd->end_pos > STREAM_BUFFER_SIZE)
>> +		/* read entrie file at one if size is known */
> 
> ? At least two typos I think
Uhm, even noticed, but forgot to fix :-\
Fixed now.
> 
>> +		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?
Yes. Upstream libass actually recently raised limit to 50M; internal libass has
same limit as my patch.
> 
>> +		if (buf) free(buf);
> 
> Useless if
Removed.
> 
>> +	buf[sz] = '\0';
> 
> Use just 0, '\0' is just pointless clutter
Changed.
> 
>> +		if(track->name)
>> +			free(track->name);
> 
> Also pointless if
Removed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libass-read-stream.patch
Type: text/x-diff
Size: 3159 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20100204/2f08681b/attachment.patch>