Re: [v2 PATCH 3/3] windowscodecs: Add support for palette image formats to PNG encoder.

[Vincent Povirk]

> +                else
> +                {
> +                    png_palette[i].red = 0;
> +                    png_palette[i].green = 0;
> +                    png_palette[i].blue = 0;
> +                }

Why is it necessary to pad the palette in this way? Couldn't you just
pass the appropriate number of colors?

[v2 PATCH 3/3] windowscodecs: Add support for palette image formats to PNG encoder.

[Sebastian Lackner]

On 27.01.2017 22:28, Vincent Povirk wrote:
> > +                else
> > +                {
> > +                    png_palette[i].red = 0;
> > +                    png_palette[i].green = 0;
> > +                    png_palette[i].blue = 0;
> > +                }
>
> Why is it necessary to pad the palette in this way? Couldn't you just
> pass the appropriate number of colors?

Clamping to 1 << This->format->bit_depth is necessary (see the comment
about newer libpng versions), but padding the palette probably isn't,
at least I'm not aware of any test for it. It should be fine to
replace

colors = 1 << This->format->bit_depth;

with

colors = min(This->colors, 1 << This->format->bit_depth);

and to remove the else branch.

[v2 PATCH 3/3] windowscodecs: Add support for palette image formats to PNG encoder.

[Dmitry Timoshkov]

Sebastian Lackner <sebastian(a)fds-team.de> wrote:
> On 27.01.2017 22:28, Vincent Povirk wrote:
> > > +                else
> > > +                {
> > > +                    png_palette[i].red = 0;
> > > +                    png_palette[i].green = 0;
> > > +                    png_palette[i].blue = 0;
> > > +                }
> >
> > Why is it necessary to pad the palette in this way? Couldn't you just
> > pass the appropriate number of colors?
>
> Clamping to 1 << This->format->bit_depth is necessary (see the comment
> about newer libpng versions), but padding the palette probably isn't,
> at least I'm not aware of any test for it.

It still would be a good idea to test this, a standalone application should
be fine. Although I'd guess this basically depends on libpng (old vs. new)
behaviour, as far as I can see from my own tests WIC in Windows doesn't impose
limitations to a PNG palette size.
> It should be fine to replace
>
> colors = 1 << This->format->bit_depth;
>
> with
>
> colors = min(This->colors, 1 << This->format->bit_depth);
>
> and to remove the else branch.

That should work, if newer libpng versions accept such palette sizes.

-- 
Dmitry.