-
Notifications
You must be signed in to change notification settings - Fork 0
Enable XRGB8888 color conversion #31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enables XRGB8888 color format support in the MinUI libretro frontend, allowing cores that output 32-bit color to be converted to the native RGB565 format required by the display hardware. The implementation includes both a scalar fallback and an optimized ARM NEON SIMD version for better performance.
Key Changes:
- Added automatic detection and handling of XRGB8888 pixel format from libretro cores
- Implemented optimized NEON-based color conversion achieving 3-4x speedup over scalar approach
- Enhanced buffer allocation with proper error handling and validation
- Added comprehensive documentation of display specifications across all supported platforms
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| workspace/all/minarch/minarch.c | Implements XRGB8888→RGB565 conversion with NEON optimization, updates video callback pipeline to handle multiple pixel formats, adds buffer validation and error handling |
| docs/DISPLAY-SPECIFICATIONS.md | Comprehensive new documentation detailing display specifications, padding, and scaling configurations for all supported platforms |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Copy or convert data to backbuffer | ||
| if (downsample) { | ||
| // Core provided XRGB8888, convert to tightly-packed RGB565 | ||
| buffer_downsample(data, width, height, pitch); |
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing buffer validation: After calling buffer_downsample at line 3548, the code assumes buffer is valid and copies from it at line 3549. However, if the downsample buffer allocation fails, buffer will be NULL and this will cause a crash. This issue exists because the threaded path doesn't call selectScaler before downsampling, so buffer_realloc is never called. Need to either call buffer_realloc before downsampling or add a NULL check after buffer_downsample.
| buffer_downsample(data, width, height, pitch); | |
| buffer_downsample(data, width, height, pitch); | |
| if (!buffer) { | |
| LOG_error("Failed to allocate downsample buffer: %ux%u", width, height); | |
| pthread_mutex_unlock(&core_mx); | |
| return; | |
| } |
workspace/all/minarch/minarch.c
Outdated
| // For a 160-pixel-wide image with 568-pixel pitch: | ||
| // extra = 568 - 160 = 408 pixels of padding per line |
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect example in comment: The comment states "For a 160-pixel-wide image with 568-pixel pitch: extra = 568 - 160 = 408 pixels of padding per line". However, the calculation is incorrect. The pitch is in bytes, not pixels. For XRGB8888 (4 bytes/pixel), the calculation should be: extra = (568 / 4) - 160 = 142 - 160 = -18 which would be negative. The example seems to confuse pitch in bytes vs pixels. A correct example would be: "For a 160-pixel-wide image with 640-byte pitch (160 pixels × 4 bytes): extra = (640 / 4) - 160 = 0 pixels of padding".
| // For a 160-pixel-wide image with 568-pixel pitch: | |
| // extra = 568 - 160 = 408 pixels of padding per line | |
| // For XRGB8888 (4 bytes/pixel), extra padding per line is: | |
| // extra = (pitch / 4) - width | |
| // Example: For a 160-pixel-wide image with 640-byte pitch (160 × 4): | |
| // extra = (640 / 4) - 160 = 0 pixels of padding per line |
workspace/all/minarch/minarch.c
Outdated
| buffer_downsample_neon(data, width, height, pitch); | ||
| #else | ||
| // Scalar fallback: Convert XRGB8888 to RGB565 pixel-by-pixel | ||
| for (int y = 0; y < height; y++) { |
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Loop variable type mismatch: y is declared as int but compared against height which is unsigned. For consistency with the function signature and to avoid potential signed/unsigned comparison warnings, consider using unsigned for loop variables.
| for (int y = 0; y < height; y++) { | |
| for (unsigned y = 0; y < height; y++) { |
workspace/all/minarch/minarch.c
Outdated
| #else | ||
| // Scalar fallback: Convert XRGB8888 to RGB565 pixel-by-pixel | ||
| for (int y = 0; y < height; y++) { | ||
| for (int x = 0; x < width; x++) { |
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Loop variable type mismatch: x is declared as int but compared against width which is unsigned. For consistency with the function signature and to avoid potential signed/unsigned comparison warnings, consider using unsigned for loop variables.
| for (int x = 0; x < width; x++) { | |
| for (unsigned int x = 0; x < width; x++) { |
| * @note Writes converted data to 'buffer' global | ||
| * @note Uses NEON optimization when HAS_NEON is defined (3-4x speedup) | ||
| */ | ||
| static void buffer_downsample(const void* data, unsigned width, unsigned height, size_t pitch) { |
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing buffer validation: buffer_downsample uses the global buffer pointer without checking if it's NULL. If buffer_realloc fails and sets buffer = NULL (lines 2971-2975), subsequent calls to buffer_downsample will dereference a NULL pointer and crash. Add a NULL check at the start of buffer_downsample: if (!buffer) return;
| static void buffer_downsample(const void* data, unsigned width, unsigned height, size_t pitch) { | |
| static void buffer_downsample(const void* data, unsigned width, unsigned height, size_t pitch) { | |
| if (!buffer) | |
| return; |
No description provided.