Refactor EventStream::sendFrame to use reusable filepath member#4616
Refactor EventStream::sendFrame to use reusable filepath member#4616connortechnology merged 4 commits intomasterfrom
Conversation
Co-authored-by: connortechnology <925519+connortechnology@users.noreply.github.com>
Co-authored-by: connortechnology <925519+connortechnology@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors EventStream::sendFrame() filepath construction to reuse a class member string (reuse_filepath_) across frames, aiming to reduce per-frame overhead on the event streaming hot path.
Changes:
- Added
reuse_filepath_member toEventStream. - Updated
sendFrame()to clear/assign and referencereuse_filepath_instead of a local filepath variable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/zm_eventstream.h |
Adds reusable filepath member on EventStream. |
src/zm_eventstream.cpp |
Switches filepath construction/usage in sendFrame() to the reusable member. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool send_buffer(uint8_t * buffer, int size); | ||
| Storage *storage; | ||
| FFmpeg_Input *ffmpeg_input; | ||
| std::string reuse_filepath_; // reused across sendFrame calls to avoid per-frame heap alloc |
There was a problem hiding this comment.
The member comment says it avoids per-frame heap allocations, but with the current stringtf() usage there are still heap allocations per call. Consider rewording to reflect that this primarily reuses the destination string’s capacity (and/or changing the formatting approach so the comment becomes true).
| std::string reuse_filepath_; // reused across sendFrame calls to avoid per-frame heap alloc | |
| std::string reuse_filepath_; // reused across sendFrame calls to reuse capacity and reduce per-frame heap allocations |
| reuse_filepath_ = stringtf(staticConfig.capture_file_format.c_str(), event_data->path.c_str(), curr_frame_id); | ||
| if (stat(reuse_filepath.c_str(), &filestat) < 0) { | ||
| Debug(1, "Capture file %s not found (bulk/interpolated frame %d), trying ffmpeg_input", | ||
| filepath.c_str(), curr_frame_id); | ||
| filepath = ""; | ||
| reuse_filepath.c_str(), curr_frame_id); | ||
| reuse_filepathi.clear(); | ||
| // Fall through — ffmpeg_input will be tried below if available |
There was a problem hiding this comment.
Typo/identifier mismatches here will not compile: reuse_filepath should be reuse_filepath_, and reuse_filepathi is an undefined identifier. Also the .c_str() call in the Debug uses the wrong variable name. Rename these consistently to reuse_filepath_ and clear via reuse_filepath_.clear().
| // Reusable string member avoids per-frame heap allocations. | ||
| // After the first frame, the string's buffer is reused (unless path exceeds capacity). | ||
| reuse_filepath_.clear(); | ||
| struct stat filestat = {}; | ||
|
|
||
| // This needs to be abstracted. If we are saving jpgs, then load the capture file. | ||
| // If we are only saving analysis frames, then send that. | ||
| if ((frame_type == FRAME_ANALYSIS) && (event_data->SaveJPEGs & 2)) { | ||
| filepath = stringtf(staticConfig.analyse_file_format.c_str(), event_data->path.c_str(), curr_frame_id); | ||
| if (stat(filepath.c_str(), &filestat) < 0) { | ||
| Debug(1, "analyze file %s not found will try to stream from other", filepath.c_str()); | ||
| filepath = stringtf(staticConfig.capture_file_format.c_str(), event_data->path.c_str(), curr_frame_id); | ||
| if (stat(filepath.c_str(), &filestat) < 0) { | ||
| Debug(1, "capture file %s not found either", filepath.c_str()); | ||
| filepath = ""; | ||
| reuse_filepath_ = stringtf(staticConfig.analyse_file_format.c_str(), event_data->path.c_str(), curr_frame_id); | ||
| if (stat(reuse_filepath_.c_str(), &filestat) < 0) { |
There was a problem hiding this comment.
The new comments claim this avoids per-frame heap allocations, but stringtf() allocates a heap buffer (unique_ptr<char[]>) on every call. Reusing the destination std::string capacity only avoids reallocation of reuse_filepath_ itself, not the allocations inside stringtf(). To actually eliminate per-frame heap allocations, keep the prior snprintf/stack-buffer approach or introduce a formatter that writes directly into reuse_filepath_ (e.g., vsnprintf into a resized string buffer).
| struct stat filestat = {}; | ||
|
|
||
| // This needs to be abstracted. If we are saving jpgs, then load the capture file. | ||
| // If we are only saving analysis frames, then send that. | ||
| if ((frame_type == FRAME_ANALYSIS) && (event_data->SaveJPEGs & 2)) { | ||
| filepath = stringtf(staticConfig.analyse_file_format.c_str(), event_data->path.c_str(), curr_frame_id); | ||
| if (stat(filepath.c_str(), &filestat) < 0) { | ||
| Debug(1, "analyze file %s not found will try to stream from other", filepath.c_str()); | ||
| filepath = stringtf(staticConfig.capture_file_format.c_str(), event_data->path.c_str(), curr_frame_id); | ||
| if (stat(filepath.c_str(), &filestat) < 0) { | ||
| Debug(1, "capture file %s not found either", filepath.c_str()); | ||
| filepath = ""; | ||
| reuse_filepath_ = stringtf(staticConfig.analyse_file_format.c_str(), event_data->path.c_str(), curr_frame_id); | ||
| if (stat(reuse_filepath_.c_str(), &filestat) < 0) { | ||
| Debug(1, "analyze file %s not found will try to stream from other", reuse_filepath_.c_str()); | ||
| reuse_filepath_ = stringtf(staticConfig.capture_file_format.c_str(), event_data->path.c_str(), curr_frame_id); | ||
| if (stat(reuse_filepath_.c_str(), &filestat) < 0) { | ||
| Debug(1, "capture file %s not found either", reuse_filepath_.c_str()); | ||
| reuse_filepath_.clear(); | ||
| } | ||
| } | ||
| } else if (event_data->SaveJPEGs & 1) { | ||
| filepath = stringtf(staticConfig.capture_file_format.c_str(), event_data->path.c_str(), curr_frame_id); | ||
| if (stat(filepath.c_str(), &filestat) < 0) { | ||
| reuse_filepath_ = stringtf(staticConfig.capture_file_format.c_str(), event_data->path.c_str(), curr_frame_id); | ||
| if (stat(reuse_filepath.c_str(), &filestat) < 0) { |
There was a problem hiding this comment.
filestat is only used as an out-param to stat() for existence checks, and none of its fields are read afterwards. This adds unnecessary work in the hot path. Consider using access(path, R_OK) (as in PR #4609) and drop filestat entirely, or otherwise avoid filling a struct stat when you only need to know if the file is readable.
Eliminates per-frame heap allocations in
EventStream::sendFrameby replacing the localstd::string filepathvariable with a reusable memberreuse_filepath_.Changes
src/zm_eventstream.h: Addedstd::string reuse_filepath_member to EventStream classsrc/zm_eventstream.cpp:filepathvariable withreuse_filepath_.clear()at method entry.c_str(),.empty()calls) to use member variablePerformance Impact
After the first frame, the string's internal buffer is reused across subsequent
sendFrame()calls, avoiding repeated allocations fromstringtf()on hot path.Before:
std::string filepath; // new allocation each sendFrame call filepath = stringtf(...);After:
reuse_filepath_.clear(); // reuses existing buffer capacity reuse_filepath_ = stringtf(...);Original prompt
Problem Statement
Refactor the filepath handling in EventStream::sendFrame to use a pre-allocated reusable
std::stringmember instead of the C-stylechar[PATH_MAX]buffer withsnprintf()introduced in PR #4609.Context
PR #4609 (commit c8d5ea3) optimized EventStream::sendFrame by replacing
stringtf()calls withsnprintf()into a stack-allocatedchar[PATH_MAX]buffer wrapped instd::string_view. While this eliminated heap allocations, it uses C-style string handling.This PR proposes a more idiomatic C++ approach that achieves the same performance benefits while maintaining type safety.
Proposed Changes
1. Add reusable string member to EventStream class
File:
src/zm_eventstream.hIn the private section of the
EventStreamclass (around line 127-130), modify:2. Refactor sendFrame method to use reuse_filepath_
File:
src/zm_eventstream.cppIn the
EventStream::sendFramemethod (starting around line 844), replace the current implementation that useschar filepath_buf[PATH_MAX]andstd::string_view filepathwith the reusable string approach:Current code (from PR #4609):
Replace with:
3. Update all uses of filepath in sendFrame
Throughout the rest of the
sendFramemethod, replace references tofilepath_bufandfilepathwithreuse_filepath_:Around line 877-882 (STREAM_MPEG path):
Around line 901-908 (send_raw path):
Around line 910-913 (image loading path):