On Sat, Oct 7, 2017 at 7:48 AM, Clément Bœsch <u...@pkh.me> wrote:
> > 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:
> > ---
>
<snip>
> > + { "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.
>
I'm not sure how that works, but apparently so.
> <snip>
> > + 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 don't see another clear solution.
<snip>
> > 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?
>
There is some ambiguity about dealing with transparency from the incoming
image, and I will have to change that condition anyway (more later).
<snip>
>
> > 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.
>
Changing the variable name was not accidental. "c" and "color" are
ambiguous when converting from rgb to indexed, and this patch no longer
works with rgb, but argb, so I believe being explicit is better. I was very
confused trying to follow which c/color variables where what when diving
into this code.
>
> > + 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?
>
this variable is the argb color, with elements (elts) separated into an
array. It is necessary for the diff function. Is there another variable
name you would prefer?
>
> > + }
> > 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?
>
Sorry, that's an artifact of a prior version.
>
> [...]
>
> 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 can't speak to this except for the testing I've done, and FATE.
> - 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 don't see anything to this effect, but that's part of the reason I kept
transparent colors out of the tree.
- I'll have to check all the debug options to make sure something didn't
> go wrong.
>
--
Bjorn Roche
Sr. Video Pipeline Engineer
bj...@giphy.com
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
