mirror of
https://github.com/libretro/RetroArch.git
synced 2026-11-07 17:31:11 -10:00
gl1/gl2/gl3: fix screenshot corruption in video_frame_convert_rgba_to_bgr
The RGBA-to-BGR24 helper used by the GL drivers' read-viewport /
screenshot path had two bugs:
1. The helper signature took only a single `width` parameter and
walked a tightly-packed source. Three of the four call sites
passed `vp.width * vp.height` (total pixel count) as that
`width`, which caused the inner loop to read past the end of the
region glReadPixels actually wrote.
2. The fourth site (gl2 GLES3 async path) iterated `vp.height`
times in an outer loop but always re-converted the same first
row of the source, dropping rows 1..N-1 entirely.
The underlying mismatch is that the readback buffer is allocated at
`vp.width * vp.height` but glReadPixels in each driver clamps its
read to `min(vp.{w,h}, video_{width,height})` (see
gl1_readback at gl1.c:1508-1509, gl2_renderchain_readback, and
gl3's sync readback). When the viewport is larger than the
current video output size -- common during resize, fullscreen
toggles, or when the frontend viewport exceeds the backing store --
the trailing bytes of the allocation are uninitialized and there
are fewer written bytes per row than `vp.width * 4`.
Fix:
- Change `video_frame_convert_rgba_to_bgr` to take explicit
`src_pitch`, `dst_pitch`, `width`, and `height`, looping both
axes. The function remains pure and `static INLINE`.
- At each of the four call sites compute
`rb_w = min(vp.w, video_w)` and `rb_h = min(vp.h, video_h)`,
then pass `rb_w * 4`, `rb_w * 3`, `rb_w`, `rb_h` to the helper.
For gl2 and gl3 the clamp uses `gl->video_width` / `video_height`,
which are cached copies of `video_driver_get_size()` set at init
and on context reset. For gl1 the same-named struct fields hold
the *core's* emulated frame size (assigned from `frame_width` at
gl1.c:1581-1582), not the window size, so the gl1 site calls
`video_driver_get_size()` directly to reconstruct the same value
glReadPixels saw. This mirrors the pattern already used in
`gl1_viewport_info` (gl1.c:1950). Screenshots are not a hot path,
so the extra mutex acquisition is negligible.
No external in-tree callers of the helper remain; verified by
grep across the source tree.
This commit is contained in:
1 parent
39c4f24f75
commit
d197d89e6c
4 files changed
+79
-27
No files matched your search
+22
-4
@@ -1976,10 +1976,28 @@ static bool gl1_read_viewport(void *data, uint8_t *buffer, bool is_idle)
|
||||
if (!is_idle)
|
||||
video_driver_cached_frame();
|
||||
|
||||
video_frame_convert_rgba_to_bgr(
|
||||
(const void*)gl1->readback_buffer_screenshot,
|
||||
buffer,
|
||||
num_pixels);
|
||||
{
|
||||
/* Clamp to the region glReadPixels actually wrote.
|
||||
* gl1_readback() clamps its read to
|
||||
* min(vp.{w,h}, video_{width,height}), where video_{width,height}
|
||||
* come from video_info and ultimately video_driver_get_size().
|
||||
* gl1->video_{width,height} holds the core's frame size, not the
|
||||
* window size, so we re-query here to match. Not a hot path. */
|
||||
unsigned vd_w = 0;
|
||||
unsigned vd_h = 0;
|
||||
unsigned rb_w = 0;
|
||||
unsigned rb_h = 0;
|
||||
video_driver_get_size(&vd_w, &vd_h);
|
||||
rb_w = (gl1->vp.width > vd_w) ? vd_w : gl1->vp.width;
|
||||
rb_h = (gl1->vp.height > vd_h) ? vd_h : gl1->vp.height;
|
||||
video_frame_convert_rgba_to_bgr(
|
||||
(const void*)gl1->readback_buffer_screenshot,
|
||||
buffer,
|
||||
rb_w * sizeof(uint32_t),
|
||||
rb_w * 3,
|
||||
rb_w,
|
||||
rb_h);
|
||||
}
|
||||
|
||||
free(gl1->readback_buffer_screenshot);
|
||||
gl1->readback_buffer_screenshot = NULL;
|
||||
|
||||
+28
-12
@@ -2331,14 +2331,19 @@ static bool gl2_renderchain_read_viewport(
|
||||
|
||||
if (ptr)
|
||||
{
|
||||
int y;
|
||||
for (y = 0; y < gl->vp.height; y++)
|
||||
{
|
||||
video_frame_convert_rgba_to_bgr(
|
||||
(const void*)ptr,
|
||||
buffer,
|
||||
gl->vp.width);
|
||||
}
|
||||
/* Clamp to the region glReadPixels actually wrote
|
||||
* (see gl2_renderchain_readback). */
|
||||
unsigned rb_w = (gl->vp.width > gl->video_width)
|
||||
? gl->video_width : gl->vp.width;
|
||||
unsigned rb_h = (gl->vp.height > gl->video_height)
|
||||
? gl->video_height : gl->vp.height;
|
||||
video_frame_convert_rgba_to_bgr(
|
||||
(const void*)ptr,
|
||||
buffer,
|
||||
rb_w * sizeof(uint32_t),
|
||||
rb_w * 3,
|
||||
rb_w,
|
||||
rb_h);
|
||||
}
|
||||
#else
|
||||
ptr = (const uint8_t*)glMapBuffer(GL_PIXEL_PACK_BUFFER, GL_READ_ONLY);
|
||||
@@ -2381,10 +2386,21 @@ static bool gl2_renderchain_read_viewport(
|
||||
if (!is_idle)
|
||||
video_driver_cached_frame();
|
||||
|
||||
video_frame_convert_rgba_to_bgr(
|
||||
(const void*)gl->readback_buffer_screenshot,
|
||||
buffer,
|
||||
num_pixels);
|
||||
{
|
||||
/* Clamp to the region glReadPixels actually wrote
|
||||
* (see gl2_renderchain_readback). */
|
||||
unsigned rb_w = (gl->vp.width > gl->video_width)
|
||||
? gl->video_width : gl->vp.width;
|
||||
unsigned rb_h = (gl->vp.height > gl->video_height)
|
||||
? gl->video_height : gl->vp.height;
|
||||
video_frame_convert_rgba_to_bgr(
|
||||
(const void*)gl->readback_buffer_screenshot,
|
||||
buffer,
|
||||
rb_w * sizeof(uint32_t),
|
||||
rb_w * 3,
|
||||
rb_w,
|
||||
rb_h);
|
||||
}
|
||||
|
||||
free(gl->readback_buffer_screenshot);
|
||||
gl->readback_buffer_screenshot = NULL;
|
||||
|
||||
+14
-4
@@ -3651,10 +3651,20 @@ static bool gl3_read_viewport(void *data, uint8_t *buffer, bool is_idle)
|
||||
if (!is_idle)
|
||||
video_driver_cached_frame();
|
||||
|
||||
video_frame_convert_rgba_to_bgr(
|
||||
(const void*)gl->readback_buffer_screenshot,
|
||||
buffer,
|
||||
num_pixels);
|
||||
{
|
||||
/* Clamp to the region glReadPixels actually wrote. */
|
||||
unsigned rb_w = (gl->vp.width > gl->video_width)
|
||||
? gl->video_width : gl->vp.width;
|
||||
unsigned rb_h = (gl->vp.height > gl->video_height)
|
||||
? gl->video_height : gl->vp.height;
|
||||
video_frame_convert_rgba_to_bgr(
|
||||
(const void*)gl->readback_buffer_screenshot,
|
||||
buffer,
|
||||
rb_w * sizeof(uint32_t),
|
||||
rb_w * 3,
|
||||
rb_w,
|
||||
rb_h);
|
||||
}
|
||||
|
||||
free(gl->readback_buffer_screenshot);
|
||||
gl->readback_buffer_screenshot = NULL;
|
||||
|
||||
@@ -185,17 +185,25 @@ static INLINE void video_frame_convert_to_bgr24(
|
||||
static INLINE void video_frame_convert_rgba_to_bgr(
|
||||
const void *src_data,
|
||||
void *dst_data,
|
||||
unsigned width)
|
||||
unsigned src_pitch,
|
||||
unsigned dst_pitch,
|
||||
unsigned width,
|
||||
unsigned height)
|
||||
{
|
||||
unsigned x;
|
||||
uint8_t *dst = (uint8_t*)dst_data;
|
||||
unsigned x, y;
|
||||
uint8_t *dst = (uint8_t*)dst_data;
|
||||
const uint8_t *src = (const uint8_t*)src_data;
|
||||
|
||||
for (x = 0; x < width; x++, dst += 3, src += 4)
|
||||
for (y = 0; y < height; y++, dst += dst_pitch, src += src_pitch)
|
||||
{
|
||||
dst[0] = src[2];
|
||||
dst[1] = src[1];
|
||||
dst[2] = src[0];
|
||||
uint8_t *d = dst;
|
||||
const uint8_t *s = src;
|
||||
for (x = 0; x < width; x++, d += 3, s += 4)
|
||||
{
|
||||
d[0] = s[2];
|
||||
d[1] = s[1];
|
||||
d[2] = s[0];
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user