> 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 ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel