From f8744f0392ee94ec45bd96ea8a094a4dade837a7 Mon Sep 17 00:00:00 2001 From: megboyzz Date: Sat, 19 Sep 2026 13:59:53 +0300 Subject: [PATCH] Fix garbled glyphs: bounce buffer for AndroidBitmap_lockPixels Shim_AndroidBitmap_lockPixels handed the guest a NULL pixel pointer whenever the real buffer lived outside the guest region, on the documented assumption that "texture decode FROM a Bitmap ... reads pixels via other means, not by writing through this specific pointer". Measurement disproved that assumption. The glyph atlas path is exactly a read through this pointer: AndroidBitmap_lockPixels rc=0 hostAddr=0x... -> guest=0x0 bitmap 1024x1024 stride=4096 format=1 (RGBA_8888) 1024x1024 is precisely the atlas size independently derived from the glyph UVs (u1 = 35/1024 for a 35px glyph), which is why glyph positions, sizes, pen advance and UVs all measured correct while the pixels were coloured noise: BitmapGraphics rendered the glyphs fine, the guest asked for them, got nothing, and uploaded guest address 0 as the font texture. Fixed with a bounce buffer - real pixels copied into a guest-visible buffer on lock, copied back and freed on unlock. The copy-back also closes the write direction the original comment called a known gap. Costs one height*stride copy each way, a handful of times per session. Verified in both directions on the Pixel 6a: text renders correctly, and the added "no bounce buffer was made" warning fired zero times, so every lock genuinely got a buffer rather than silently falling back to the old NULL path. Co-Authored-By: Claude --- mpcore/src/main/cpp/emu/gles_shim.cpp | 94 ++++++++++++++++++++++++--- 1 file changed, 84 insertions(+), 10 deletions(-) diff --git a/mpcore/src/main/cpp/emu/gles_shim.cpp b/mpcore/src/main/cpp/emu/gles_shim.cpp index 5e4914a..d8d2762 100644 --- a/mpcore/src/main/cpp/emu/gles_shim.cpp +++ b/mpcore/src/main/cpp/emu/gles_shim.cpp @@ -183,26 +183,100 @@ uint32_t Shim_AndroidBitmap_getInfo(GuestEngine& eng, uint32_t, uint32_t r1, uin if (r2) memcpy(eng.G2H(r2), &info, sizeof(AndroidBitmapInfo)); return (uint32_t)rc; } +// FIX (2026-09-19, task #41 - garbled glyphs). The version below used to +// hand the guest a NULL pixel pointer whenever the real buffer lived outside +// the guest region, on the documented assumption that "texture decode FROM a +// Bitmap ... reads pixels via other means, not by writing through this +// specific pointer". Measurement disproved that assumption: the glyph atlas +// path is exactly a read through this pointer. Live log - +// AndroidBitmap_lockPixels rc=0 hostAddr=0x... -> guest=0x0 +// bitmap 1024x1024 stride=4096 format=1 (RGBA_8888) +// - 1024x1024 being precisely the atlas size independently derived from the +// glyph UVs (u1 = 35/1024 for a 35px glyph). So BitmapGraphics rendered the +// glyphs correctly, the guest asked for the pixels, got nothing, and +// uploaded whatever was at guest address 0 as the font texture. That is the +// coloured noise on screen. +// +// Fixed with a bounce buffer: the real pixels are copied into a guest-visible +// buffer on lock, and copied back on unlock (so a guest that WRITES pixels - +// the case the original comment worried about - works too). Costs one +// height*stride copy each way, but this fires a handful of times per session, +// not per frame. +namespace { +struct BouncedBitmap { + GuestAddr guestBuf = 0; + uint32_t bytes = 0; + void* hostPixels = nullptr; +}; +std::mutex g_bouncedBitmapsMutex; +std::unordered_map g_bouncedBitmaps; // keyed by the guest bitmap handle +} // namespace + uint32_t Shim_AndroidBitmap_lockPixels(GuestEngine& eng, uint32_t, uint32_t r1, uint32_t r2, uint32_t, uint32_t) { jobject bitmap = (jobject)JniShim::Instance().handles().Resolve(r1); void* addr = nullptr; int rc = AndroidBitmap_lockPixels(JniShim::Instance().RealEnv(), bitmap, &addr); + + // Preferred path: give the guest a real, readable copy. + if (rc == 0 && addr && r2 && !eng.IsHostPointerInRegion(addr)) { + AndroidBitmapInfo info{}; + if (AndroidBitmap_getInfo(JniShim::Instance().RealEnv(), bitmap, &info) == 0 && + info.stride && info.height) { + uint32_t bytes = info.stride * info.height; + GuestAddr buf = eng.heap().Alloc(bytes); + if (buf) { + memcpy(eng.G2H(buf), addr, bytes); + memcpy(eng.G2H(r2), &buf, 4); + { + std::lock_guard lock(g_bouncedBitmapsMutex); + g_bouncedBitmaps[r1] = BouncedBitmap{buf, bytes, addr}; + } + return (uint32_t)rc; + } + // Allocation failed - fall through to the old NULL behaviour + // rather than handing out a host pointer the guest would + // dereference into its own region. + Log("gles_shim: AndroidBitmap_lockPixels: could not allocate a %u-byte guest bounce " + "buffer for a %ux%u bitmap - guest will see NULL pixels", bytes, info.width, + info.height); + } + } + if (r2) { - // Real pixel buffer lives in host (driver-owned) memory, not our - // guest region - H2G on it would silently produce a garbage/ - // wraparound "guest address" (see IsHostPointerInRegion's own - // comment), so explicitly return NULL instead of a dangerous - // almost-valid-looking pointer. Guest code that wants to directly - // WRITE pixel data through this pointer can't do so under this - // design (documented gap, low priority - texture decode FROM a - // Bitmap is the main real use, which reads pixels via other means, - // not by writing through this specific pointer). + // Remaining cases: the lock itself failed, or the buffer already lives + // inside the guest region (then H2G is exact and no bounce is needed). + // A host pointer outside the region is never handed over as-is - H2G + // on it would silently produce a garbage/wraparound "guest address" + // (see IsHostPointerInRegion's own comment), so NULL is the honest + // answer. uint32_t g = (addr && eng.IsHostPointerInRegion(addr)) ? eng.H2G(addr) : 0; memcpy(eng.G2H(r2), &g, 4); + if (rc == 0 && !g) { + Log("gles_shim: AndroidBitmap_lockPixels succeeded (hostAddr=%p) but the guest gets " + "NULL - no bounce buffer was made, pixels read through this pointer will be " + "garbage", addr); + } } return (uint32_t)rc; } -uint32_t Shim_AndroidBitmap_unlockPixels(GuestEngine&, uint32_t, uint32_t r1, uint32_t, uint32_t, uint32_t) { + +uint32_t Shim_AndroidBitmap_unlockPixels(GuestEngine& eng, uint32_t, uint32_t r1, uint32_t, uint32_t, uint32_t) { + // Flush a bounce buffer back into the real bitmap before unlocking, so + // guest-side writes (the direction the old NULL-returning code could not + // support at all) land where Java will see them. + BouncedBitmap bounced{}; + { + std::lock_guard lock(g_bouncedBitmapsMutex); + auto it = g_bouncedBitmaps.find(r1); + if (it != g_bouncedBitmaps.end()) { + bounced = it->second; + g_bouncedBitmaps.erase(it); + } + } + if (bounced.guestBuf) { + memcpy(bounced.hostPixels, eng.G2H(bounced.guestBuf), bounced.bytes); + eng.heap().Free(bounced.guestBuf); + } jobject bitmap = (jobject)JniShim::Instance().handles().Resolve(r1); return (uint32_t)AndroidBitmap_unlockPixels(JniShim::Instance().RealEnv(), bitmap); }