games/NXDoom: RGB565 support, loadable-module conversion, and crash fixes

games/NXDoom/Kconfig

@@ -4,7 +4,7 @@
 #
 
 config GAMES_NXDOOM
-	bool "NXDoom"
+	tristate "NXDoom"
 	default n
 	depends on ALLOW_GPL_COMPONENTS
 	depends on VIDEO_FB
@@ -178,6 +178,26 @@ config GAMES_NXDOOM_MAXDRAWSEGS
 		memory, so you may reduce the number. However, too few will cause
 		rendering issues (overflow is checked to avoid crashes).
 
+config GAMES_NXDOOM_HEAP_BUFFERS
+	bool "Allocate renderer scratch buffers on the heap"
+	default n
+	---help---
+		The visplanes/openings/drawsegs/vissprites renderer scratch buffers
+		(sized by the options above) are static arrays by default, matching
+		vanilla DOOM. On a target where their combined size threatens the
+		internal DRAM budget once linked into a full application image,
+		enable this to allocate them from the heap instead (this target's
+		heap may be backed by external RAM/PSRAM). Static allocation is
+		preferred where DRAM budget is not a concern.
+
+config GAMES_NXDOOM_STATDUMP_MAX_CAPTURES
+	int "Maximum statdump capture buffer entries"
+	default 32
+	---help---
+		Number of playtime-statistics capture slots statdump.c reserves.
+		This is diagnostic/debug capture storage, not required for normal
+		gameplay - reduce it on a DRAM-constrained target.
+
 config GAMES_NXDOOM_RANGECHECK
 	bool "Perform range checks"
 	default y

games/NXDoom/src/d_iwad.c

@@ -271,6 +271,16 @@ static void buld_iwad_dir_list(void)
 
   add_iwad_dir(m_dir_name(myargv[0]));
 
+  /* Add the board's configured DOOM data directory.  Kconfig documents
+   * CONFIG_GAMES_NXDOOM_PREFDIR as "Directory where DOOM WAD files are
+   * stored", but until now it was only used for the config/save file
+   * location -- nothing actually searched it for IWADs, forcing every
+   * launch to rely on the current directory or DOOMWADDIR/DOOMWADPATH
+   * being set by hand first.
+   */
+
+  add_iwad_dir(CONFIG_GAMES_NXDOOM_PREFDIR);
+
   /* Add DOOMWADDIR if it is in the environment */
 
   env = getenv("DOOMWADDIR");

games/NXDoom/src/doom/r_bsp.c

@@ -78,7 +78,11 @@ line_t *linedef;
 sector_t *frontsector;
 sector_t *backsector;
 
+#ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS
+drawseg_t *drawsegs;
+#else
 drawseg_t drawsegs[CONFIG_GAMES_NXDOOM_MAXDRAWSEGS];
+#endif
 drawseg_t *ds_p;
 
 /* newend is one past the last valid seg */

games/NXDoom/src/doom/r_bsp.h

@@ -52,7 +52,11 @@ extern boolean markceiling;
 
 extern boolean skymap;
 
+#ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS
+extern drawseg_t *drawsegs;
+#else
 extern drawseg_t drawsegs[CONFIG_GAMES_NXDOOM_MAXDRAWSEGS];
+#endif
 extern drawseg_t *ds_p;
 
 extern lighttable_t **hscalelight;

games/NXDoom/src/doom/r_main.c

@@ -685,6 +685,24 @@ fixed_t r_scale_from_global_angle(angle_t visangle)
 
 void r_set_view_size(int blocks, int detail)
 {
+  /* screenblocks is only ever meant to hold 3..11 (set that way by the
+   * options menu and by the config default of 9).  The renderer's view
+   * geometry math divides by values derived from it - notably
+   * pspriteiscale = FRACUNIT * SCREENWIDTH / viewwidth in
+   * r_execute_set_view_size() - so a 0 or otherwise out-of-range value
+   * turns into a divide-by-zero hardware exception (EXCCAUSE=6), which on
+   * this flat-memory build takes the whole board down rather than just
+   * this task.  Clamp defensively so a bad/missing config value degrades
+   * to the default screen size instead of a system crash.
+   */
+
+  if (blocks < 3 || blocks > 11)
+    {
+      printf("r_set_view_size: screenblocks=%d out of range, using 10\n",
+            blocks);
+      blocks = 10;
+    }
+
   setsizeneeded = true;
   setblocks = blocks;
   setdetail = detail;

games/NXDoom/src/doom/r_plane.c

@@ -57,12 +57,17 @@ planefunction_t ceilingfunc;
 
 /* Here comes the obnoxious "visplane". */
 
+#ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS
+visplane_t *visplanes;
+short *openings;
+#else
 visplane_t visplanes[CONFIG_GAMES_NXDOOM_MAXVISPLANES];
+short openings[MAXOPENINGS];
+#endif
 visplane_t *lastvisplane;
 visplane_t *floorplane;
 visplane_t *ceilingplane;
 
-short openings[MAXOPENINGS];
 short *lastopening;
 
 /* Clip values are the solid pixel bounding the range. floorclip starts out
@@ -114,12 +119,34 @@ static void r_map_plane(int y, int x1, int x2)
   fixed_t length;
   unsigned index;
 
-#ifdef CONFIG_GAMES_NXDOOM_RANGECHECK
-  if (x2 < x1 || x1 < 0 || x2 >= viewwidth || y > viewheight)
+  /* y indexes cachedheight[]/cacheddistance[]/cachedxstep[]/cachedystep[]
+   * below, all sized SCREENHEIGHT - a y outside that range (observed on
+   * this port: y=255 against a 200-entry array, well past even
+   * viewheight) is an out-of-bounds array write, not just a "debug
+   * assertion".  This used to be gated behind CONFIG_GAMES_NXDOOM_
+   * RANGECHECK and fatal (i_error(), which tears down the whole process
+   * on what vanilla Doom would just render as one glitched span) - both
+   * wrong: the memory-safety check must not be optional, and killing the
+   * entire game over one bad plane span is worse than just not drawing
+   * it.  Clamp y into range instead of touching memory outside the
+   * buffers' real bounds - this still renders the span (as one glitched
+   * row, the same "wrong but visible" failure mode vanilla DOOM has) so
+   * a bad plane doesn't leave a blank gap on screen either.
+   */
+
+  if (x2 < x1 || x1 < 0 || x2 >= viewwidth)
     {
-      i_error("R_MapPlane: %i, %i at %i", x1, x2, y);
+      return;
+    }
+
+  if (y < 0)
+    {
+      y = 0;
+    }
+  else if (y >= SCREENHEIGHT)
+    {
+      y = SCREENHEIGHT - 1;
     }
-#endif
 
   if (planeheight != cachedheight[y])
     {
@@ -195,7 +222,55 @@ static void r_make_spans(int x, int t1, int b1, int t2, int b2)
 
 void r_init_planes(void)
 {
-  /* Doh! */
+#ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS
+  /* These renderer scratch buffers are sized for a comfortable margin
+   * above vanilla DOOM's original limits and, on a DRAM-constrained
+   * target, blow the internal DRAM budget as static arrays - opt-in
+   * heap allocation instead (comes out of the PSRAM-backed user heap
+   * on this target) via CONFIG_GAMES_NXDOOM_HEAP_BUFFERS.
+   */
+
+  visplanes = malloc(sizeof(visplane_t) * CONFIG_GAMES_NXDOOM_MAXVISPLANES);
+  openings = malloc(sizeof(short) * MAXOPENINGS);
+  drawsegs = malloc(sizeof(drawseg_t) * CONFIG_GAMES_NXDOOM_MAXDRAWSEGS);
+  vissprites = malloc(sizeof(vissprite_t) *
+                       CONFIG_GAMES_NXDOOM_MAXVISSPRITES);
+
+  if (visplanes == NULL || openings == NULL || drawsegs == NULL ||
+      vissprites == NULL)
+    {
+      i_error("r_init_planes: failed to allocate renderer buffers");
+    }
+
+  /* i_quit() can be followed by another r_init_planes() call within the
+   * same boot (relaunching the game via nxpkg on this flat, single
+   * address-space build), so these heap buffers must be freed on exit
+   * or every relaunch leaks the previous allocation permanently.
+   */
+
+  i_at_exit(r_shutdown_planes, true);
+#endif
+}
+
+/* r_shutdown_planes
+ * Frees the renderer scratch buffers allocated by r_init_planes.  Only
+ * registered as an exit handler when CONFIG_GAMES_NXDOOM_HEAP_BUFFERS is
+ * set - the static-array buffers have nothing to free.
+ */
+
+void r_shutdown_planes(void)
+{
+#ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS
+  free(visplanes);
+  free(openings);
+  free(drawsegs);
+  free(vissprites);
+
+  visplanes = NULL;
+  openings = NULL;
+  drawsegs = NULL;
+  vissprites = NULL;
+#endif
 }
 
 /* r_clear_planes

games/NXDoom/src/doom/r_plane.h

@@ -58,6 +58,7 @@ extern fixed_t distscale[SCREENWIDTH];
  ****************************************************************************/
 
 void r_init_planes(void);
+void r_shutdown_planes(void);
 void r_clear_planes(void);
 
 void r_draw_planes(void);

games/NXDoom/src/doom/r_things.c

@@ -93,7 +93,11 @@ spriteframe_t sprtemp[29];
 int maxframe;
 const char *spritename;
 
+#ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS
+vissprite_t *vissprites;
+#else
 vissprite_t vissprites[CONFIG_GAMES_NXDOOM_MAXVISSPRITES];
+#endif
 vissprite_t *vissprite_p;
 int newvissprite;
 

games/NXDoom/src/doom/r_things.h

@@ -28,7 +28,11 @@
  * Public Data
  ****************************************************************************/
 
+#ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS
+extern vissprite_t *vissprites;
+#else
 extern vissprite_t vissprites[CONFIG_GAMES_NXDOOM_MAXVISSPRITES];
+#endif
 extern vissprite_t *vissprite_p;
 extern vissprite_t vsprsortedhead;
 

games/NXDoom/src/doom/statdump.c

@@ -39,7 +39,7 @@
  * Pre-processor Definitions
  ****************************************************************************/
 
-#define MAX_CAPTURES 32
+#define MAX_CAPTURES CONFIG_GAMES_NXDOOM_STATDUMP_MAX_CAPTURES
 
 /****************************************************************************
  * Private Data

games/NXDoom/src/i_main.c

@@ -57,17 +57,28 @@ void d_doom_main(void);
 
 int main(int argc, char **argv)
 {
-  /* save arguments */
+  /* save arguments
+   *
+   * +1 and an explicit NULL terminator: argv is conventionally
+   * NULL-terminated at argv[argc] (this is what the OS/exec path
+   * guarantees for the `argv` parameter above), and some of this
+   * codebase's own argument handling was written assuming that holds
+   * for myargv too - an under-sized allocation here leaves myargv[argc]
+   * pointing at whatever the allocator happens to return next, which
+   * only reads as "probably zero" by chance depending on heap layout.
+   */
 
   myargc = argc;
-  myargv = malloc(argc * sizeof(char *));
+  myargv = malloc((argc + 1) * sizeof(char *));
   assert(myargv != NULL);
 
   for (int i = 0; i < argc; i++)
     {
       myargv[i] = m_string_duplicate(argv[i]);
     }
 
+  myargv[argc] = NULL;
+
   /* Print the program version and exit. */
 
   if (m_parm_exists("-version") || m_parm_exists("--version"))

games/NXDoom/src/i_video.c

@@ -92,6 +92,13 @@ struct graphics_state_s
 
   uint8_t scale;
 
+  /* Pixel offset to center the scaled game viewport within the frame
+   * buffer when the buffer is larger than SCREENWIDTH/HEIGHT * scale.
+   */
+
+  int xoffset;
+  int yoffset;
+
   bool inited; /* Track initialization */
 };
 
@@ -262,13 +269,25 @@ static void blit_screen(void)
   uint8_t p_idx;
   void *fbptr;
 
-  /* TODO: It would be best to do this more efficiently/with less memory.
-   * It also would be good if we could handle the palette translation here
-   * such that DOOM can be played on frame buffers with differing bit depths
-   * and pixel formats.
-   */
+  /* TODO: It would be best to do this more efficiently/with less memory. */
+
+  if (g_graphics_state.pinfo.bpp != 16 && g_graphics_state.pinfo.bpp != 32)
+    {
+      /* The xoffset/stride math below assumes one of those two pixel
+       * sizes, so continuing would read/write past the intended pixel
+       * bounds on the very first frame.  Fail loudly instead of
+       * silently corrupting framebuffer memory.
+       */
+
+      i_error("Unsupported framebuffer depth: %u bpp",
+              g_graphics_state.pinfo.bpp);
+    }
+
+  fbptr = g_graphics_state.fbmem +
+          g_graphics_state.yoffset * g_graphics_state.pinfo.stride +
+          g_graphics_state.xoffset *
+              (g_graphics_state.pinfo.bpp == 16 ? 2 : 4);
 
-  fbptr = g_graphics_state.fbmem;
   for (unsigned y = 0; y < SCREENHEIGHT * g_graphics_state.scale; y++)
     {
       for (unsigned x = 0; x < SCREENWIDTH * g_graphics_state.scale; x++)
@@ -277,9 +296,18 @@ static void blit_screen(void)
                       .scrnbuf[(y / g_graphics_state.scale) * SCREENWIDTH +
                                (x / g_graphics_state.scale)];
 
-          ((uint32_t *)(fbptr))[x] =
-              ARGBTO32(g_palette[p_idx].a, g_palette[p_idx].r,
-                       g_palette[p_idx].g, g_palette[p_idx].b);
+          if (g_graphics_state.pinfo.bpp == 16)
+            {
+              ((uint16_t *)(fbptr))[x] =
+                  RGBTO16(g_palette[p_idx].r, g_palette[p_idx].g,
+                          g_palette[p_idx].b);
+            }
+          else
+            {
+              ((uint32_t *)(fbptr))[x] =
+                  ARGBTO32(g_palette[p_idx].a, g_palette[p_idx].r,
+                           g_palette[p_idx].g, g_palette[p_idx].b);
+            }
         }
 
       fbptr += g_graphics_state.pinfo.stride;
@@ -724,6 +752,18 @@ void i_init_graphics(void)
   yscale = g_graphics_state.vinfo.yres / SCREENHEIGHT;
   g_graphics_state.scale = xscale > yscale ? yscale : xscale;
 
+  /* Center the scaled viewport within the frame buffer rather than
+   * pinning it to the top-left corner, since the buffer is typically
+   * larger than SCREENWIDTH/HEIGHT * scale.
+   */
+
+  g_graphics_state.xoffset =
+      (g_graphics_state.vinfo.xres -
+       SCREENWIDTH * g_graphics_state.scale) / 2;
+  g_graphics_state.yoffset =
+      (g_graphics_state.vinfo.yres -
+       SCREENHEIGHT * g_graphics_state.scale) / 2;
+
   /* Get frame buffer plane info */
 
   if (ioctl(g_graphics_state.fd, FBIOGET_PLANEINFO,