Assertion fails in RGBGFX

[Rangi42]

RGBGFX somehow thinks a 5-color tile has 13 colors:

$ ./rgbgfx -n 64 foo.png
warning: Fusing colors #00447eff and #00407dff into Game Boy color $3d00 [first seen at x: 17, y: 120]
FATAL: Tile at (152, 224) has 13 opaque colors, more than 4!
Conversion aborted after 1 error

If the tile is edited to have 4 colors, an assertion is tripped:

$ ./rgbgfx -n 64 -o foo2.2bpp foo2.png
warning: Fusing colors #00447eff and #00407dff into Game Boy color $3d00 [first seen at x: 17, y: 120]
rgbgfx: src/gfx/process.cpp:733: static uint16_t TileData::rowBitplanes(const Png::TilesVisitor::Tile&, const Palette&, uint32_t): Assertion `index < palette.size()' failed.
Aborted (core dumped)

Notes:

• Editing #00447e and #00407d to be the same color to avoid fusing has no effect on these issues.

[Rangi42]

Generate a tile that has 4 colours, and then fill the rest with a fifth colour. The bug occurs in process.cpp:1128.

[Rangi42]

A smaller file that reproduces this error:

$ ./rgbgfx -o foo.2bpp foo.png
FATAL: Tile at (8, 0) has 13 opaque colors, more than 4!
Conversion aborted after 1 error

However, when the tile is fixed to be 4 colors, it does not fail the assertion, even with make develop.

Same goes for this larger attempt:

[Rangi42]

This 4-tile image fails the assertion:

$ ./rgbgfx -n 64 -o foo.2bpp foo.png
rgbgfx: src/gfx/process.cpp:733: static uint16_t TileData::rowBitplanes(const Png::TilesVisitor::Tile&, const Palette&, uint32_t): Assertion `index < palette.size()' failed.
Aborted (core dumped)

Note that it has a 5-color tile which is not caught!

[Rangi42]

...oh. All it takes is this 3-tile image, not even with -n 64:

$ file foo.png
foo.png: PNG image data, 24 x 8, 8-bit/color RGBA, non-interlaced
$ ./rgbgfx -o foo.2bpp foo.png
rgbgfx: src/gfx/process.cpp:733: static uint16_t TileData::rowBitplanes(const Png::TilesVisitor::Tile&, const Palette&, uint32_t): Assertion `index < palette.size()' failed.
Aborted (core dumped)

[Rangi42]

This did not happen with rgbgfx 0.8.0; I expect it's due to the modifications to the palette-packing algorithm since then.

Edit: wait, I just retested and it does fail on the above three-tile foo.png with 0.8.0? Did I make a mistake here earlier? And 0.7.0, and 0.6.0! But 0.5.2 works (error: Too many colors in input PNG file to fit into a 2-bit palette (max 4).).

[Rangi42]

Okay, git bisect has found that something like this bug existed ever since the C++ rewrite in 8c62e80. Prior to that, we got the Too many colors in input PNG error. After that, at first it would silently just work (combining the dark red and light blue into one hue), or as of fcce42d it would fail the assertion. (Note that both of those commits are part of #981.)

[Rangi42]

So, #1546 may have "fixed" the issue after all. Not directly, but because palette assignment was broken to begin with (false negatives, i.e. allowing through 5-color tiles and somehow converting them anyway).

We're not totally clear on why the logic was broken in the first place, but the assertion here has since been catching these cases, and the protopalette fix has also prevented false positives.

The initial image in this issue no longer produces the bug. So we feel this can be closed.

[ISSOtm]

I'll add that some other reproducing images no longer cause the error, and they all had in common that they contain 5-colour tiles; so, since they no longer reproduce the assertion failure and the new colour-counting logic is straightforward and reasonably smoke-tested, I also gave my vote of confidence to closing this issue.