fix: address review feedback for Renderer#read_pixels (ohai/ruby-sdl2#29)

[takaokouji]

Summary

SDL2::Renderer#read_pixels (added in smalruby/add-read-pixels branch) was submitted as ohai/ruby-sdl2#29 but closed with quality concerns. This issue tracks the necessary fixes before re-submitting.

Issues to fix

1. Does not accept SDL2::PixelFormat

Current implementation uses NUM2UINT(format) which only accepts Integer. Other methods like create_texture use the uint32_for_format() helper to accept both SDL2::PixelFormat and Integer.

Fix: Use uint32_for_format(format) instead of NUM2UINT(format).

2. Incorrect default pixel format

Current implementation overrides format 0 with SDL_PIXELFORMAT_ARGB8888:

if (fmt == 0)
    fmt = SDL_PIXELFORMAT_ARGB8888;

According to the SDL2 documentation, passing 0 means "use the format of the rendering target." The implementation should pass 0 through to SDL_RenderReadPixels as-is, respecting SDL2's defined behavior.

Fix: Remove the if (fmt == 0) override. Pass the format value directly to SDL_RenderReadPixels.

3. Sample code assumes ARGB8888 byte order

The sample code in the PR used BMP format with hardcoded BGRA byte order, which only works when the rendering target happens to use ARGB8888. On platforms like Wayland/Linux, the rendering target format may differ, producing corrupted output.

Fix: Provide a revised sample that either:

• Explicitly specifies SDL2::PixelFormat::ARGB8888 to guarantee known byte order, or
• Queries the renderer's output format and adapts accordingly

Updated RDoc

/*
 * Read pixels from the current rendering target.
 *
 * This is a very slow operation and should not be used frequently.
 * Call this after rendering but before {#present}.
 *
 * @overload read_pixels(rect, format)
 *   @param rect [SDL2::Rect, nil] the area to read (nil for entire target)
 *   @param format [SDL2::PixelFormat, Integer] desired pixel format
 *     (0 to use the format of the rendering target)
 *   @return [String] raw pixel data as a binary string
 *
 * @example Read entire screen in the rendering target's native format
 *   pixels = renderer.read_pixels(nil, 0)
 *
 * @example Read a sub-region in ARGB8888
 *   rect = SDL2::Rect.new(10, 10, 100, 100)
 *   pixels = renderer.read_pixels(rect, SDL2::PixelFormat::ARGB8888)
 *
 * @raise [SDL2::Error] raised on failure
 * @see https://wiki.libsdl.org/SDL2/SDL_RenderReadPixels SDL_RenderReadPixels
 */

Updated sample code

require "sdl2"

SDL2.init(SDL2::INIT_VIDEO)
window = SDL2::Window.create("read_pixels demo", 100, 100, 320, 240, 0)
renderer = window.create_renderer(-1, 0)

# Draw something
renderer.draw_color = [0, 128, 255]
renderer.fill_rect(SDL2::Rect.new(60, 40, 200, 160))
renderer.draw_color = [255, 255, 0]
renderer.fill_rect(SDL2::Rect.new(110, 80, 100, 80))

# Capture the rendered pixels in a known format
# Explicitly specifying ARGB8888 ensures consistent byte order across platforms
pixels = renderer.read_pixels(nil, SDL2::PixelFormat::ARGB8888)

# Save as BMP (ARGB8888 on little-endian stores bytes as B, G, R, A — which BMP expects)
width, height = 320, 240
row_size = width * 4
file_size = 54 + row_size * height
bmp = String.new(encoding: Encoding::BINARY)
bmp << ["BM", file_size, 0, 0, 54].pack("A2Vv2V")           # file header
bmp << [40, width, height, 1, 32, 0, 0, 0, 0, 0, 0].pack("VV2v2V6") # DIB header
# BMP stores rows bottom-to-top
(height - 1).downto(0) do |y|
  bmp << pixels.byteslice(y * row_size, row_size)
end
File.binwrite("screenshot.bmp", bmp)
puts "Saved screenshot.bmp (#{File.size("screenshot.bmp")} bytes)"

References

• Upstream PR: feat: add SDL2::Renderer#read_pixels ohai/ruby-sdl2#29
• SDL2 docs: https://wiki.libsdl.org/SDL2/SDL_RenderReadPixels