[FFmpeg-devel] [PATCH] Fix for paletteuse to support transparency

[Bjorn Roche]

On Sat, Oct 7, 2017 at 7:48 AM, Clément Bœsch <u at 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

bjorn at giphy.com