[Experiment] Clip/cull (partially-)offscreen glyphs #874
Conversation
This function sucks and should be rewritten by someone who understands the blur math better.
|
I'll do in-depth review later, only couple of thoughts now.
|
|
Pushed grid-tile-snapping; currently I'm using a 64-pixel grid, and it does seem to help a bit; I didn't see a substantial difference between a 64 and a 256-pixel grid on some quick-and-dirty testing, but I'd imagine it'll vary depending on content in practice and we should test a wider variety of samples to determine what works best. If you can write a routine that calculates a blur padding estimate guaranteed to always be ≥ what's required, I'd very much appreciate it. I think it'd probably be best to compute that alongside the blur value itself, and then store it in |
There was a problem hiding this comment.
Pushed grid-tile-snapping; currently I'm using a 64-pixel grid, and it does seem to help a bit;
I think it would be better to do a two-stage snapping: first on composite level and second on individual glyph level. Snapping only on glyph level can pollute composite cache (for example, for long scrolling text) and it can be costly in case of heavy blur. I've done something similar for subpixel shift quantization.
If you can write a routine that calculates a blur padding estimate guaranteed to always be ≥ what's required
Padding generated by the blur algorithm is not a strict requirement, it's mostly due to the padding requirements of individual stages. So we can choose arbitrary padding that doesn't introduce visible artifacts. I think (int) (4 * sqrt(r2) + 1) is good enough (internal algorithm padding is between 4 * sqrt(r2) and 6 * sqrt(r2)).
| #define RECT(member) \ | ||
| a->member.x_min == b->member.x_min && a->member.y_min == b->member.y_min && \ | ||
| a->member.x_max == b->member.x_max && a->member.y_max == b->member.y_max && | ||
|
|
There was a problem hiding this comment.
Other entries do not have empty lines.
libass/ass_bitmap.c
Outdated
| @@ -167,6 +168,19 @@ bool ass_outline_to_bitmap(RenderContext *state, Bitmap *bm, | |||
| int32_t y_min = (rst->bbox.y_min - 1) >> 6; | |||
| int32_t x_max = (rst->bbox.x_max + 127) >> 6; | |||
| int32_t y_max = (rst->bbox.y_max + 127) >> 6; | |||
|
|
|||
|
|
|||
| if (clip->x_min != 0 || clip->y_min != 0 || clip->x_max != 0 || clip->y_max != 0) { | |||
There was a problem hiding this comment.
I think it's better to use default no-clip rectangle {INT32_MIN, INT32_MIN, INT32_MAX, INT32_MAX} instead of {0, 0, 0, 0} so that condition could be eliminated.
| render_priv->settings.frame_height, | ||
| }; | ||
|
|
||
| #ifdef USE_RECT_CLIP |
There was a problem hiding this comment.
With grid snapping it should be safe to remove USE_RECT_CLIP.
There was a problem hiding this comment.
I agree, retesting the perf now would be good
| } | ||
| #endif | ||
|
|
||
| bool potentially_clipped = ( |
There was a problem hiding this comment.
I think it's better to do the other way around: calculate clip rectangle unconditionally, but crop by outline cbox before cache request.
There was a problem hiding this comment.
Hmm, can we actually skip the cache request altogether if the clip rectangle doesn't intersect with the cbox?
| ((qr[0] + key->outline->cbox.x_max) > (int64_t)((uint64_t)clip->x_max << SUBPIXEL_ORDER)) || | ||
| ((qr[1] + key->outline->cbox.y_max) > (int64_t)((uint64_t)clip->y_max << SUBPIXEL_ORDER)) | ||
| )) | ||
| clip = NULL; |
There was a problem hiding this comment.
I think it's better to set clip to shifted outline cbox instead of using it as flag.
Actually, using outline cbox is incorrect, it should be transformed outline cbox.
As first approximation, we could transform 4 corners of source outline cbox and use it.
There was a problem hiding this comment.
There might be some heuristics we could use to improve behavior on 2D-scroll cases? Maybe something like "never actually render a partial glyph if position is animated but rotation isn't; instead, make a decision on whether to reject rasterizing at all [if fully offscreen], or to re-request the unclipped glyph from cache"?
I like the idea. How about always rendering each individual glyph fully (or not at all), unless it exceeds a certain size threshold (too costly) or rotation is animated (unlikely the cached version actually will be used again)?
In pathological cases even an unanimated rotation might lead to very large bitmaps for a single glyph (rotation around x or y which puts the glyph close to the camera plane just outside the render frame). While perhaps less likely (scripted) scale effects may achieve the same without rotation.
| @@ -1372,6 +1379,8 @@ static void calc_transform_matrix(RenderContext *state, | |||
| } | |||
| } | |||
|
|
|||
| int ass_be_padding(int be); | |||
| render_priv->settings.frame_height, | ||
| }; | ||
|
|
||
| #ifdef USE_RECT_CLIP |
There was a problem hiding this comment.
I agree, retesting the perf now would be good
| clip_rect.x_min -= (shadow_x + 63) >> 6; | ||
| else | ||
| clip_rect.x_max -= (shadow_x - 63) >> 6; | ||
| if (shadow_y > 0) |
There was a problem hiding this comment.
nit: different if/else constructs should be separated by an empty line
This vastly improves performance on some samples by culling (parts of) glyphs that are far enough offscreen that they can't possibly affect our final output. It's a very large strict improvement on things like "star wars title crawls", but results in increased CPU usage (though still generally better worst-frame performance and fewer framedrops) on some simple 2D-scrolling signs (since it results in more cache misses when the same glyph with the same parameters is drawn with multiple clips).
TODO:
ass_blur_padding()is trash and should be rewritten (@MrSmile, any interest?)USE_RECT_CLIPis currently disabled because I expect it would worsen clip-sliced gradientsass_composite_construct)