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:
U-DESKTOP-SPFP6AQ\twistedtechre committed 2026-04-21 21:05:42 +02:00
1 parent 39c4f24f75
commit d197d89e6c
4 files changed
+79 -27

No files matched your search

+22 -4
View File
@@ -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
View File
@@ -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
View File
@@ -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;
+15 -7
View File
@@ -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];
}
}
}