--- a/src/MacSrc/OpenGL.cc +++ b/src/MacSrc/OpenGL.cc @@ -41,6 +41,7 @@ #include #include +#include struct CachedTexture { SDL_Surface *bitmap; @@ -83,8 +84,23 @@ static SDL_Palette *opaquePalette; static SDL_Palette *transparentPalette; -// Texture cache to keep SDL surfaces and GL textures in memory -static std::map texturesByBitsPtr; +// Texture cache to keep SDL surfaces and GL textures in memory. Keyed by the +// full (bits, w, h) tuple, not a packed uint64_t - bm->bits is a full +// pointer, and a 64-bit Linux heap/mmap address routinely has non-zero bits +// in the 32-47 range, so OR-packing w/h on top of it (as this used to do) +// can collide: a genuinely different (bits, w, h) triple produces the same +// packed key as a cached entry for a different bitmap, so a lookup reports a +// false "hit" and set_texture() reuses that entry's SDL surfaces - sized for +// the old entry's own dimensions - for an SDL_memmove and glTexImage2D using +// the *current* bitmap's byte count, corrupting the result with a buffer +// overflow or leftover stale pixel data from whatever was cached before. +// bm->bits itself is a recycled slot in the resource system's own bounded +// LRU cache, not a stable per-resource address, so a collision is a real +// risk in practice, not just theoretical - most noticeable with content +// whose per-frame bitmap dimensions vary a lot (critter animations with +// different poses per frame, especially after an AI upscale mod re-exports +// each frame at its own independently-upscaled size). +static std::map, CachedTexture> texturesByBitsPtr; static float view_scale; static int phys_width; @@ -577,7 +593,7 @@ bind_texture(toCache.texture); glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, bm->w, bm->h, 0, GL_RGBA, GL_UNSIGNED_BYTE, toCache.converted->pixels); - texturesByBitsPtr[(uint64_t)bm->bits | ((uint64_t)bm->w << 32u) | ((uint64_t)bm->h << 48u)] = toCache; + texturesByBitsPtr[std::make_tuple(bm->bits, (int)bm->w, (int)bm->h)] = toCache; return true; } @@ -589,7 +605,7 @@ } static CachedTexture *opengl_get_texture(grs_bitmap *bm) { - auto iter = texturesByBitsPtr.find((uint64_t)bm->bits | ((uint64_t)bm->w << 32u) | ((uint64_t)bm->h << 48u)); + auto iter = texturesByBitsPtr.find(std::make_tuple(bm->bits, (int)bm->w, (int)bm->h)); if (iter != texturesByBitsPtr.end()) { return &iter->second; } @@ -685,13 +701,27 @@ isDirty = true; } } else { + // t->bitmap wraps its source pixels rather than owning a copy (see + // convert_texture()), and a cache hit means bm->bits is by + // definition the same pointer this entry was first cached with + // (opengl_get_texture()'s key includes it) - so memmove-ing + // bm->bits into t->bitmap->pixels used to be a no-op self-copy, + // never actually refreshing anything. bm->bits is a slot in the + // resource system's own bounded LRU cache, reused for different + // content across draws while its address stays the same, so a hit + // here doesn't mean the pixels are still current - only that this + // entry's GL texture object and converted-surface buffer are the + // right size to reuse. Re-pointing at the current source instead + // keeps every other field (dimensions, palette, clip rect) intact + // while making the surface actually see this draw's real pixels. if (bm->type == BMT_RSD8) { grs_bitmap decoded; gr_rsd8_convert(bm, &decoded); - SDL_memmove(t->bitmap->pixels, decoded.bits, bm->w * bm->h); + t->bitmap->pixels = decoded.bits; } else { - SDL_memmove(t->bitmap->pixels, bm->bits, bm->w * bm->h); + t->bitmap->pixels = bm->bits; } + t->bitmap->pitch = bm->row; setPaletteForBitmap(bm, t->bitmap); SDL_BlitSurface(t->bitmap, nullptr, t->converted, nullptr);