[MPlayer-dev-eng] [**13 ESSENTIAL PATCHES**] Libass enhancements that need to coincide with the new EOSD patches

[Reimar Döffinger]

On Mon, Feb 23, 2009 at 12:16:16PM -0500, Eric Appleman wrote:
> As my patience is wearing thing, I must repeat once again. Why are we 
> implementing Greg's EOSD patches if we aren't even looking at his 13 newest 
> libass patches that would enhance EOSD rendering that have been repeatedly 
> submitted and ignored? Why are they not getting any attention?

Because for now I maintain VDPAU (and I know EOSD well enough) but I have hardly
any clue of libass, or ASS in general.

> If you guys are content with these patches flying under the radar yet 
> again, PLEASE SAY SO RIGHT HERE AND NOW.
>
> I apologize for coming off as an ass with this message, but it seems that I 
> must turn up the volume in order to be heard.

No, you need to get the attention of the right people.
And to my knowledge that is basically Evgeniy Stepanovi, who is listed
as the sole maintainer of libass.
If for some reason he can not maintain it (though 1 week delay is not
enough to count as that) we can look for someone who will either in
addition or instead maintain it, but unless we see no other choice
these things remain Evgeniy's decisions.

> * 0009-Add-simple-blur-with-1-2-1-for-be.patch
> Use a kernel blur with [[1,2,1], [1,2,1], [1,2,1]] matrix for \be. This is 
> the same filter used by vsfilter and also a bit faster than the gaussian 
> blur.

Uh, the matrix used in the code is
[[1,2,1], [2,4,2], [1,2,1]]

> diff --git a/libass/ass_utils.c b/libass/ass_utils.c
> index d48685c..91b55eb 100644
> --- a/libass/ass_utils.c
> +++ b/libass/ass_utils.c
> @@ -32,8 +32,21 @@
>  
>  int mystrtoi(char** p, int base, int* res)
>  {
> +	// NOTE: base argument is ignored, but not used in libass anyway
> +	double temp_res;
>  	char* start = *p;
> -	*res = strtol(*p, p, base);
> +	temp_res = strtod(*p, p);
> +	*res = (int) (temp_res + 0.5);
> +	if (*p != start) return 1;
> +	else return 0;
> +}
> +
> +int mystrtoll(char** p, int base, long long* res)
> +{
> +	double temp_res;
> +	char* start = *p;
> +	temp_res = strtod(*p, p);
> +	*res = (long long) (temp_res + 0.5);
>  	if (*p != start) return 1;
>  	else return 0;
>  }

Is that return value ever used? Otherwise you could just use one
function.

> >From edce1412b4bdb6561432b23260f6c2694007d410 Mon Sep 17 00:00:00 2001
> From: greg <greg at blackbox.(none)>
> Date: Wed, 18 Feb 2009 03:14:02 +0100
> Subject: [PATCH 09/12] Add simple blur with [1,2,1] for \be.
>  This is the same blur vsfilter uses. It's also faster than gaussian
>  blur.

Is it _exactly_ the same code as vsfilter? Because that code does not
blur the borders at all.

> +	for (y=0; y<h; y++)
> +		for (x=1; x<w-1; x++) {
> +			p = buf[y*w+x-1];
> +			p += 2 * buf[y*w+x];
> +			p += buf[y*w+x+1];
> +			p = p >> 2;
> +			tmp_buf[y*w+x] = p;

This can be done with less calculation:
for (y=0; y<h; y++)
    old_sum = 2 * buf[0];
    for (x=0; x<w-1; x++) {
        new_sum = buf[y*w+x] + buf[y*w+x+1];
        buf[y*w+x] = (old_sum + new_sum) >> 2;
        old_sum = new_sum;
    }
which is also in-place, thus halving the memory bandwidth requirements
and blurs the left border. Right border is still not handled though.

> +	for (x=0; x<w; x++)
> +		for (y=1; y<h-1; y++) {
> +			p = tmp_buf[((y-1)*w+x)];
> +			p += 2 * tmp_buf[y*w+x];
> +			p += tmp_buf[((y+1)*w+x)];
> +			p = p >> 2;
> +			buf[y*w+x] = p;
> +		}

The same principle could be applied here, though traversing the image
column-wise is putting a very high strain on the cache (though that is
the same as the current code), usually you would process at least one
cache-line of data horizontally at once, though performance may not
matter enough to justify the complexity here.

> @@ -278,19 +328,16 @@ int glyph_to_bitmap(ass_synth_priv_t* priv, ass_synth_priv_t* priv_blur,
>  			return 1;
>  		}
>  	}
> -	if (*bm_o) {
> -		resize_tmp(priv, (*bm_o)->w, (*bm_o)->h);
> +	if (*bm_o)
>  		resize_tmp(priv_blur, (*bm_o)->w, (*bm_o)->h);
> -	}
> -	resize_tmp(priv, (*bm_g)->w, (*bm_g)->h);
>  	resize_tmp(priv_blur, (*bm_g)->w, (*bm_g)->h);

Not removing the {} would make the diff a lot more readable.