[FFmpeg-devel] [PATCH] Fix for paletteuse to support transparency
Clément Bœsch
u at pkh.me
Sat Oct 7 14:48:16 EEST 2017
> Subject: Re: [FFmpeg-devel] [PATCH] Fix for paletteuse to support transparency
"lavf/paletteuse: add support for transparency"
On Fri, Oct 06, 2017 at 04:59:55PM -0400, Bjorn Roche wrote:
> ---
> libavfilter/vf_paletteuse.c | 175 ++++++++++++++++++++++++++++----------------
> 1 file changed, 112 insertions(+), 63 deletions(-)
>
> diff --git a/libavfilter/vf_paletteuse.c b/libavfilter/vf_paletteuse.c
> index ffd37bf1da..4203543843 100644
> --- a/libavfilter/vf_paletteuse.c
> +++ b/libavfilter/vf_paletteuse.c
> @@ -56,7 +56,7 @@ enum diff_mode {
> };
>
> struct color_node {
> - uint8_t val[3];
> + uint8_t val[4];
> uint8_t palette_id;
> int split;
> int left_id, right_id;
> @@ -86,6 +86,7 @@ typedef struct PaletteUseContext {
> struct cache_node cache[CACHE_SIZE]; /* lookup cache */
> struct color_node map[AVPALETTE_COUNT]; /* 3D-Tree (KD-Tree with K=3) for reverse colormap */
> uint32_t palette[AVPALETTE_COUNT];
> + int transparency_index; /* index in the palette of transparency. -1 if there isn't a transparency. */
> int palette_loaded;
> int dither;
> int new;
> @@ -108,6 +109,7 @@ typedef struct PaletteUseContext {
> #define FLAGS AV_OPT_FLAG_FILTERING_PARAM|AV_OPT_FLAG_VIDEO_PARAM
> static const AVOption paletteuse_options[] = {
> { "dither", "select dithering mode", OFFSET(dither), AV_OPT_TYPE_INT, {.i64=DITHERING_SIERRA2_4A}, 0, NB_DITHERING-1, FLAGS, "dithering_mode" },
> + { "none", "no dither", 0, AV_OPT_TYPE_CONST, {.i64=DITHERING_NONE}, INT_MIN, INT_MAX, FLAGS, "dithering_mode" },
I think none is already supported as builtin in AVOption, isn't it? In any
case, this should probably be in a separated patch if deemed useful.
> { "bayer", "ordered 8x8 bayer dithering (deterministic)", 0, AV_OPT_TYPE_CONST, {.i64=DITHERING_BAYER}, INT_MIN, INT_MAX, FLAGS, "dithering_mode" },
> { "heckbert", "dithering as defined by Paul Heckbert in 1982 (simple error diffusion)", 0, AV_OPT_TYPE_CONST, {.i64=DITHERING_HECKBERT}, INT_MIN, INT_MAX, FLAGS, "dithering_mode" },
> { "floyd_steinberg", "Floyd and Steingberg dithering (error diffusion)", 0, AV_OPT_TYPE_CONST, {.i64=DITHERING_FLOYD_STEINBERG}, INT_MIN, INT_MAX, FLAGS, "dithering_mode" },
> @@ -157,7 +159,8 @@ static int query_formats(AVFilterContext *ctx)
>
> static av_always_inline int dither_color(uint32_t px, int er, int eg, int eb, int scale, int shift)
> {
> - return av_clip_uint8((px >> 16 & 0xff) + ((er * scale) / (1<<shift))) << 16
> + return av_clip_uint8((px >> 24 & 0xff) ) << 24
Here and several times later, I believe you can drop the `& 0xff`.
> + | av_clip_uint8((px >> 16 & 0xff) + ((er * scale) / (1<<shift))) << 16
> | av_clip_uint8((px >> 8 & 0xff) + ((eg * scale) / (1<<shift))) << 8
> | av_clip_uint8((px & 0xff) + ((eb * scale) / (1<<shift)));
> }
> @@ -165,10 +168,18 @@ static av_always_inline int dither_color(uint32_t px, int er, int eg, int eb, in
> static av_always_inline int diff(const uint8_t *c1, const uint8_t *c2)
> {
> // XXX: try L*a*b with CIE76 (dL*dL + da*da + db*db)
> - const int dr = c1[0] - c2[0];
> - const int dg = c1[1] - c2[1];
> - const int db = c1[2] - c2[2];
> - return dr*dr + dg*dg + db*db;
> + const static int max_diff = 255*255 + 255*255 + 255*255;
> + const int dr = c1[1] - c2[1];
> + const int dg = c1[2] - c2[2];
> + const int db = c1[3] - c2[3];
> +
> + if (c1[0] == 0 && c2[0] == 0) {
> + return 0;
> + } else if (c1[0] == c2[0]) {
> + return dr*dr + dg*dg + db*db;
> + } else {
> + return max_diff;
> + }
So if both alpha are 0, you consider the color identical, and otherwise if
both alpha are different, you consider the color completely different?
I guess that's OK. Please inline 255*255 + 255*255 + 255*255 in the return
though, I don't trust compilers into optimizing that static int.
> }
>
> static av_always_inline uint8_t colormap_nearest_bruteforce(const uint32_t *palette, const uint8_t *rgb)
> @@ -179,18 +190,20 @@ static av_always_inline uint8_t colormap_nearest_bruteforce(const uint32_t *pale
> const uint32_t c = palette[i];
>
> if ((c & 0xff000000) == 0xff000000) { // ignore transparent entry
> - const uint8_t palrgb[] = {
> + const uint8_t palargb[] = {
> + palette[i]>>24 & 0xff,
according to the condition just above, this is always 0xff. Is this what
you want?
> palette[i]>>16 & 0xff,
> palette[i]>> 8 & 0xff,
> palette[i] & 0xff,
> };
> - const int d = diff(palrgb, rgb);
> + const int d = diff(palargb, rgb);
> if (d < min_dist) {
> pal_id = i;
> min_dist = d;
> }
> }
> }
> +
unrelated change
> return pal_id;
> }
>
> @@ -325,14 +338,15 @@ end:
> * Note: r, g, and b are the component of c but are passed as well to avoid
> * recomputing them (they are generally computed by the caller for other uses).
> */
This comment probably needs adjustment for `a` (and yes, it refers to `c`
instead of `color`, this needs to be adjusted too).
> -static av_always_inline int color_get(struct cache_node *cache, uint32_t color,
> - uint8_t r, uint8_t g, uint8_t b,
> +static av_always_inline int color_get(struct cache_node *cache, uint32_t argb,
I'd prefer if you'd keep "color" instead of "argb", it would reduce the
diff.
> + uint8_t a, uint8_t r, uint8_t g, uint8_t b,
> + int transparency_index,
> const struct color_node *map,
> const uint32_t *palette,
> const enum color_search_method search_method)
> {
> int i;
> - const uint8_t rgb[] = {r, g, b};
> + const uint8_t argb_elts[] = {a, r, g, b};
elts?
> const uint8_t rhash = r & ((1<<NBITS)-1);
> const uint8_t ghash = g & ((1<<NBITS)-1);
> const uint8_t bhash = b & ((1<<NBITS)-1);
> @@ -340,9 +354,14 @@ static av_always_inline int color_get(struct cache_node *cache, uint32_t color,
> struct cache_node *node = &cache[hash];
> struct cached_color *e;
>
> + // first, check for transparency
> + if (a == 0 && transparency_index >= 0) {
> + return transparency_index;
> + }
> +
> for (i = 0; i < node->nb_entries; i++) {
> e = &node->entries[i];
> - if (e->color == color)
> + if (e->color == argb)
> return e->pal_entry;
> }
>
> @@ -350,21 +369,24 @@ static av_always_inline int color_get(struct cache_node *cache, uint32_t color,
> sizeof(*node->entries), NULL);
> if (!e)
> return AVERROR(ENOMEM);
> - e->color = color;
> - e->pal_entry = COLORMAP_NEAREST(search_method, palette, map, rgb);
> + e->color = argb;
> + e->pal_entry = COLORMAP_NEAREST(search_method, palette, map, argb_elts);
> +
> return e->pal_entry;
> }
>
> static av_always_inline int get_dst_color_err(struct cache_node *cache,
> - uint32_t c, const struct color_node *map,
> + uint32_t argb, const struct color_node *map,
ditto, please keep c
> const uint32_t *palette,
> + int transparency_index,
> int *er, int *eg, int *eb,
> const enum color_search_method search_method)
[...]
> - i = 0;
> + pal_index = 0;
> for (y = 0; y < palette_frame->height; y++) {
> - for (x = 0; x < palette_frame->width; x++)
> - s->palette[i++] = p[x];
> + for (x = 0; x < palette_frame->width; x++) {
> + s->palette[pal_index] = p[x];
> + if ((p[x]>>24 & 0xff) == 0) {
> + s->transparency_index = pal_index; // we are assuming at most one transparent color
> + }
> + ++pal_index;
please just keep `i` as in the original code to keep diff small.
> + }
> p += p_linesize;
> }
>
> - load_colormap(s);
> + load_colormap(s, pal_index);
Why do you need to pass that `pal_index` as `nb_colors`; in what situation
do you end up with nb_colors ≠ AVPALETTE_COUNT?
[...]
Overall this looks pretty fine, but I didn't test yet.
I have a few concerns that may or may not be justified:
- I hope RGB32 pix fmt always set the alpha to 0xff in the rest of the
framework, otherwise this is going to break pretty hard. We may need to
add some extra check at some point if we find "too much" transparency
colors.
- I don't remember if diff should be linear somehow in the kdtree
algorithm (I think that was originally the reason I didn't use L*a*b
instead of RGB, but I might be wrong).
- I'll have to check all the debug options to make sure something didn't
go wrong.
So I'll test all of this at your next iteration.
Thanks for working on this.
--
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171007/f3bdcc07/attachment.sig>
More information about the ffmpeg-devel
mailing list