Skip to content

Attempt to address SPI SDCard speed limits #29

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.

Sign up for GitHub

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

Open
echo-lalia wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Attempt to address SPI SDCard speed limits #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
120dee8
Attempt to address SDCard speed limits
echo-lalia Aug 23, 2025
d3e6527
Null check before setvbuf
echo-lalia Aug 23, 2025
5739b15
Error return in getVideoFrame
echo-lalia Aug 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions player/platformio.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,8 @@ build_flags =
-DSD_CARD_MOSI=GPIO_NUM_23
-DSD_CARD_CLK=GPIO_NUM_18
-DSD_CARD_CS=GPIO_NUM_5
; increase file buffer size to prevent playback stalling on SD Card read
-DFILE_BUFFER_SIZE=16384

[env:touch-down]
extends = esp32_common
Expand Down Expand Up @@ -235,6 +237,8 @@ build_flags =
-DSD_CARD_MOSI=GPIO_NUM_15
-DSD_CARD_CLK=GPIO_NUM_13
-DSD_CARD_CS=GPIO_NUM_2
; increase file buffer size to prevent playback stalling on SD Card read
-DFILE_BUFFER_SIZE=16384

[env:m5core2]
extends = esp32_common
Expand Down
29 changes: 21 additions & 8 deletions player/src/AVIParser/AVIParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@ bool AVIParser::open()
Serial.printf("Failed to open file.\n");
return false;
}
#ifdef FILE_BUFFER_SIZE
// Increase size of file buffer if needed.
Serial.printf("Setting file buffer size to %d bytes.\n", FILE_BUFFER_SIZE);
setvbuf(mFile, NULL, _IOFBF, FILE_BUFFER_SIZE);
#endif
// check the file is valid
ChunkHeader header;
// Read RIFF header
Expand Down Expand Up @@ -129,7 +134,7 @@ bool AVIParser::open()
return true;
}

size_t AVIParser::getNextChunk(uint8_t **buffer, size_t &bufferLength)
size_t AVIParser::getNextChunk(uint8_t **buffer, size_t &bufferLength, bool skipRead)
{
// check if the file is open
if (!mFile)
Expand All @@ -153,14 +158,22 @@ size_t AVIParser::getNextChunk(uint8_t **buffer, size_t &bufferLength)
if (mRequiredChunkType == AVIChunkType::VIDEO && isVideoChunk ||
mRequiredChunkType == AVIChunkType::AUDIO && isAudioChunk)
{
// we've got the required chunk - copy it into the provided buffer
// reallocate the buffer if necessary
if (header.chunkSize > bufferLength)
{
*buffer = (uint8_t *)realloc(*buffer, header.chunkSize);
// we've got the required chunk
if (skipRead)
{ // skip over this chunk
fseek(mFile, header.chunkSize, SEEK_CUR);
}
else
Comment on lines +161 to +166
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Aug 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
// 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.

{ // copy this chunk into the provided buffer
// reallocate the buffer if necessary
if (header.chunkSize > bufferLength)
{
*buffer = (uint8_t *)realloc(*buffer, header.chunkSize);
bufferLength = header.chunkSize;
}
// copy the chunk data
fread(*buffer, header.chunkSize, 1, mFile);
}
Comment on lines +169 to 176
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Aug 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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;
}

// copy the chunk data
fread(*buffer, header.chunkSize, 1, mFile);
mMoviListLength -= header.chunkSize;
// handle any padding bytes
if (header.chunkSize % 2 != 0)
Expand Down
2 changes: 1 addition & 1 deletion player/src/AVIParser/AVIParser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,5 @@ class AVIParser
AVIParser(std::string fname, AVIChunkType requiredChunkType);
~AVIParser();
bool open();
size_t getNextChunk(uint8_t **buffer, size_t &bufferLength);
size_t getNextChunk(uint8_t **buffer, size_t &bufferLength, bool skipRead=false);
};
22 changes: 16 additions & 6 deletions player/src/VideoSource/SDCardVideoSource.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,25 @@ bool SDCardVideoSource::getVideoFrame(uint8_t **buffer, size_t &bufferLength, si
// work out the video time from a combination of the currentAudioSample and the elapsed time
int elapsedTime = millis() - mLastAudioTimeUpdateMs;
int videoTime = mAudioTimeMs + elapsedTime;
int frameTime = 1000 * mFrameCount / DEFAULT_FPS;
if (videoTime <= frameTime)
{
int targetFrame = videoTime * DEFAULT_FPS / 1000;
if (mFrameCount >= targetFrame){
return false;
}
while (videoTime > 1000 * mFrameCount / DEFAULT_FPS)
{
// Skip some number of frames if we have fallen behind
while (targetFrame - mFrameCount > 1){
size_t skipped = parser->getNextChunk(buffer, bufferLength, true);
if (skipped == 0) {
// No more data or error — bail without advancing the frame counter.
return false;
}
mFrameCount++;
frameLength = parser->getNextChunk((uint8_t **)buffer, bufferLength);
}
// We are caught up to targetFrame-1, so load the next frame to show.
frameLength = parser->getNextChunk(buffer, bufferLength);
if (frameLength == 0){
return false;
}
mFrameCount++;

return true;
}