Add zero-copy readRef(); document thread-safety#75
Merged
Conversation
#28 was closed as COMPLETED citing #36, but #36 was closed without being merged and readRef() never existed in main. The README has been listing "Zero-copy frames | readRef() avoids per-frame clone" as a feature the whole time, so the promise was already public -- only the code was missing. Re-applies the design from the stranded feat/zero-copy-read branch rather than rebasing it: that commit predates both the rename and the EOF-drain fix, and its diff sits inside the decode loop that fix changed. The decode loop is now factored into decodeNextFrame(), shared by read() (clones) and readRef() (does not). The aliasing is the whole point, so a test asserts it directly: the Mat returned by readRef() must keep the same data pointer and have its contents overwritten by the next decode. Verified that test does real work by making readRef() clone -- exactly one assertion fails, and the same-pixels and read()-independence tests keep passing, which is why the aliasing check has to exist separately. readRef() is deliberately not exposed to Python. A numpy array aliasing a buffer the reader overwrites is a far sharper footgun than a cv::Mat in a C++ scope, with no idiom to bound its lifetime. Also documents thread-safety, which was written down nowhere: instances are not thread-safe, separate instances are fine, and the log level is process-global and unsynchronised -- g_logLevel is a plain namespace-scope variable that setLogLevel() writes with no guard, so changing it while another thread logs is a data race. Fixes #73, fixes #74. Reopens and closes #28. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #73, fixes #74.
Why land it rather than just correct the tracker
#73 framed this as a choice: implement
readRef(), or reopen #28 so the record is honest. While working on it I found the deciding fact —main's README has been advertising the method all along:So the API was already promised publicly; only the implementation was missing. #36 was closed unmerged, #28 was closed as COMPLETED citing it, and the README kept the claim. Correcting the tracker alone would have left the README lying.
Approach
Re-applied the design from the stranded
feat/zero-copy-readbranch rather than rebasing it. That commit predates both the rename and the EOF-drain fix from #71, and its diff sits inside the decode loop that fix rewrote — a rebase would have conflicted exactly where correctness matters most.The decode loop is now
decodeNextFrame(), shared byread()(clones) andreadRef()(does not).The test that earns its keep
A
readRef()that quietly kept cloning would pass any "same pixels" check. So the aliasing is asserted directly: the returnedMatmust keep the samedatapointer and have its contents overwritten by the next decode.Verified by sabotage — making
readRef()clone produces exactly one failure, the aliasing assertion. The same-pixels andread()-independence tests keep passing, which is precisely why the aliasing check has to exist as its own test.44 cases, 1179 assertions, zero skips.
Deliberately not exposed to Python
A numpy array aliasing a buffer the reader overwrites in place is a far sharper footgun than a
cv::Matin a C++ scope — there's no idiom to bound its lifetime, and the failure is silent data corruption rather than a crash.read()remains the only Python path. Happy to revisit if you want it, but it deserves its own design discussion.Thread-safety (#74)
Documented on both classes and on
setLogLevel(). The substantive finding:g_logLevelis a plain namespace-scope variable thatsetLogLevel()writes with no synchronisation, so it is process-global shared state and changing it while another thread logs is a data race. The header now says to set it once at start-up. I confirmed this by readingLogLevel.cpprather than assuming it, since #74 flagged it as needing verification.🤖 Generated with Claude Code