Fix RGBGFX reversal (#1425)

* Print all OoB tilemap IDs before aborting

* Rename `nbTileInstances` to `mapSize`

* Check that reversing doesn't overflow the tile array

---------

Co-authored-by: ISSOtm <me@eldred.fr>

include/gfx/main.hpp

@@ -65,6 +65,10 @@ extern Options options;
  * Prints the error count, and exits with failure
  */
 [[noreturn]] void giveUp();
+/*
+ * If any error has been emitted thus far, calls `giveUp()`.
+ */
+void requireZeroErrors();
 /*
  * Prints a warning, and does not change the error count
  */

src/gfx/main.cpp

@@ -46,6 +46,12 @@ static uintmax_t nbErrors;
 	exit(1);
 }
 
+void requireZeroErrors() {
+	if (nbErrors != 0) {
+		giveUp();
+	}
+}
+
 void warning(char const *fmt, ...) {
 	va_list ap;
 
@@ -822,10 +828,8 @@ int main(int argc, char *argv[]) {
 		fputs("Ready.\n", stderr);
 	}
 
-	// Do not do anything if option parsing went wrong
+	// Do not do anything if option parsing went wrong.
-	if (nbErrors) {
+	requireZeroErrors();
-		giveUp();
-	}
 
 	if (!options.input.empty()) {
 		if (localOptions.reverse) {
@@ -842,9 +846,7 @@ int main(int argc, char *argv[]) {
 		exit(1);
 	}
 
-	if (nbErrors) {
+	requireZeroErrors();
-		giveUp();
-	}
 	return 0;
 }
 

src/gfx/reverse.cpp

@@ -15,7 +15,6 @@
 #include "defaultinitalloc.hpp"
 #include "file.hpp"
 #include "helpers.hpp" // assume
-#include "itertools.hpp"
 
 #include "gfx/main.hpp"
 
@@ -138,22 +137,23 @@ void reverse() {
 	}
 
 	// By default, assume tiles are not deduplicated, and add the (allegedly) trimmed tiles
-	size_t nbTileInstances = tiles.size() / tileSize + options.trim; // Image size in tiles
+	size_t const nbTiles = tiles.size() / tileSize;
-	options.verbosePrint(Options::VERB_INTERM, "Read %zu tiles.\n", nbTileInstances);
+	options.verbosePrint(Options::VERB_INTERM, "Read %zu tiles.\n", nbTiles);
+	size_t mapSize = nbTiles + options.trim; // Image size in tiles
 	std::optional<DefaultInitVec<uint8_t>> tilemap;
 	if (!options.tilemap.empty()) {
 		tilemap = readInto(options.tilemap);
-		nbTileInstances = tilemap->size();
+		mapSize = tilemap->size();
-		options.verbosePrint(Options::VERB_INTERM, "Read %zu tilemap entries.\n", nbTileInstances);
+		options.verbosePrint(Options::VERB_INTERM, "Read %zu tilemap entries.\n", mapSize);
 	}
 
-	if (nbTileInstances == 0) {
+	if (mapSize == 0) {
 		fatal("Cannot generate empty image");
 	}
-	if (nbTileInstances > options.maxNbTiles[0] + options.maxNbTiles[1]) {
+	if (mapSize > options.maxNbTiles[0] + options.maxNbTiles[1]) {
 		warning(
-		    "Read %zu tiles, more than the limit of %" PRIu16 " + %" PRIu16,
+		    "Total number of tiles (%zu) is more than the limit of %" PRIu16 " + %" PRIu16,
-		    nbTileInstances,
+		    mapSize,
 		    options.maxNbTiles[0],
 		    options.maxNbTiles[1]
 		);
@@ -164,21 +164,21 @@ void reverse() {
 		// Pick the smallest width that will result in a landscape-aspect rectangular image.
 		// Thus a prime number of tiles will result in a horizontal row.
 		// This avoids redundancy with `-r 1` which results in a vertical column.
-		width = (size_t)ceil(sqrt(nbTileInstances));
+		width = (size_t)ceil(sqrt(mapSize));
-		for (; width < nbTileInstances; ++width) {
+		for (; width < mapSize; ++width) {
-			if (nbTileInstances % width == 0)
+			if (mapSize % width == 0)
 				break;
 		}
 		options.verbosePrint(Options::VERB_INTERM, "Picked reversing width of %zu tiles\n", width);
 	}
-	if (nbTileInstances % width != 0) {
+	if (mapSize % width != 0) {
 		fatal(
-		    "Total number of tiles read (%zu) cannot be divided by image width (%zu tiles)",
+		    "Total number of tiles (%zu) cannot be divided by image width (%zu tiles)",
-		    nbTileInstances,
+		    mapSize,
 		    width
 		);
 	}
-	height = nbTileInstances / width;
+	height = mapSize / width;
 
 	options.verbosePrint(
 	    Options::VERB_INTERM, "Reversed image dimensions: %zux%zu tiles\n", width, height
@@ -254,13 +254,14 @@ void reverse() {
 	}
 
 	std::optional<DefaultInitVec<uint8_t>> attrmap;
+	uint16_t nbTilesInBank[2] = {0, 0}; // Only used if there is an attrmap.
 	if (!options.attrmap.empty()) {
 		attrmap = readInto(options.attrmap);
-		if (attrmap->size() != nbTileInstances) {
+		if (attrmap->size() != mapSize) {
 			fatal(
 			    "Attribute map size (%zu tiles) doesn't match image's (%zu)",
 			    attrmap->size(),
-			    nbTileInstances
+			    mapSize
 			);
 		}
 
@@ -268,57 +269,118 @@ void reverse() {
 		// We do this now for two reasons:
 		// 1. Checking those during the main loop is harmful to optimization, and
 		// 2. It clutters the code more, and it's not in great shape to begin with
-		bool bad = false;
+		for (size_t index = 0; index < mapSize; ++index) {
-		for (auto attr : *attrmap) {
+			uint8_t attr = (*attrmap)[index];
+			size_t tx = index % width, ty = index / width;
+
 			if ((attr & 0b111) > palettes.size()) {
 				error(
-				    "Referencing palette %u, but there are only %zu!", attr & 0b111, palettes.size()
+				    "Attribute map references palette #%u at (%zu, %zu), but there are only %zu!",
+				    attr & 0b111,
+				    tx,
+				    ty,
+				    palettes.size()
 				);
-				bad = true;
 			}
-			if (attr & 0x08 && !tilemap) {
+
-				warning("Tile in bank 1 but no tilemap specified; ignoring the bank bit");
+			bool bank = attr & 0b1000;
+
+			if (!tilemap) {
+				if (bank) {
+					warning(
+					    "Attribute map assigns tile at (%zu, %zu) to bank 1, but no tilemap "
+					    "specified; "
+					    "ignoring the bank bit",
+					    tx,
+					    ty
+					);
+				}
+			} else {
+				if (uint8_t tileOfs = (*tilemap)[index] - options.baseTileIDs[bank];
+				    tileOfs >= nbTilesInBank[bank]) {
+					nbTilesInBank[bank] = tileOfs + 1;
 				}
 			}
-		if (bad) {
-			giveUp();
 		}
+
+		options.verbosePrint(
+		    Options::VERB_INTERM,
+		    "Number of tiles in bank {0: %" PRIu16 ", 1: %" PRIu16 "}\n",
+		    nbTilesInBank[0],
+		    nbTilesInBank[1]
+		);
+
+		for (int bank = 0; bank < 2; ++bank) {
+			if (nbTilesInBank[bank] > options.maxNbTiles[bank]) {
+				error(
+				    "Bank %d contains %" PRIu16 " tiles, but the specified limit is %" PRIu16,
+				    bank,
+				    nbTilesInBank[bank],
+				    options.maxNbTiles[bank]
+				);
+			}
+		}
+
+		if (nbTilesInBank[0] + nbTilesInBank[1] > nbTiles) {
+			fatal(
+			    "The tilemap references %" PRIu16 " tiles in bank 0 and %" PRIu16
+			    " in bank 1, but only %zu have been read in total",
+			    nbTilesInBank[0],
+			    nbTilesInBank[1],
+			    nbTiles
+			);
+		}
+
+		requireZeroErrors();
 	}
 
 	if (tilemap) {
 		if (attrmap) {
-			for (auto [id, attr] : zip(*tilemap, *attrmap)) {
+			for (size_t index = 0; index < mapSize; ++index) {
-				bool bank = attr & 1 << 3;
+				size_t tx = index % width, ty = index / width;
-				if (id >= options.maxNbTiles[bank]) {
+				uint8_t tileID = (*tilemap)[index];
-					warning(
+				uint8_t attr = (*attrmap)[index];
-					    "Tile #%" PRIu8 " was referenced, but the limit for bank %u is %" PRIu16,
+				bool bank = attr & 0x08;
-					    id,
+
+				if (uint8_t tileOfs = tileID - options.baseTileIDs[bank];
+				    tileOfs >= options.maxNbTiles[bank]) {
+					error(
+					    "Tilemap references tile #%" PRIu8
+					    " at (%zu, %zu), but the limit for bank %u is %" PRIu16,
+					    tileID,
+					    tx,
+					    ty,
 					    bank,
 					    options.maxNbTiles[bank]
 					);
 				}
 			}
 		} else {
-			for (auto id : *tilemap) {
+			size_t const limit = std::min<size_t>(nbTiles, options.maxNbTiles[0]);
-				if (id >= options.maxNbTiles[0]) {
+			for (size_t index = 0; index < mapSize; ++index) {
-					warning(
+				if (uint8_t tileID = (*tilemap)[index];
-					    "Tile #%" PRIu8 " was referenced, but the limit is %" PRIu16,
+				    static_cast<uint8_t>(tileID - options.baseTileIDs[0]) >= limit) {
-					    id,
+					size_t tx = index % width, ty = index / width;
-					    options.maxNbTiles[0]
+					error(
+					    "Tilemap references tile #%" PRIu8 " at (%zu, %zu), but the limit is %zu",
+					    tileID,
+					    tx,
+					    ty,
+					    limit
 					);
 				}
 			}
 		}
+
+		requireZeroErrors();
 	}
 
 	std::optional<DefaultInitVec<uint8_t>> palmap;
 	if (!options.palmap.empty()) {
 		palmap = readInto(options.palmap);
-		if (palmap->size() != nbTileInstances) {
+		if (palmap->size() != mapSize) {
 			fatal(
-			    "Palette map size (%zu tiles) doesn't match image's (%zu)",
+			    "Palette map size (%zu tiles) doesn't match image size (%zu)", palmap->size(), mapSize
-			    palmap->size(),
-			    nbTileInstances
 			);
 		}
 	}
@@ -381,15 +443,15 @@ void reverse() {
 		for (size_t tx = 0; tx < width; ++tx) {
 			size_t index = options.columnMajor ? ty + tx * height : ty * width + tx;
 			// By default, a tile is unflipped, in bank 0, and uses palette #0
-			uint8_t attribute = attrmap.has_value() ? (*attrmap)[index] : 0x00;
+			uint8_t attribute = attrmap ? (*attrmap)[index] : 0b0000;
-			bool bank = attribute & 0x08;
+			bool bank = attribute & 0b1000;
 			// Get the tile ID at this location
-			size_t tileID = index;
+			size_t tileOfs =
-			if (tilemap.has_value()) {
+			    tilemap ? static_cast<uint8_t>((*tilemap)[index] - options.baseTileIDs[bank])
-				tileID =
+			                  + (bank ? nbTilesInBank[0] : 0)
-				    (*tilemap)[index] - options.baseTileIDs[bank] + bank * options.maxNbTiles[0];
+			            : index;
-			}
+			// This should have been enforced by the earlier checking.
-			assume(tileID < nbTileInstances); // Should have been checked earlier
+			assume(tileOfs < nbTiles + options.trim);
 			size_t palID = palmap ? (*palmap)[index] : attribute & 0b111;
 			assume(palID < palettes.size()); // Should be ensured on data read
 
@@ -412,9 +474,8 @@ void reverse() {
 			    0x00,
 			    0x00,
 			};
-			uint8_t const *tileData = tileID > nbTileInstances - options.trim
+			uint8_t const *tileData =
-			                              ? trimmedTile.data()
+			    tileOfs >= nbTiles ? trimmedTile.data() : &tiles[tileOfs * tileSize];
-			                              : &tiles[tileID * tileSize];
 			auto const &palette = palettes[palID];
 			for (uint8_t y = 0; y < 8; ++y) {
 				// If vertically mirrored, fetch the bytes from the other end

test/gfx/.gitignore

@@ -6,6 +6,7 @@
 /*.rng
 # Generated by the test program
 /result.2bpp
+/result.tilemap
 /result.pal
 /result.attrmap
 /result.png

test/gfx/rgbgfx_test.cpp

@@ -432,10 +432,20 @@ int main(int argc, char *argv[]) {
 
 	{
 		char path[] = "../../rgbgfx", out_opt[] = "-o", out_file[] = "result.2bpp",
-		     pal_opt[] = "-p", pal_file[] = "result.pal", attr_opt[] = "-a",
+		     tmap_opt[] = "-t", tmap_file[] = "result.tilemap", pal_opt[] = "-p",
-		     attr_file[] = "result.attrmap", in_file[] = "out0.png";
+		     pal_file[] = "result.pal", attr_opt[] = "-a", attr_file[] = "result.attrmap",
+		     in_file[] = "out0.png";
 		std::vector<char *> args(
-		    {path, out_opt, out_file, pal_opt, pal_file, attr_opt, attr_file, in_file}
+		    {path,
+		     out_opt,
+		     out_file,
+		     tmap_opt,
+		     tmap_file,
+		     pal_opt,
+		     pal_file,
+		     attr_opt,
+		     attr_file,
+		     in_file}
 		);
 		// Also copy the trailing `nullptr`
 		std::copy_n(&argv[2], argc - 1, std::back_inserter(args));
@@ -449,8 +459,9 @@ int main(int argc, char *argv[]) {
 
 	{
 		char path[] = "../../rgbgfx", reverse_opt[] = "-r", out_opt[] = "-o",
-		     out_file[] = "result.2bpp", pal_opt[] = "-p", pal_file[] = "result.pal",
+		     out_file[] = "result.2bpp", tmap_opt[] = "-t", tmap_file[] = "result.tilemap",
-		     attr_opt[] = "-a", attr_file[] = "result.attrmap", in_file[] = "result.png";
+		     pal_opt[] = "-p", pal_file[] = "result.pal", attr_opt[] = "-a",
+		     attr_file[] = "result.attrmap", in_file[] = "result.png";
 		auto width_string = std::to_string(image0.getWidth() / 8);
 		std::vector<char *> args = {
 		    path,
@@ -458,6 +469,8 @@ int main(int argc, char *argv[]) {
 		    width_string.data(),
 		    out_opt,
 		    out_file,
+		    tmap_opt,
+		    tmap_file,
 		    pal_opt,
 		    pal_file,
 		    attr_opt,