Add NTSC handling and rotation

[nchapman]

• Fix NTSC handling although in the end I'm not sure it was necessary but at least it's working!
• Add rotation support in libretro primarily for arcade games

[Copilot]

Pull request overview

This PR adds rotation support and improved timing/geometry handling for arcade games in the MinArch libretro frontend. The implementation includes software-based rotation (90°, 180°, 270°) for cores that request display rotation via the libretro API, along with enhanced handling of dynamic geometry changes and frame timing callbacks.

Key Changes:

• Software rotation implementation with RGB565 pixel-by-pixel transformation for all rotation angles
• Dynamic geometry and AV info handling via SET_GEOMETRY, SET_SYSTEM_AV_INFO, and SET_ROTATION environment callbacks
• Frame timing callback support and throttle state reporting for better core synchronization

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
workspace/all/minarch/minarch.c	Core changes: added rotation state management, rotation buffer allocation, software rotation application in video pipeline, environment callback handlers for rotation/geometry/timing, and frame time callback invocations
workspace/all/common/scaler.h	Added rotation function declaration, rotation angle constants (ROTATION_0 through ROTATION_270), and improved memcpy_neon signature with const correctness
workspace/all/common/scaler.c	Implemented rotate_c16() function with pixel-by-pixel rotation logic for all four rotation angles (0°, 90°, 180°, 270°)
skeleton/TEMPLATES/minarch-paks/configs/base/A2600/default.cfg	Removed Stella-specific core options (filter and crop settings) as part of NTSC handling fixes

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

[nitpick] The rotation implementation lacks a default case in the switch statement. While rotation values are validated in the environment callback, adding a default case would make the code more defensive and easier to debug if an invalid value somehow gets through.

Suggestion: Add a default case that logs an error or at least does nothing explicitly:

default:
    // Invalid rotation value, skip rotation
    break;

Suggested change

	break;
	break;
	default:
	// Invalid rotation value, skip rotation and log error
	fprintf(stderr, "rotate_c16: Invalid rotation value %u\n", rotation);
	break;

[Copilot]

When rotation is applied (including 180°), the rotation buffer uses a tightly-packed pitch (dst_w * sizeof(uint16_t)), which may differ from the original pitch if it had padding. The renderer.src_p should be updated for all rotation cases where rotated_data != frame_data, not just for 90°/270° rotations.

Suggestion: Change the condition to:

// Update pitch in renderer if rotation was applied
if (rotated_data != frame_data) {
    renderer.src_p = rotation_buffer.pitch;
}

Suggested change

	// Update pitch in renderer if dimensions were swapped by rotation
	if (rotated_data != frame_data &&
	(video_state.rotation == ROTATION_90 || video_state.rotation == ROTATION_270)) {
	// Update pitch in renderer if rotation was applied
	if (rotated_data != frame_data) {

[Copilot]

[nitpick] The rotation implementation uses a naive pixel-by-pixel approach which could be a performance bottleneck for larger resolutions. For 90° and 270° rotations, the memory access pattern is cache-unfriendly (writing to non-sequential memory locations).

Consider optimizing in the future by:

1. Processing in tiles/blocks to improve cache locality
2. Using SIMD instructions (NEON on ARM) for bulk pixel operations
3. Transposing in chunks rather than individual pixels

This is acceptable for arcade games at typical resolutions (e.g., 224x320), but may struggle with higher resolution cores.

[Copilot]

[nitpick] The expression fast_forward ? true : false is redundant. Since fast_forward is already a boolean-compatible value, it can be directly assigned.

Suggestion: Simplify to:

*out = fast_forward;

Suggested change

	*out = fast_forward ? true : false;
	*out = fast_forward;

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Duplicate pitch validation: This validation is also performed in video_refresh_callback_main (lines 3999-4004) before calling buffer_downsample. Since comprehensive validation is already done here, the duplicate check above could be removed.

Suggested change

	// Validate pitch before attempting conversion
	size_t min_pitch = width * sizeof(uint32_t);
	if (pitch < min_pitch) {
	LOG_error("Skipping threaded frame due to invalid pitch: %zu < %zu", pitch,
	min_pitch);
	pthread_mutex_unlock(&core_mx);
	return; // Abort frame to prevent buffer corruption
	}

[Copilot]

Critical bug: Pitch not updated after dimension swap. When rotation is 90° or 270°, the dimensions are swapped (lines 3685-3691), but the pitch src_p is not updated to match the swapped dimensions. For example, if the original frame is 240×320 with pitch=480 bytes, after swapping it becomes 320×240, but the pitch remains 480 bytes when it should be 640 bytes (320 * 2).

This will cause the scaler to use an incorrect pitch value, potentially leading to rendering corruption.

Fix: After swapping dimensions, recalculate the pitch:

if (video_state.rotation == ROTATION_90 || video_state.rotation == ROTATION_270) {
    int temp = src_w;
    src_w = src_h;
    src_h = temp;
    src_p = src_w * sizeof(uint16_t);  // Recalculate pitch for swapped dimensions
}

Suggested change

	// Keep aspect ratio as-is (it's already for rotated dimensions)
	// Keep aspect ratio as-is (it's already for rotated dimensions)
	src_p = src_w * sizeof(uint16_t); // Recalculate pitch for swapped dimensions

[Copilot]

Incorrect comment: The comment states max_ff_speed: 0=2x, 1=3x, 2=4x, 3=5x, but based on the calculation formula max_ff_speed + 1 and the implementation at line 6202 (ff_frame_time = 1000000 / (core.fps * (max_ff_speed + 1))), the actual mapping is: 0→1x, 1→2x, 2→3x, 3→4x.

Update the comment to reflect the correct mapping or adjust the calculation to match the comment.

Suggested change

	state->rate = (float)(max_ff_speed + 1); // max_ff_speed: 0=2x, 1=3x, 2=4x, 3=5x
	state->rate = (float)(max_ff_speed + 1); // max_ff_speed: 0=1x, 1=2x, 2=3x, 3=4x

[Copilot]

Buffer state inconsistency after allocation failure: If realloc fails (returns NULL), the function returns early (line 3582) without updating rotation_buffer.size. On subsequent calls with the same or smaller size requirement, the condition at line 3578 will be false, causing the function to skip reallocation and proceed to lines 3590-3592, updating width/height/pitch fields even though rotation_buffer.buffer is NULL. This creates an inconsistent state where the fields indicate a valid buffer, but the buffer pointer is NULL.

Additionally, the standard realloc(NULL, size) is equivalent to malloc(size), so this should work on the first call. However, the inconsistent state after a failed reallocation is problematic.

Fix: Either check if the buffer is NULL before updating fields, or reset rotation_buffer.size = 0 when allocation fails to ensure retry on next call:

if (!new_buffer) {
    LOG_error("Failed to allocate rotation buffer: %zu bytes", required_size);
    rotation_buffer.size = 0;  // Force retry on next call
    return;
}

[Copilot]

Low precision timestamp for frame time callback: The frame time callback is called with SDL_GetTicks() * 1000, which provides millisecond-precision timestamps scaled to microseconds. This means the microsecond-level precision is lost (values will always be multiples of 1000).

A more accurate approach would be to use getMicroseconds() (already used at line 6205 for timing), which likely provides true microsecond precision:

retro_usec_t current_time = getMicroseconds();
video_state.frame_time_cb(current_time);

This would provide more accurate timing information to cores that rely on precise frame timing.

Suggested change

	retro_usec_t current_time = SDL_GetTicks() * 1000; // Convert ms to µs
	retro_usec_t current_time = getMicroseconds(); // True microsecond precision

[Copilot]

Low precision timestamp for frame time callback: The frame time callback is called with SDL_GetTicks() * 1000, which provides millisecond-precision timestamps scaled to microseconds. This means the microsecond-level precision is lost (values will always be multiples of 1000).

A more accurate approach would be to use getMicroseconds() (already used at line 6205 for timing), which likely provides true microsecond precision:

retro_usec_t current_time = getMicroseconds();
video_state.frame_time_cb(current_time);

This would provide more accurate timing information to cores that rely on precise frame timing.

Suggested change

	retro_usec_t current_time = SDL_GetTicks() * 1000; // Convert ms to µs
	retro_usec_t current_time = getMicroseconds();

[Copilot]

Duplicate pitch validation: The pitch validation for XRGB8888 format is performed both here (lines 3999-4004) and inside the buffer_downsample function (lines 3507-3514). This is redundant since buffer_downsample already performs comprehensive validation. Consider removing this duplicate check to avoid code duplication and rely on the validation inside buffer_downsample.

Suggested change

	// Validate pitch before attempting conversion
	size_t min_pitch = width * sizeof(uint32_t);
	if (pitch < min_pitch) {
	LOG_error("Skipping frame due to invalid pitch: %zu < %zu", pitch, min_pitch);
	return; // Abort entire frame to prevent rendering corrupted data
	}