Conversation
…ers to allow decode failures to be discarded without visual artifacts
|
✅Static analysis result - no issues found! ✅ |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for the Waveshare S3 Touch hardware platform and implements a framebuffer-based video rendering system to improve robustness against JPEG decode failures. The implementation separates JPEG decoding from display operations to prevent visual artifacts when decode errors occur.
Key changes:
- Added Waveshare S3 Touch hardware configuration and dependencies
- Implemented dual framebuffer system with separate decode and display buffers
- Refactored video rendering into a dedicated task with queue-based frame processing
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| main/main.cpp | Core implementation with framebuffer allocation, video task, and updated JPEG decode/display pipeline |
| main/idf_component.yml | Added ws-s3-touch component dependency |
| main/Kconfig.projbuild | Added Waveshare S3 Touch hardware configuration option |
| CMakeLists.txt | Updated component list to include ws-s3-touch |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return true; // we're done with our work, no need to run again, so stop the task | ||
| } | ||
|
|
||
| static size_t frame_buffer_index = 0; |
There was a problem hiding this comment.
The static variable frame_buffer_index is accessed from multiple tasks (display task and video task) without synchronization. This creates a race condition that could lead to corruption or incorrect buffer selection. Consider using atomic operations or protecting access with a mutex.
| static size_t frame_buffer_index = 0; | |
| static size_t frame_buffer_index = 0; | |
| static std::mutex frame_buffer_index_mutex; |
| xQueueSend(video_queue_, &buffer, portMAX_DELAY); | ||
| } | ||
|
|
||
| void IRAM_ATTR push_frame(const void *frame) { xQueueSend(video_queue_, &frame, portMAX_DELAY); } |
There was a problem hiding this comment.
The function passes the address of the local parameter frame to the queue instead of the frame pointer value itself. This should be xQueueSend(video_queue_, &frame, portMAX_DELAY) where frame is already a pointer, or the queue should be defined to hold void* directly rather than uint16_t*.
| uint32_t *src = (uint32_t *)&_frame[(y + i) * lcd_width + j]; | ||
| uint32_t *dst = (uint32_t *)&_buf[i * lcd_width + j]; | ||
| dst[0] = src[0]; // copy two pixels (32 bits) at a time | ||
| } |
There was a problem hiding this comment.
The loop assumes lcd_width is always even, but if it's odd, this will access memory beyond the allocated buffer bounds on the last iteration. Add a check to handle odd widths or ensure the loop doesn't exceed the buffer size.
| } | |
| int j = 0; | |
| for (; j + 1 < lcd_width; j += 2) { | |
| uint32_t *src = (uint32_t *)&_frame[(y + i) * lcd_width + j]; | |
| uint32_t *dst = (uint32_t *)&_buf[i * lcd_width + j]; | |
| dst[0] = src[0]; // copy two pixels (32 bits) at a time | |
| } | |
| // If lcd_width is odd, copy the last pixel | |
| if (lcd_width % 2 != 0) { | |
| _buf[i * lcd_width + lcd_width - 1] = _frame[(y + i) * lcd_width + lcd_width - 1]; | |
| } |
| return true; | ||
| } | ||
|
|
||
| video_queue_ = xQueueCreate(1, sizeof(uint16_t *)); |
There was a problem hiding this comment.
The queue is created to hold uint16_t* but the push_frame function expects to store void*. This type mismatch could cause issues on platforms where pointer sizes differ. The queue should be created with sizeof(void*) to match the actual usage.
| video_queue_ = xQueueCreate(1, sizeof(uint16_t *)); | |
| video_queue_ = xQueueCreate(1, sizeof(void *)); |
| // special case: if _frame_ptr is null, then we simply fill the screen with 0 | ||
| if (_frame_ptr == nullptr) { | ||
| for (int y = 0; y < lcd_height; y += num_lines_to_write) { | ||
| Pixel *_buf = (Pixel *)((uint32_t)vram0 * (vram_index ^ 0x01) + (uint32_t)vram1 * vram_index); |
There was a problem hiding this comment.
The buffer selection logic (uint32_t)vram0 * (vram_index ^ 0x01) + (uint32_t)vram1 * vram_index is duplicated and complex. Extract this into a helper function or use a simpler ternary operator like vram_index ? vram1 : vram0 for better readability and maintainability.
| Pixel *_buf = (Pixel *)((uint32_t)vram0 * (vram_index ^ 0x01) + (uint32_t)vram1 * vram_index); | |
| Pixel *_buf = (Pixel *)(vram_index ? vram1 : vram0); |
1 participant
Description
Motivation and Context
How has this been tested?
Screenshots (if appropriate, e.g. schematic, board, console logs, lab pictures):
Types of changes
Checklist:
Software
.github/workflows/build.ymlfile to add my new test to the automated cloud build github action.Hardware