Attempt to address SPI SDCard speed limits#29
Attempt to address SPI SDCard speed limits#29echo-lalia wants to merge 3 commits intoatomic14:mainfrom
Conversation
WalkthroughBuild configs add a FILE_BUFFER_SIZE macro for two PlatformIO environments. AVIParser enables optional file buffering in open() and extends getNextChunk with a skipRead flag and early validations. SDCardVideoSource switches to target-frame logic and uses getNextChunk(..., true) during catch-up. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant SD as SDCardVideoSource
participant AVI as AVIParser
participant FS as FILE/SD
rect rgb(245,245,255)
note over AVI: open() with optional buffering
App->>AVI: open(path)
AVI->>FS: fopen(path, "rb")
alt FILE_BUFFER_SIZE defined
AVI->>FS: setvbuf(stream, NULL, _IOFBF, FILE_BUFFER_SIZE)
end
AVI-->>App: open result
end
rect rgb(240,255,240)
note over SD,AVI: getVideoFrame with catch-up (skipRead)
App->>SD: getVideoFrame(videoTime)
SD->>SD: compute targetFrame
loop while (targetFrame - mFrameCount > 1)
SD->>AVI: getNextChunk(&buf, len, skipRead=true)
AVI->>FS: fseek past chunk
SD->>SD: mFrameCount++
end
SD->>AVI: getNextChunk(&buf, len, skipRead=false)
alt chunk read
AVI->>FS: fread chunk into buffer
AVI-->>SD: size>0
else no movi/file
AVI-->>SD: 0
end
SD-->>App: frame or no frame
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
player/src/AVIParser/AVIParser.cpp (1)
154-158: Header read lacks EOF/error checks; mMoviListLength accounting can desync.readChunk doesn’t report success. If fread hits EOF, you still decrement mMoviListLength by 8 and proceed. Consider:
- Make readChunk return bool and check it here; or
- After readChunk, immediately check feof/ferror and return 0.
Minimal inline guard:
readChunk(mFile, &header); - mMoviListLength -= 8; + if (feof(mFile) || ferror(mFile)) { + Serial.println("EOF or read error while reading chunk header."); + return 0; + } + mMoviListLength -= 8;
🧹 Nitpick comments (5)
player/platformio.ini (2)
171-172: Define FILE_BUFFER_SIZE once (and guard memory on low-RAM boards).Good call adding a larger stdio buffer. Two suggestions:
- Avoid duplication: define FILE_BUFFER_SIZE in a shared section (e.g., [esp32_common]) and override per-env only when different.
- Sanity-guard RAM usage: 16 KB is fine on PSRAM-equipped boards, but may be tight on variants without PSRAM. Consider a lower default (e.g., 8 KB) and bump only on environments known-good.
If you prefer to keep it per-env, add a short comment with RAM rationale so future edits don’t regress.
240-241: Keep the buffer-size policy consistent across SD-enabled envs.Same note as cheap-yellow-display: centralize FILE_BUFFER_SIZE or document why this env differs. Also validate that all SD-enabled envs (e.g., atomic14-custom-pcb with SDIO) either define an explicit FILE_BUFFER_SIZE or are intentionally left to the libc default.
player/src/AVIParser/AVIParser.h (1)
23-23: API extension looks good; document skipRead semantics.Adding skipRead with a default preserves source compatibility. Please document:
- When skipRead should be used (catch-up vs normal decode),
- Whether return value is still the chunk size even when skipping,
- That buffer/bufferLength are ignored when skipRead=true.
This will help callers avoid undefined assumptions.
player/src/VideoSource/SDCardVideoSource.cpp (1)
41-44: Use unsigned/64-bit intermediate for time-to-frame math; reduce truncation risk.videoTime and targetFrame are computed with signed 32-bit ints. Prefer unsigned millisecond arithmetic (handles millis() wrap) and a 64-bit intermediate for the multiply:
- int targetFrame = videoTime * DEFAULT_FPS / 1000; + uint32_t targetFrame = (uint64_t(videoTime) * DEFAULT_FPS) / 1000;Optionally also change
videoTimetouint32_twhere it’s defined to benefit from unsigned wrap-safe subtraction.player/src/AVIParser/AVIParser.cpp (1)
121-126: RIFF padding handling when skipping non-LIST chunks in open().When scanning for the movi LIST, skipping a non-LIST chunk doesn’t account for the RIFF padding byte on odd-sized chunks. This can misalign parsing later. Recommend:
- // skip the chunk data bytes - fseek(mFile, header.chunkSize, SEEK_CUR); + // skip the chunk data bytes (+ padding for odd sizes) + fseek(mFile, header.chunkSize + (header.chunkSize & 1u), SEEK_CUR);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
player/platformio.ini(2 hunks)player/src/AVIParser/AVIParser.cpp(3 hunks)player/src/AVIParser/AVIParser.h(1 hunks)player/src/VideoSource/SDCardVideoSource.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
player/src/AVIParser/AVIParser.h (1)
player/src/AVIParser/AVIParser.cpp (2)
getNextChunk(139-204)getNextChunk(139-139)
| // we've got the required chunk | ||
| if (skipRead) | ||
| { // skip over this chunk | ||
| fseek(mFile, header.chunkSize, SEEK_CUR); | ||
| } | ||
| else |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Seek error handling when skipping chunks.
When skipRead is true, fseek could fail on a short file or I/O error. Check the return value and bail gracefully:
- if (skipRead)
- { // skip over this chunk
- fseek(mFile, header.chunkSize, SEEK_CUR);
- }
+ if (skipRead) {
+ if (fseek(mFile, header.chunkSize, SEEK_CUR) != 0) {
+ Serial.println("Failed to seek over chunk.");
+ return 0;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // we've got the required chunk | |
| if (skipRead) | |
| { // skip over this chunk | |
| fseek(mFile, header.chunkSize, SEEK_CUR); | |
| } | |
| else | |
| // we've got the required chunk | |
| if (skipRead) { | |
| if (fseek(mFile, header.chunkSize, SEEK_CUR) != 0) { | |
| Serial.println("Failed to seek over chunk."); | |
| return 0; | |
| } | |
| } | |
| else |
🤖 Prompt for AI Agents
In player/src/AVIParser/AVIParser.cpp around lines 163 to 168, the code calls
fseek(mFile, header.chunkSize, SEEK_CUR) when skipRead is true but does not
check the return value; detect if fseek returns non-zero (failure), handle it by
clearing/recording the error, optionally logging a descriptive message, clean
up/close the file or reset parser state as needed, and return a failure result
(or throw) so the caller can bail gracefully instead of continuing on a failed
seek.
| if (header.chunkSize > bufferLength) | ||
| { | ||
| *buffer = (uint8_t *)realloc(*buffer, header.chunkSize); | ||
| bufferLength = header.chunkSize; | ||
| } | ||
| // copy the chunk data | ||
| fread(*buffer, header.chunkSize, 1, mFile); | ||
| } |
There was a problem hiding this comment.
Unsafe realloc and unchecked fread can crash on OOM or I/O error.
- realloc result is assigned directly to *buffer — if it returns NULL, you lose the original pointer and will dereference NULL on fread.
- fread result is ignored; partial reads will go unnoticed.
Use a temporary pointer and verify bytes read:
- if (header.chunkSize > bufferLength)
- {
- *buffer = (uint8_t *)realloc(*buffer, header.chunkSize);
- bufferLength = header.chunkSize;
- }
- // copy the chunk data
- fread(*buffer, header.chunkSize, 1, mFile);
+ if (header.chunkSize > bufferLength) {
+ uint8_t *newBuf = (uint8_t *)realloc(*buffer, header.chunkSize);
+ if (!newBuf) {
+ Serial.println("Out of memory reallocating frame buffer.");
+ return 0;
+ }
+ *buffer = newBuf;
+ bufferLength = header.chunkSize;
+ }
+ // copy the chunk data
+ size_t n = fread(*buffer, 1, header.chunkSize, mFile);
+ if (n != header.chunkSize) {
+ Serial.println("Short read while reading chunk.");
+ return 0;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (header.chunkSize > bufferLength) | |
| { | |
| *buffer = (uint8_t *)realloc(*buffer, header.chunkSize); | |
| bufferLength = header.chunkSize; | |
| } | |
| // copy the chunk data | |
| fread(*buffer, header.chunkSize, 1, mFile); | |
| } | |
| if (header.chunkSize > bufferLength) { | |
| uint8_t *newBuf = (uint8_t *)realloc(*buffer, header.chunkSize); | |
| if (!newBuf) { | |
| Serial.println("Out of memory reallocating frame buffer."); | |
| return 0; | |
| } | |
| *buffer = newBuf; | |
| bufferLength = header.chunkSize; | |
| } | |
| // copy the chunk data | |
| size_t n = fread(*buffer, 1, header.chunkSize, mFile); | |
| if (n != header.chunkSize) { | |
| Serial.println("Short read while reading chunk."); | |
| return 0; | |
| } |
|
Quick update: I'm fairly certain that having both the video player and audio player tasks separately access the file is the real source of the slowdown that happens. This is probably because of the fact that AVI files are synchronous, so the video data chunks are weaved between the audio data chunks, and therefore the video task wastes quite a bit of resources reading audio data, and the audio tasks wastes a lot of resources reading video data. I'm working on a fork of this repo's video player, where I've rewritten it to use a single file access object in the audio task, and when it find video data, it just sends it into a buffer for the video task to decode when it's ready. Edit: here's a little demo of the video running at 20 fps. Again, this pull request doesn't quite get to that point (only up to around 10 fps consistently for me, still). But you can see what I'm talking about. https://github.com/user-attachments/assets/51247b77-4828-47df-b002-89086950066d |
1 participant
Addresses #26
This is an attempt to address issues with playback from the SD Card. After some amount of time (about a minute or two in my case) playback drastically slows from ~15 fps down to 0, and then starts to ruin the audio too. If I wait for the video to restart, playback resumes at 15fps.
I did some basic profiling and found that the Video task was spending almost all of its time in the
AVIParser::getNextChunkmethod, primarily on thefread(*buffer, header.chunkSize, 1, mFile);function call, but sometimes even spending more than 50ms on thereadChunk(mFile, &header);call. I think the root cause of the issue is that there are two tasks fighting over the same file resources using two separate File objects, which causes them to get stuck waiting for each other sometimes.After a bit of experimenting it seems like once the video task falls a little bit behind, it has to play catch up by quickly reading multiple frames from the SD Card before it can finally display a new one, and this is why the issue only gets worse and worse as the video goes on.
In an attempt to improve the rate of 'catch up', I added an optional
skipReadparameter to thegetNextChunkmethod, which literally just skips thefreadstep when it isn't required. This seemed to improve things slightly in my test, but it only delayed the issue from occurring.(I also noticed that
bufferLengthwas always 0 from the video thread. I added a line to update bufferLength to chunkSize when the buffer is reallocated, because I assumed that was the intended behaviour. However, that doesn't seem to have affected speed at all.)Finally, I found this issue posted on the esp-idf repo, where it is recommended to try increasing the internal buffer size for a File object if it isn't reading fast enough. I tried some different values, and a 16kb buffer (which happens to correspond to about 2 video frames in my test file) seems to be the sweet spot for my board.
With these changes, video playback speed still drops throughout the playback, but it happens much slower (dropping to 1~3fps after ~10 minutes, rather than ~0fps after ~2 minutes). And, if I re-encode the video (and change DEFAULT_FPS) to 12fps, I can stay at roughly 10-12 fps more consistently.
Summary by CodeRabbit
Bug Fixes
Performance