> Re: [FFmpeg-devel] [PATCH] Fix for paletteuse to support transparency The commit message needs adjustment "lavfi/paletteuse: ..."
[...]
> 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,8 @@ 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. */
"if there is no transparent color" might be more correct but I'm not a
native speaker.
> + int trans_thresh;
> int palette_loaded;
> int dither;
> int new;
> @@ -116,6 +118,7 @@ static const AVOption paletteuse_options[] = {
> { "bayer_scale", "set scale for bayer dithering", OFFSET(bayer_scale),
> AV_OPT_TYPE_INT, {.i64=2}, 0, 5, FLAGS },
> { "diff_mode", "set frame difference mode", OFFSET(diff_mode),
> AV_OPT_TYPE_INT, {.i64=DIFF_MODE_NONE}, 0, NB_DIFF_MODE-1, FLAGS, "diff_mode"
> },
> { "rectangle", "process smallest different rectangle", 0,
> AV_OPT_TYPE_CONST, {.i64=DIFF_MODE_RECTANGLE}, INT_MIN, INT_MAX, FLAGS,
> "diff_mode" },
> + { "threshold", "set the alpha threshold for transparency. Above this
> threshold, pixels are considered opaque, below they are considered
> transparent.", OFFSET(trans_thresh), AV_OPT_TYPE_INT, {.i64=128}, 0, 255, },
The long explanation belongs in doc/filters.texi
[...]
> -static av_always_inline int diff(const uint8_t *c1, const uint8_t *c2)
> +static av_always_inline int diff(const uint8_t *c1, const uint8_t *c2, const
> int trans_thresh)
> {
> // 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 = 195075; //equal to 255*255 + 255*255 +
> 255*255
That's not what I meant; I wasn't concerned about the expression but the
static const int. I was thinking "return 255*255 + 255*255 + 255*255"
[...]
> /**
> * Check if the requested color is in the cache already. If not, find it in
> the
> * color tree and cache it.
> - * Note: r, g, and b are the component of c but are passed as well to avoid
> + * Note: a, r, g, and b are the components of argb, but are passed as well
> to avoid
> * recomputing them (they are generally computed by the caller for other
> uses).
> */
> -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'm sorry to insist, but renaming "color" belongs in another commit.
[...]
> 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, "c" should be renamed in another commit
[...]
> i = 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[i] = p[x];
> + if (p[x]>>24 < s->trans_thresh) {
> + s->transparency_index = i; // we are assuming at most one
> transparent color in palette
> + }
> + ++i;
i++ is the appropriate idiom.
Aside from these nitpicks, I'm still concerned about how it's going to
conflict with GIF encoding where the transparent color is actually used as
a mean of not updating pixels from previous frames.
--
Clément B.
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list [email protected] http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
