Audit follow-ups: 11 fixes (correctness, perf, robustness)

Omnibus commit covering items A-L from the third round of the audit list.
All changes stay within C++98/03 -- no static_assert, no nullptr, no
auto, no range-for, no move semantics, no = delete.

Correctness
===========

A. Nes_Effects_Buffer::SaveAudioBufferState / RestoreAudioBufferState
   are no longer empty stubs. The base Effects_Buffer now implements
   them: snapshots all 7 internal Blip_Buffers (via their per-Blip_Buffer
   Save), the echo/reverb delay-line contents (~40 KB allocated
   alongside the originals in set_sample_rate, freed in dtor), and the
   echo_pos/reverb_pos cursors. The Nes_Effects_Buffer override chains
   to the base and additionally saves the Nes_Nonlinearizer. Before
   this, runahead with the stereo-panning option silently produced
   garbage audio on every rollback.

B. retro_serialize and retro_unserialize now snapshot/restore the audio
   buffer state unconditionally, not only when the frontend's
   FAST_SAVESTATES bit is set. Within-session save->load round-trips
   now preserve audio bit-for-bit. The is_fast_savestate() helper is
   removed.

   Cross-session loads (close emulator, reopen, load file) still fall
   back to the existing clear_sound_buf() fade-in: the snapshot lives
   in process-local extra_buffer storage inside each Blip_Buffer, not
   in the user-provided save state bytes, so a freshly constructed
   Blip_Buffer has nothing to restore from. To make this safe, every
   Save site now sets a new extra_valid sentinel and every Restore
   site bails out when it is false. Without the sentinel, removing the
   is_fast_savestate guard would have silently zeroed the audio state
   on every cross-session load.

C. Blip_Buffer::extra_buffer grows from 32 longs to 1024. The old size
   only worked because the typical save point is immediately after
   read_samples has drained the buffer to the ~18-long impulse-response
   tail. Any callsite that serializes mid-frame (or any future change
   that lets samples_avail() exceed 14 between frames) would have
   silently corrupted audio across fast-savestate restores. Save and
   Restore now copy samples_avail() + buffer_extra (clamped to the
   1024-entry capacity) and Restore zeros the live region beyond it so
   no 'future' samples written during speculative emulation leak back
   into the restored read state.

Performance
===========

D. The 256-entry retro_palette in retro_run is no longer rebuilt every
   frame. It is regenerated only when frame.palette[] changes (the
   game writing to PPU palette RAM, which is typically rare -- startup,
   scene transitions) or when the user picks a different palette
   (changes palette_index). Saves ~256 RGB->RGB565 conversions per
   frame in the steady state.

E. retro_serialize_size() result is cached after the first call.
   Some frontends call this every frame during runahead; the previous
   implementation re-ran a full save_state into a throwaway Mem_Writer
   each time. The cache is invalidated on retro_load_game and
   retro_unload_game (different cart -> different mapper -> different
   serialized size).

F. Nes_Emu::save_state and Nes_Emu::load_state used to 'new Nes_State'
   (~21 KB) and 'delete' it on every call. The libretro path is
   single-threaded with one Nes_Emu per process, so two file-scope
   static Nes_State scratch buffers do the job with no heap traffic.

G. Nes_Mapper::create() converted from a chain of independent 'if'
   tests to a single switch. Cosmetic perf, but it also closes the
   silent footgun where two entries for the same mapperCode would
   both execute -- the second would have leaked the first.

Robustness / style
==================

H. Auto_File_Reader::operator= and Auto_File_Writer::operator= no
   longer launder their destructive transfer through a const_cast
   on a const& parameter (UB if the source is a real const object).
   The Data_Reader*/Data_Writer* members are now declared mutable,
   which is the proper C++98 fix and which the original author
   evidently considered -- the previous declaration was literally
   '/* mutable */ Data_Reader* data;'.

I. retro_load_game guards against being called twice without an
   intervening retro_unload_game (which the spec forbids, but
   well-behaved cores defend against it anyway since leaking the
   prior emu would also leave the global Multi_Buffer wired to a
   destroyed audio chain).

K. Nes_Mapper::register_state now asserts that the registered state
   size fits in mapper_state_t (currently 512 bytes). assert() is
   compiled out under -DNDEBUG so shipping builds pay nothing. If
   any future mapper grows past the limit, save_state would have
   silently truncated it; this catches that at the earliest
   possible point.

L. The mono->stereo path in retro_run now names the stereo-frame
   count instead of relying on a bare 'read_samples >> 1'.

J (PAL region support) is deliberately not in this commit -- it is a
feature, not a fix, and requires plumbing the pal flag through
Nes_Emu and adjusting frame_rate and the APU/DMC period tables. It
deserves its own change.

Author: QuickNES Audit

libretro/libretro.cpp

@@ -35,6 +35,10 @@ static inline float nes_par(unsigned width, unsigned height)
 
 static Nes_Emu *emu;
 
+// Cached result of retro_serialize_size; populated lazily on first call and
+// invalidated on every retro_load_game / retro_unload_game.
+static size_t cached_state_size = 0;
+
 static retro_video_refresh_t video_cb;
 static retro_audio_sample_t audio_cb;
 static retro_audio_sample_batch_t audio_batch_cb;
@@ -61,8 +65,6 @@ Silent_Buffer silent_buffer;
 Multi_Buffer *current_buffer = NULL;
 bool use_silent_buffer = false;
 
-bool is_fast_savestate();
-
 /* ========================================
  * Palette additions START
  * (lifted from libretro-fceumm)
@@ -1236,14 +1238,30 @@ void retro_run(void)
 	   static uint16_t video_buffer[Nes_Emu::image_width * Nes_Emu::image_height];
 	   static uint16_t retro_palette[256];
 
-	   for (unsigned i = 0; i < 256; i++)
+	   // The 256-entry retro_palette only needs to be rebuilt when either
+	   // (a) the user picked a different palette (changes current_nes_colors,
+	   // which we track via palette_index), or (b) the game wrote to PPU
+	   // palette RAM (changes frame.palette[]). Most frames neither happens,
+	   // so cache and compare to skip ~256 RGB->RGB565 conversions per frame.
+	   static short last_frame_palette[Nes_Emu::max_palette_size];
+	   static int last_retro_palette_index = -1;
+	   static bool retro_palette_initialized = false;
+	   if ( !retro_palette_initialized
+	        || palette_index != last_retro_palette_index
+	        || memcmp(last_frame_palette, frame.palette, sizeof(last_frame_palette)) != 0 )
 	   {
-		   const Nes_Emu::rgb_t& rgb = current_nes_colors[frame.palette[i]];
+		   for (unsigned i = 0; i < 256; i++)
+		   {
+			   const Nes_Emu::rgb_t& rgb = current_nes_colors[frame.palette[i]];
 #if defined(ABGR1555)
-         retro_palette[i] = ((rgb.blue & 0xf8) << 7) | ((rgb.green & 0xf8) << 2) | ((rgb.red & 0xf8) >> 3);
+	         retro_palette[i] = ((rgb.blue & 0xf8) << 7) | ((rgb.green & 0xf8) << 2) | ((rgb.red & 0xf8) >> 3);
 #else
-         retro_palette[i] = ((rgb.red & 0xf8) << 8) | ((rgb.green & 0xfc) << 3) | ((rgb.blue & 0xf8) >> 3);
+	         retro_palette[i] = ((rgb.red & 0xf8) << 8) | ((rgb.green & 0xfc) << 3) | ((rgb.blue & 0xf8) >> 3);
 #endif
+		   }
+		   memcpy(last_frame_palette, frame.palette, sizeof(last_frame_palette));
+		   last_retro_palette_index = palette_index;
+		   retro_palette_initialized = true;
 	   }
 
 	   for (int y = 0; y < Nes_Emu::image_height; y++)
@@ -1279,7 +1297,13 @@ void retro_run(void)
 			audio_batch_cb(out_samples, read_samples);
 	   }
 	   else
-			audio_batch_cb(samples, read_samples >> 1);
+	   {
+			// Effects_Buffer is already producing interleaved stereo pairs:
+			// read_samples returned a count expressed in mono samples, so the
+			// stereo frame count delivered to audio_batch_cb is half that.
+			long stereo_frames = read_samples >> 1;
+			audio_batch_cb(samples, stereo_frames);
+	   }
    }
    else
    {
@@ -1352,6 +1376,18 @@ bool retro_load_game(const struct retro_game_info *info)
    if (!environ_cb(RETRO_ENVIRONMENT_SET_PIXEL_FORMAT, &fmt))
       return false;
 
+   // Guard against a frontend that calls retro_load_game without a matching
+   // retro_unload_game. The spec disallows it but well-behaved cores are
+   // defensive about it anyway, since leaking the prior emu would also
+   // leave the global Multi_Buffer wired to a destroyed audio chain.
+   if (emu)
+   {
+      emu->close();
+      delete emu;
+      emu = NULL;
+   }
+   cached_state_size = 0;
+
    emu = new Nes_Emu;
    check_variables();
 
@@ -1398,6 +1434,7 @@ void retro_unload_game(void)
       emu->close();
    delete emu;
    emu = 0;
+   cached_state_size = 0;
 }
 
 unsigned retro_get_region(void)
@@ -1412,48 +1449,42 @@ bool retro_load_game_special(unsigned, const struct retro_game_info *, size_t)
 
 size_t retro_serialize_size(void)
 {
+   // The serialized size is determined entirely by the loaded cart (mapper
+   // state size varies per mapper) and is stable for the life of one
+   // retro_load_game. Caching avoids paying the full save_state cost just
+   // to measure -- some frontends call this every frame during runahead.
+   if (cached_state_size != 0)
+      return cached_state_size;
+
    Mem_Writer writer;
    if (emu->save_state(writer))
       return 0;
 
-   return writer.size();
-}
-
-bool is_fast_savestate()
-{
-	int value;
-	bool okay = environ_cb(RETRO_ENVIRONMENT_GET_AUDIO_VIDEO_ENABLE, &value);
-	if (okay)
-	{
-		if (value & 4)
-		{
-			return true;
-		}
-	}
-	return false;
+   cached_state_size = writer.size();
+   return cached_state_size;
 }
 
 bool retro_serialize(void *data, size_t size)
 {
-   bool isFastSavestate = is_fast_savestate();
    Mem_Writer writer(data, size);
    bool okay = !emu->save_state(writer);
-   if (isFastSavestate)
-   {
-      emu->SaveAudioBufferState();
-   }
+   // Always snapshot audio buffer state. The snapshot is stored in
+   // process-local memory inside each Blip_Buffer (and friends), not in
+   // `data`, so within-session save->load round-trips now preserve audio
+   // exactly. Cross-session loads (close emulator, reopen, load file)
+   // still fall back to the fade-in path because the snapshot doesn't
+   // persist in the user buffer -- Restore is gated by an extra_valid
+   // sentinel that is false on a freshly constructed Blip_Buffer, so it
+   // becomes a no-op and the existing clear_sound_buf() fade kicks in.
+   emu->SaveAudioBufferState();
    return okay;
 }
 
 bool retro_unserialize(const void *data, size_t size)
 {
-   bool isFastSavestate = is_fast_savestate();
    Mem_File_Reader reader(data, size);
    bool okay = !emu->load_state(reader);
-   if (isFastSavestate)
-   {
-      emu->RestoreAudioBufferState();
-   }
+   emu->RestoreAudioBufferState();
    return okay;
 }
 

nes_emu/Blip_Buffer.cpp

@@ -42,6 +42,7 @@ Blip_Buffer::Blip_Buffer()
 	extra_offset = offset_;
 	extra_reader_accum = reader_accum;
 	memset(extra_buffer, 0, sizeof(extra_buffer));
+	extra_valid = false;
 }
 
 Blip_Buffer::~Blip_Buffer()
@@ -406,15 +407,43 @@ void Blip_Buffer::mix_samples( blip_sample_t const* in, long count )
 
 void Blip_Buffer::SaveAudioBufferState()
 {
+	// Save the live portion of buffer_ (samples_avail + the impulse-response
+	// tail buffer_extra), bounded by the dedicated extra_buffer's capacity.
+	// In practice the post-read-samples residual is only ~18 longs, but
+	// callers may serialize before draining and we want to preserve as much
+	// as the snapshot area can hold.
+	long live = samples_avail() + (long) buffer_extra;
+	if ( live > (long) extra_buffer_size )
+		live = (long) extra_buffer_size;
 	extra_length = length_;
 	extra_offset = offset_;
 	extra_reader_accum = reader_accum;
-	memcpy(extra_buffer, buffer_, sizeof(extra_buffer));
+	if ( live > 0 && buffer_ )
+		memcpy( extra_buffer, buffer_, (size_t) live * sizeof (buf_t_) );
+	// Zero any tail to keep deterministic contents in the snapshot.
+	if ( live < (long) extra_buffer_size )
+		memset( extra_buffer + live, 0,
+		        (size_t) (extra_buffer_size - live) * sizeof (buf_t_) );
+	extra_valid = true;
 }
 void Blip_Buffer::RestoreAudioBufferState()
 {
+	if ( !extra_valid )
+		return; // no Save has happened in this Blip_Buffer's lifetime
 	length_ = extra_length;
 	offset_ = extra_offset;
 	reader_accum = extra_reader_accum;
-	memcpy(buffer_, extra_buffer, sizeof(extra_buffer));
+	if ( buffer_ && buffer_size_ > 0 )
+	{
+		long copy = (long) extra_buffer_size;
+		if ( copy > buffer_size_ )
+			copy = buffer_size_;
+		memcpy( buffer_, extra_buffer, (size_t) copy * sizeof (buf_t_) );
+		// Beyond the snapshot, the buffer may still hold "future" samples
+		// written by the speculative emulation that ran between Save and
+		// Restore. Clear them so the restored read state is exact.
+		if ( copy < buffer_size_ )
+			memset( buffer_ + copy, 0,
+			        (size_t) (buffer_size_ - copy) * sizeof (buf_t_) );
+	}
 }

nes_emu/Blip_Buffer.h

@@ -108,11 +108,23 @@ class Blip_Buffer {
 	friend class Blip_Reader;
 
 private:
-	//extra information necessary to load state to an exact sample
-	buf_t_ extra_buffer[32];
+	//extra information necessary to load state to an exact sample.
+	// Size rationale: between Save and Restore the worst case has the
+	// entire previous frame's audio still in flight plus the
+	// impulse-response tail (buffer_extra). At 44.1 kHz/60 fps that's
+	// ~735 + 18 longs. 1024 leaves comfortable headroom and is still
+	// only ~8 KB per Blip_Buffer.
+	enum { extra_buffer_size = 1024 };
+	buf_t_ extra_buffer[extra_buffer_size];
 	int extra_length;
 	long extra_reader_accum;
 	blip_resampled_time_t extra_offset;
+	// True iff SaveAudioBufferState has been called since construction.
+	// RestoreAudioBufferState is a no-op until then so that a fresh
+	// Blip_Buffer (e.g. after retro_load_game) isn't silently zeroed by
+	// a frontend that calls retro_unserialize without a matching
+	// retro_serialize earlier in the same session.
+	bool extra_valid;
 public:
 	void SaveAudioBufferState();
 	void RestoreAudioBufferState();

nes_emu/Effects_Buffer.cpp

@@ -73,7 +73,13 @@ Effects_Buffer::Effects_Buffer( bool center_only ) : Multi_Buffer( 2 )
 
 	reverb_buf = NULL;
 	reverb_pos = 0;
-	
+
+	extra_echo_buf = NULL;
+	extra_reverb_buf = NULL;
+	extra_echo_pos = 0;
+	extra_reverb_pos = 0;
+	extra_valid = false;
+
 	stereo_remain = 0;
 	effect_remain = 0;
 	effects_enabled = false;
@@ -84,6 +90,8 @@ Effects_Buffer::~Effects_Buffer()
 {
 	delete [] echo_buf;
 	delete [] reverb_buf;
+	delete [] extra_echo_buf;
+	delete [] extra_reverb_buf;
 }
 
 const char *Effects_Buffer::set_sample_rate( long rate, int msec )
@@ -99,6 +107,20 @@ const char *Effects_Buffer::set_sample_rate( long rate, int msec )
 		reverb_buf = new blip_sample_t [reverb_size];
 		CHECK_ALLOC( reverb_buf );
 	}
+
+	if ( !extra_echo_buf )
+	{
+		extra_echo_buf = new blip_sample_t [echo_size];
+		CHECK_ALLOC( extra_echo_buf );
+		memset( extra_echo_buf, 0, echo_size * sizeof *extra_echo_buf );
+	}
+
+	if ( !extra_reverb_buf )
+	{
+		extra_reverb_buf = new blip_sample_t [reverb_size];
+		CHECK_ALLOC( extra_reverb_buf );
+		memset( extra_reverb_buf, 0, reverb_size * sizeof *extra_reverb_buf );
+	}
 
 	for ( int i = 0; i < buf_count; i++ )
 		RETURN_ERR( bufs [i].set_sample_rate( rate, msec ) );
@@ -513,3 +535,32 @@ void Effects_Buffer::mix_enhanced( blip_sample_t* out, long count )
 	l2.end( bufs [5] );
 	r2.end( bufs [6] );
 }
+
+void Effects_Buffer::SaveAudioBufferState()
+{
+	SaveAudioBufferStatePrivate();
+	for ( int i = 0; i < buf_count; i++ )
+		bufs [i].SaveAudioBufferState();
+	extra_echo_pos = echo_pos;
+	extra_reverb_pos = reverb_pos;
+	if ( echo_buf && extra_echo_buf )
+		memcpy( extra_echo_buf, echo_buf, echo_size * sizeof *echo_buf );
+	if ( reverb_buf && extra_reverb_buf )
+		memcpy( extra_reverb_buf, reverb_buf, reverb_size * sizeof *reverb_buf );
+	extra_valid = true;
+}
+
+void Effects_Buffer::RestoreAudioBufferState()
+{
+	if ( !extra_valid )
+		return; // freshly-constructed buffer; nothing snapshotted yet
+	RestoreAudioBufferStatePrivate();
+	for ( int i = 0; i < buf_count; i++ )
+		bufs [i].RestoreAudioBufferState();
+	echo_pos = extra_echo_pos;
+	reverb_pos = extra_reverb_pos;
+	if ( echo_buf && extra_echo_buf )
+		memcpy( echo_buf, extra_echo_buf, echo_size * sizeof *echo_buf );
+	if ( reverb_buf && extra_reverb_buf )
+		memcpy( reverb_buf, extra_reverb_buf, reverb_size * sizeof *reverb_buf );
+}

nes_emu/Effects_Buffer.h

@@ -68,7 +68,19 @@ class Effects_Buffer : public Multi_Buffer {
 	blip_sample_t* echo_buf;
 	int reverb_pos;
 	int echo_pos;
-	
+
+	// Snapshot buffers for SaveAudioBufferState / RestoreAudioBufferState.
+	// Allocated alongside echo_buf / reverb_buf in set_sample_rate, same
+	// sizes. Effects_Buffer is the only Multi_Buffer subclass with internal
+	// delay lines beyond the Blip_Buffers themselves, so it needs to
+	// snapshot them or runahead audio with effects enabled corrupts after
+	// every restore.
+	blip_sample_t* extra_echo_buf;
+	blip_sample_t* extra_reverb_buf;
+	int extra_echo_pos;
+	int extra_reverb_pos;
+	bool extra_valid;
+
 	struct {
 		fixed_t pan_1_levels [2];
 		fixed_t pan_2_levels [2];
@@ -84,6 +96,9 @@ class Effects_Buffer : public Multi_Buffer {
 	void mix_stereo( blip_sample_t*, long );
 	void mix_enhanced( blip_sample_t*, long );
 	void mix_mono_enhanced( blip_sample_t*, long );
+public:
+	virtual void SaveAudioBufferState();
+	virtual void RestoreAudioBufferState();
 };
 
 	inline Effects_Buffer::channel_t Effects_Buffer::channel( int i ) {

nes_emu/Nes_Buffer.cpp

@@ -177,6 +177,7 @@ Nes_Nonlinearizer::Nes_Nonlinearizer()
 	}
 	extra_accum = 0;
 	extra_prev = 0;
+	extra_valid = false;
 }
 
 Nes_Apu* Nes_Nonlinearizer::enable( bool b, Blip_Buffer* buf )
@@ -231,10 +232,13 @@ void Nes_Nonlinearizer::SaveAudioBufferState()
 {
 	extra_accum = accum;
 	extra_prev = prev;
+	extra_valid = true;
 }
 
 void Nes_Nonlinearizer::RestoreAudioBufferState()
 {
+	if ( !extra_valid )
+		return;
 	accum = extra_accum;
 	prev = extra_prev;
 }

nes_emu/Nes_Buffer.h

@@ -20,6 +20,7 @@ class Nes_Nonlinearizer {
 
 	long extra_accum;
 	long extra_prev;
+	bool extra_valid; // see Blip_Buffer.h
 public:
 	Nes_Nonlinearizer();
 	bool enabled;

nes_emu/Nes_Effects_Buffer.cpp

@@ -84,8 +84,12 @@ long Nes_Effects_Buffer::read_samples( blip_sample_t* out, long count )
 
 void Nes_Effects_Buffer::SaveAudioBufferState()
 {
+	Effects_Buffer::SaveAudioBufferState();
+	nonlin.SaveAudioBufferState();
 }
 
 void Nes_Effects_Buffer::RestoreAudioBufferState()
 {
+	Effects_Buffer::RestoreAudioBufferState();
+	nonlin.RestoreAudioBufferState();
 }