Re: [FFmpeg-devel] [PATCH v2 02/32] avfilter/palette{gen, use}: revert support palettes with alpha

2023-01-03 Thread Paul B Mahol
Please add alpha support back.

On Wed, Dec 28, 2022 at 12:18 AM Clément Bœsch  wrote:

> This reverts commit dea673d0d548c864ec85f9260d8900d944ef7a2a.
>
> This change cannot work for several reasons, the most obvious ones are:
>
> - the alpha is being part of the scoring of the color difference, even
>   though we can not interpret the alpha as part of the perception of the
>   color (we don't even know if it's premultiplied or postmultiplied)
> - the colors are averaged with their alpha value which simply cannot
>   work
>
> The command proposed in the original thread of the patch actually
> produces a completely broken file:
>
> ffmpeg -y -loglevel verbose -i fate-suite/apng/o_sample.png
> -filter_complex
> "split[split1][split2];[split1]palettegen=max_colors=254:use_alpha=1[pal1];[split2][pal1]paletteuse=use_alpha=1"
> -frames:v 1 out.png
>
> We can see that many color pixels are off, but more importantly some
> colors have a random alpha value: https://imgur.com/eFQ2UK7
>
> I don't see any easy fix for this unfortunately, the approach appears to
> be flawed by design.
> ---
>  doc/filters.texi|   8 --
>  libavfilter/vf_palettegen.c | 127 +++-
>  libavfilter/vf_paletteuse.c | 225 +++-
>  3 files changed, 138 insertions(+), 222 deletions(-)
>
> diff --git a/doc/filters.texi b/doc/filters.texi
> index 9b866de5ae..f51623d16a 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -18474,9 +18474,6 @@ Compute new histogram for each frame.
>  @end table
>
>  Default value is @var{full}.
> -@item use_alpha
> -Create a palette of colors with alpha components.
> -Setting this, will automatically disable 'reserve_transparent'.
>  @end table
>
>  The filter also exports the frame metadata @code{lavfi.color_quant_ratio}
> @@ -18555,11 +18552,6 @@ will be treated as completely opaque, and values
> below this threshold will be
>  treated as completely transparent.
>
>  The option must be an integer value in the range [0,255]. Default is
> @var{128}.
> -
> -@item use_alpha
> -Apply the palette by taking alpha values into account. Only useful with
> -palettes that are containing multiple colors with alpha components.
> -Setting this will automatically disable 'alpha_treshold'.
>  @end table
>
>  @subsection Examples
> diff --git a/libavfilter/vf_palettegen.c b/libavfilter/vf_palettegen.c
> index c03f62b942..bea3292796 100644
> --- a/libavfilter/vf_palettegen.c
> +++ b/libavfilter/vf_palettegen.c
> @@ -59,7 +59,7 @@ enum {
>  };
>
>  #define NBITS 5
> -#define HIST_SIZE (1<<(4*NBITS))
> +#define HIST_SIZE (1<<(3*NBITS))
>
>  typedef struct PaletteGenContext {
>  const AVClass *class;
> @@ -67,7 +67,6 @@ typedef struct PaletteGenContext {
>  int max_colors;
>  int reserve_transparent;
>  int stats_mode;
> -int use_alpha;
>
>  AVFrame *prev_frame;// previous frame used for
> the diff stats_mode
>  struct hist_node histogram[HIST_SIZE];  // histogram/hashtable of the
> colors
> @@ -89,7 +88,6 @@ static const AVOption palettegen_options[] = {
>  { "full", "compute full frame histograms", 0, AV_OPT_TYPE_CONST,
> {.i64=STATS_MODE_ALL_FRAMES}, INT_MIN, INT_MAX, FLAGS, "mode" },
>  { "diff", "compute histograms only for the part that differs from
> previous frame", 0, AV_OPT_TYPE_CONST, {.i64=STATS_MODE_DIFF_FRAMES},
> INT_MIN, INT_MAX, FLAGS, "mode" },
>  { "single", "compute new histogram for each frame", 0,
> AV_OPT_TYPE_CONST, {.i64=STATS_MODE_SINGLE_FRAMES}, INT_MIN, INT_MAX,
> FLAGS, "mode" },
> -{ "use_alpha", "create a palette including alpha values",
> OFFSET(use_alpha), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, FLAGS },
>  { NULL }
>  };
>
> @@ -115,16 +113,15 @@ static int cmp_##name(const void *pa, const void
> *pb)   \
>  {   \
>  const struct color_ref * const *a = pa; \
>  const struct color_ref * const *b = pb; \
> -return   (int)((*a)->color >> (8 * (3 - (pos))) & 0xff)  \
> -   - (int)((*b)->color >> (8 * (3 - (pos))) & 0xff); \
> +return   (int)((*a)->color >> (8 * (2 - (pos))) & 0xff)  \
> +   - (int)((*b)->color >> (8 * (2 - (pos))) & 0xff); \
>  }
>
> -DECLARE_CMP_FUNC(a, 0)
> -DECLARE_CMP_FUNC(r, 1)
> -DECLARE_CMP_FUNC(g, 2)
> -DECLARE_CMP_FUNC(b, 3)
> +DECLARE_CMP_FUNC(r, 0)
> +DECLARE_CMP_FUNC(g, 1)
> +DECLARE_CMP_FUNC(b, 2)
>
> -static const cmp_func cmp_funcs[] = {cmp_a, cmp_r, cmp_g, cmp_b};
> +static const cmp_func cmp_funcs[] = {cmp_r, cmp_g, cmp_b};
>
>  /**
>   * Simple color comparison for sorting the final palette
> @@ -146,17 +143,6 @@ static av_always_inline int diff(const uint32_t a,
> const uint32_t b)
>  return dr*dr + dg*dg + db*db;
>  }
>
> -static av_always_inline int diff_alpha(const uint32_t a, const uint32_t b)
> -{
> -const uint8_t c1[] = {a >> 24 & 0xff, a >> 16 & 0xff, a >> 8 & 0xff,
> a & 0xff};
> -const uint8_t c2[] 

[FFmpeg-devel] [PATCH v2 02/32] avfilter/palette{gen, use}: revert support palettes with alpha

2022-12-27 Thread Clément Bœsch
This reverts commit dea673d0d548c864ec85f9260d8900d944ef7a2a.

This change cannot work for several reasons, the most obvious ones are:

- the alpha is being part of the scoring of the color difference, even
  though we can not interpret the alpha as part of the perception of the
  color (we don't even know if it's premultiplied or postmultiplied)
- the colors are averaged with their alpha value which simply cannot
  work

The command proposed in the original thread of the patch actually
produces a completely broken file:

ffmpeg -y -loglevel verbose -i fate-suite/apng/o_sample.png -filter_complex 
"split[split1][split2];[split1]palettegen=max_colors=254:use_alpha=1[pal1];[split2][pal1]paletteuse=use_alpha=1"
 -frames:v 1 out.png

We can see that many color pixels are off, but more importantly some
colors have a random alpha value: https://imgur.com/eFQ2UK7

I don't see any easy fix for this unfortunately, the approach appears to
be flawed by design.
---
 doc/filters.texi|   8 --
 libavfilter/vf_palettegen.c | 127 +++-
 libavfilter/vf_paletteuse.c | 225 +++-
 3 files changed, 138 insertions(+), 222 deletions(-)

diff --git a/doc/filters.texi b/doc/filters.texi
index 9b866de5ae..f51623d16a 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -18474,9 +18474,6 @@ Compute new histogram for each frame.
 @end table
 
 Default value is @var{full}.
-@item use_alpha
-Create a palette of colors with alpha components.
-Setting this, will automatically disable 'reserve_transparent'.
 @end table
 
 The filter also exports the frame metadata @code{lavfi.color_quant_ratio}
@@ -18555,11 +18552,6 @@ will be treated as completely opaque, and values below 
this threshold will be
 treated as completely transparent.
 
 The option must be an integer value in the range [0,255]. Default is @var{128}.
-
-@item use_alpha
-Apply the palette by taking alpha values into account. Only useful with
-palettes that are containing multiple colors with alpha components.
-Setting this will automatically disable 'alpha_treshold'.
 @end table
 
 @subsection Examples
diff --git a/libavfilter/vf_palettegen.c b/libavfilter/vf_palettegen.c
index c03f62b942..bea3292796 100644
--- a/libavfilter/vf_palettegen.c
+++ b/libavfilter/vf_palettegen.c
@@ -59,7 +59,7 @@ enum {
 };
 
 #define NBITS 5
-#define HIST_SIZE (1<<(4*NBITS))
+#define HIST_SIZE (1<<(3*NBITS))
 
 typedef struct PaletteGenContext {
 const AVClass *class;
@@ -67,7 +67,6 @@ typedef struct PaletteGenContext {
 int max_colors;
 int reserve_transparent;
 int stats_mode;
-int use_alpha;
 
 AVFrame *prev_frame;// previous frame used for the 
diff stats_mode
 struct hist_node histogram[HIST_SIZE];  // histogram/hashtable of the 
colors
@@ -89,7 +88,6 @@ static const AVOption palettegen_options[] = {
 { "full", "compute full frame histograms", 0, AV_OPT_TYPE_CONST, 
{.i64=STATS_MODE_ALL_FRAMES}, INT_MIN, INT_MAX, FLAGS, "mode" },
 { "diff", "compute histograms only for the part that differs from 
previous frame", 0, AV_OPT_TYPE_CONST, {.i64=STATS_MODE_DIFF_FRAMES}, INT_MIN, 
INT_MAX, FLAGS, "mode" },
 { "single", "compute new histogram for each frame", 0, 
AV_OPT_TYPE_CONST, {.i64=STATS_MODE_SINGLE_FRAMES}, INT_MIN, INT_MAX, FLAGS, 
"mode" },
-{ "use_alpha", "create a palette including alpha values", 
OFFSET(use_alpha), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, FLAGS },
 { NULL }
 };
 
@@ -115,16 +113,15 @@ static int cmp_##name(const void *pa, const void *pb)   \
 {   \
 const struct color_ref * const *a = pa; \
 const struct color_ref * const *b = pb; \
-return   (int)((*a)->color >> (8 * (3 - (pos))) & 0xff)  \
-   - (int)((*b)->color >> (8 * (3 - (pos))) & 0xff); \
+return   (int)((*a)->color >> (8 * (2 - (pos))) & 0xff)  \
+   - (int)((*b)->color >> (8 * (2 - (pos))) & 0xff); \
 }
 
-DECLARE_CMP_FUNC(a, 0)
-DECLARE_CMP_FUNC(r, 1)
-DECLARE_CMP_FUNC(g, 2)
-DECLARE_CMP_FUNC(b, 3)
+DECLARE_CMP_FUNC(r, 0)
+DECLARE_CMP_FUNC(g, 1)
+DECLARE_CMP_FUNC(b, 2)
 
-static const cmp_func cmp_funcs[] = {cmp_a, cmp_r, cmp_g, cmp_b};
+static const cmp_func cmp_funcs[] = {cmp_r, cmp_g, cmp_b};
 
 /**
  * Simple color comparison for sorting the final palette
@@ -146,17 +143,6 @@ static av_always_inline int diff(const uint32_t a, const 
uint32_t b)
 return dr*dr + dg*dg + db*db;
 }
 
-static av_always_inline int diff_alpha(const uint32_t a, const uint32_t b)
-{
-const uint8_t c1[] = {a >> 24 & 0xff, a >> 16 & 0xff, a >> 8 & 0xff, a & 
0xff};
-const uint8_t c2[] = {b >> 24 & 0xff, b >> 16 & 0xff, b >> 8 & 0xff, b & 
0xff};
-const int da = c1[0] - c2[0];
-const int dr = c1[1] - c2[1];
-const int dg = c1[2] - c2[2];
-const int db = c1[3] - c2[3];
-return da*da + dr*dr + dg*dg + db*db;
-}
-
 /**
  * Find the next box to split: pick the one