Skip to content

Commit cc4ed1d

Browse files
takaokoujiclaude
andcommitted
fix: address review feedback for Renderer#read_pixels
- Accept SDL2::PixelFormat in addition to Integer by using uint32_for_format() instead of NUM2UINT() - Remove incorrect ARGB8888 default override; pass format=0 through to SDL_RenderReadPixels as-is (SDL2 uses the rendering target's format when 0 is specified) - Query renderer info to determine correct pitch when format is 0 - Improve RDoc with @overload, usage examples, and caveats Addresses feedback from ohai#29. Fixes #9 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 6f153fc commit cc4ed1d

File tree

1 file changed

+31
-11
lines changed

1 file changed

+31
-11
lines changed

video.c.m4

Lines changed: 31 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1341,22 +1341,34 @@ static VALUE Renderer_present(VALUE self)
13411341
}
13421342

13431343
/*
1344-
* Read pixels from the current rendering target.
1344+
* @overload read_pixels(rect, format)
1345+
* Read pixels from the current rendering target.
13451346
*
1346-
* @param rect [SDL2::Rect,nil] the area to read (the entire target if nil)
1347-
* @param format [Integer] the desired pixel format (0 for ARGB8888)
1348-
* @return [String] raw pixel data as a binary string (ARGB8888 by default)
1347+
* This is a very slow operation and should not be used frequently.
1348+
* Call this after rendering but before {#present}.
13491349
*
1350-
* @example
1351-
* # Read the entire screen before presenting
1352-
* pixels = renderer.read_pixels(nil, 0)
1350+
* @param rect [SDL2::Rect, nil] the area to read (nil for the entire target)
1351+
* @param format [SDL2::PixelFormat, Integer] the desired pixel format
1352+
* (0 to use the format of the rendering target)
1353+
* @return [String] raw pixel data as a binary string
1354+
*
1355+
* @example Read the entire screen in the rendering target's native format
1356+
* pixels = renderer.read_pixels(nil, 0)
1357+
*
1358+
* @example Read a sub-region in ARGB8888
1359+
* rect = SDL2::Rect.new(10, 10, 100, 100)
1360+
* pixels = renderer.read_pixels(rect, SDL2::PixelFormat::ARGB8888)
1361+
*
1362+
* @raise [SDL2::Error] raised on failure
1363+
* @see https://wiki.libsdl.org/SDL2/SDL_RenderReadPixels SDL_RenderReadPixels
13531364
*/
13541365
static VALUE Renderer_read_pixels(VALUE self, VALUE rect, VALUE format)
13551366
{
13561367
SDL_Renderer* renderer = Get_SDL_Renderer(self);
13571368
SDL_Rect sdl_rect;
13581369
SDL_Rect* rect_ptr = NULL;
1359-
Uint32 fmt = NUM2UINT(format);
1370+
Uint32 fmt = uint32_for_format(format);
1371+
Uint32 pitch_fmt;
13601372
int w, h, pitch;
13611373
void* pixels;
13621374
VALUE result;
@@ -1370,10 +1382,18 @@ static VALUE Renderer_read_pixels(VALUE self, VALUE rect, VALUE format)
13701382
HANDLE_ERROR(SDL_GetRendererOutputSize(renderer, &w, &h));
13711383
}
13721384

1373-
if (fmt == 0)
1374-
fmt = SDL_PIXELFORMAT_ARGB8888;
1385+
/* When fmt is 0 SDL uses the rendering target's format, so we need
1386+
to query it to calculate the correct pitch and buffer size. */
1387+
if (fmt == 0) {
1388+
SDL_RendererInfo info;
1389+
HANDLE_ERROR(SDL_GetRendererInfo(renderer, &info));
1390+
pitch_fmt = (info.num_texture_formats > 0)
1391+
? info.texture_formats[0] : SDL_PIXELFORMAT_ARGB8888;
1392+
} else {
1393+
pitch_fmt = fmt;
1394+
}
13751395

1376-
pitch = w * SDL_BYTESPERPIXEL(fmt);
1396+
pitch = w * SDL_BYTESPERPIXEL(pitch_fmt);
13771397
pixels = ruby_xmalloc(pitch * h);
13781398

13791399
if (SDL_RenderReadPixels(renderer, rect_ptr, fmt, pixels, pitch) < 0) {

0 commit comments

Comments
 (0)