Skip to content

Commit d197d89

Browse files
committed
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.
1 parent 39c4f24 commit d197d89

4 files changed

Lines changed: 79 additions & 27 deletions

File tree

gfx/drivers/gl1.c

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1976,10 +1976,28 @@ static bool gl1_read_viewport(void *data, uint8_t *buffer, bool is_idle)
19761976
if (!is_idle)
19771977
video_driver_cached_frame();
19781978

1979-
video_frame_convert_rgba_to_bgr(
1980-
(const void*)gl1->readback_buffer_screenshot,
1981-
buffer,
1982-
num_pixels);
1979+
{
1980+
/* Clamp to the region glReadPixels actually wrote.
1981+
* gl1_readback() clamps its read to
1982+
* min(vp.{w,h}, video_{width,height}), where video_{width,height}
1983+
* come from video_info and ultimately video_driver_get_size().
1984+
* gl1->video_{width,height} holds the core's frame size, not the
1985+
* window size, so we re-query here to match. Not a hot path. */
1986+
unsigned vd_w = 0;
1987+
unsigned vd_h = 0;
1988+
unsigned rb_w = 0;
1989+
unsigned rb_h = 0;
1990+
video_driver_get_size(&vd_w, &vd_h);
1991+
rb_w = (gl1->vp.width > vd_w) ? vd_w : gl1->vp.width;
1992+
rb_h = (gl1->vp.height > vd_h) ? vd_h : gl1->vp.height;
1993+
video_frame_convert_rgba_to_bgr(
1994+
(const void*)gl1->readback_buffer_screenshot,
1995+
buffer,
1996+
rb_w * sizeof(uint32_t),
1997+
rb_w * 3,
1998+
rb_w,
1999+
rb_h);
2000+
}
19832001

19842002
free(gl1->readback_buffer_screenshot);
19852003
gl1->readback_buffer_screenshot = NULL;

gfx/drivers/gl2.c

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2331,14 +2331,19 @@ static bool gl2_renderchain_read_viewport(
23312331

23322332
if (ptr)
23332333
{
2334-
int y;
2335-
for (y = 0; y < gl->vp.height; y++)
2336-
{
2337-
video_frame_convert_rgba_to_bgr(
2338-
(const void*)ptr,
2339-
buffer,
2340-
gl->vp.width);
2341-
}
2334+
/* Clamp to the region glReadPixels actually wrote
2335+
* (see gl2_renderchain_readback). */
2336+
unsigned rb_w = (gl->vp.width > gl->video_width)
2337+
? gl->video_width : gl->vp.width;
2338+
unsigned rb_h = (gl->vp.height > gl->video_height)
2339+
? gl->video_height : gl->vp.height;
2340+
video_frame_convert_rgba_to_bgr(
2341+
(const void*)ptr,
2342+
buffer,
2343+
rb_w * sizeof(uint32_t),
2344+
rb_w * 3,
2345+
rb_w,
2346+
rb_h);
23422347
}
23432348
#else
23442349
ptr = (const uint8_t*)glMapBuffer(GL_PIXEL_PACK_BUFFER, GL_READ_ONLY);
@@ -2381,10 +2386,21 @@ static bool gl2_renderchain_read_viewport(
23812386
if (!is_idle)
23822387
video_driver_cached_frame();
23832388

2384-
video_frame_convert_rgba_to_bgr(
2385-
(const void*)gl->readback_buffer_screenshot,
2386-
buffer,
2387-
num_pixels);
2389+
{
2390+
/* Clamp to the region glReadPixels actually wrote
2391+
* (see gl2_renderchain_readback). */
2392+
unsigned rb_w = (gl->vp.width > gl->video_width)
2393+
? gl->video_width : gl->vp.width;
2394+
unsigned rb_h = (gl->vp.height > gl->video_height)
2395+
? gl->video_height : gl->vp.height;
2396+
video_frame_convert_rgba_to_bgr(
2397+
(const void*)gl->readback_buffer_screenshot,
2398+
buffer,
2399+
rb_w * sizeof(uint32_t),
2400+
rb_w * 3,
2401+
rb_w,
2402+
rb_h);
2403+
}
23882404

23892405
free(gl->readback_buffer_screenshot);
23902406
gl->readback_buffer_screenshot = NULL;

gfx/drivers/gl3.c

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3651,10 +3651,20 @@ static bool gl3_read_viewport(void *data, uint8_t *buffer, bool is_idle)
36513651
if (!is_idle)
36523652
video_driver_cached_frame();
36533653

3654-
video_frame_convert_rgba_to_bgr(
3655-
(const void*)gl->readback_buffer_screenshot,
3656-
buffer,
3657-
num_pixels);
3654+
{
3655+
/* Clamp to the region glReadPixels actually wrote. */
3656+
unsigned rb_w = (gl->vp.width > gl->video_width)
3657+
? gl->video_width : gl->vp.width;
3658+
unsigned rb_h = (gl->vp.height > gl->video_height)
3659+
? gl->video_height : gl->vp.height;
3660+
video_frame_convert_rgba_to_bgr(
3661+
(const void*)gl->readback_buffer_screenshot,
3662+
buffer,
3663+
rb_w * sizeof(uint32_t),
3664+
rb_w * 3,
3665+
rb_w,
3666+
rb_h);
3667+
}
36583668

36593669
free(gl->readback_buffer_screenshot);
36603670
gl->readback_buffer_screenshot = NULL;

libretro-common/include/gfx/video_frame.h

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -185,17 +185,25 @@ static INLINE void video_frame_convert_to_bgr24(
185185
static INLINE void video_frame_convert_rgba_to_bgr(
186186
const void *src_data,
187187
void *dst_data,
188-
unsigned width)
188+
unsigned src_pitch,
189+
unsigned dst_pitch,
190+
unsigned width,
191+
unsigned height)
189192
{
190-
unsigned x;
191-
uint8_t *dst = (uint8_t*)dst_data;
193+
unsigned x, y;
194+
uint8_t *dst = (uint8_t*)dst_data;
192195
const uint8_t *src = (const uint8_t*)src_data;
193196

194-
for (x = 0; x < width; x++, dst += 3, src += 4)
197+
for (y = 0; y < height; y++, dst += dst_pitch, src += src_pitch)
195198
{
196-
dst[0] = src[2];
197-
dst[1] = src[1];
198-
dst[2] = src[0];
199+
uint8_t *d = dst;
200+
const uint8_t *s = src;
201+
for (x = 0; x < width; x++, d += 3, s += 4)
202+
{
203+
d[0] = s[2];
204+
d[1] = s[1];
205+
d[2] = s[0];
206+
}
199207
}
200208
}
201209

0 commit comments

Comments
 (0)