fix: critical zero-copy and format conversion bugs#52
Open
Conversation
Fixes 11 critical and medium-priority bugs across backends: CRITICAL (HIGH): - Fix double CVPixelBufferUnlockBaseAddress in Apple camera - Fix double CVPixelBufferUnlockBaseAddress in Apple file reader - Fix use-after-free in Windows file reader DANGLING POINTER (MEDIUM): - Fix dangling nativeHandle in Apple camera after conversion - Fix dangling nativeHandle in Apple file reader after conversion - Fix dangling nativeHandle in DirectShow backend FORMAT CONVERSION (MEDIUM): - Guard shouldConvert against Unknown output format in DShow/MSMF - Log warnings for unsupported YUV-to-different-YUV conversion - Log warnings for unsupported RGB-to-YUV conversion DOCUMENTATION (LOW): - Fix kPixelFormatBGRBit comment typo - Remove misleading @refitem from I420 docs - Clarify zero-copy requirements in PixelFormatOutput - Document Apple YUV subtype behavior All 907 functional tests pass with ASAN enabled. No regressions.
WalkthroughRefines pixel-format conversion and zero-copy decisions across platforms: normalize unspecified output formats to the camera/frame format, add logged early-fails for unsupported RGB→YUV and YUV-subtype conversions, and conditionally unlock/clear native buffers only when conversion actually performed. Changes
Sequence Diagram(s)sequenceDiagram
participant Camera as Camera (native)
participant Provider as Provider (platform reader)
participant Converter as inplaceConvertFrame
participant Frame as Frame (buffer)
rect rgba(200,200,255,0.5)
Camera->>Provider: deliver raw frame (pixelFormat)
end
Provider->>Provider: compute effectiveOutputFormat = outputProp ?: frame.pixelFormat
Provider->>Converter: call inplaceConvertFrame(frame, effectiveOutputFormat)
alt Converter performs zero-copy (no conversion needed)
Converter-->>Provider: returns true (zero-copy)
Provider->>Frame: keep nativeHandle, do not unlock
else Converter performs conversion
Converter-->>Provider: returns false (conversion done)
Provider->>Frame: unlock/release native buffer, clear nativeHandle
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple correctness and safety issues around zero-copy buffer lifetime management, pixel-format conversion edge cases, and API/documentation consistency across Apple and Windows providers.
Changes:
- Fixes zero-copy lifetime bugs (double-unlock, dangling
nativeHandle, and a Windows file-reader zero-copy flag bug). - Hardens pixel-format conversion decisions (avoid attempting conversion when
PixelFormatOutputisUnknown) and adds diagnostics for unsupported conversions. - Updates public documentation/comments for pixel format constants and output-format behavior.
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/ccap_imp_windows_msmf.cpp |
Guards format conversion decision when output format is Unknown. |
src/ccap_imp_windows.h |
COM QueryInterface signature formatting cleanup. |
src/ccap_imp_windows.cpp |
Fixes nativeHandle handling and guards conversion decision when output format is Unknown; formatting cleanup for QueryInterface. |
src/ccap_imp_linux.h |
Minor braced-initializer formatting changes for V4L2 structs. |
src/ccap_imp_apple.mm |
Fixes double-unlock and clears nativeHandle on non-zero-copy conversion success; adds clarifying comment about YUV subtype behavior. |
src/ccap_file_reader_windows.cpp |
Captures conversion return value to correctly update zeroCopy. |
src/ccap_file_reader_apple.mm |
Fixes double-unlock and clears nativeHandle on non-zero-copy conversion success. |
src/ccap_convert_frame.cpp |
Adds warning logs for unsupported conversion cases; adds include for logging utilities. |
include/ccap_def.h |
Documentation fixes/clarifications for pixel-format bits and PixelFormatOutput behavior. |
Comments suppressed due to low confidence (1)
src/ccap_imp_windows_msmf.cpp:815
- Same issue as the DirectShow path:
outputPixelFormat == PixelFormat::UnknowndisablesshouldConvert, but ifshouldFlipis true the code still callsinplaceConvertFrame(..., PixelFormat::Unknown, ...), which can yieldpixelFormat == Unknown/ unintended conversions. TreatUnknownas “no conversion” by computing aneffectiveOutputFormat(fallback tonewFrame->pixelFormat) and using it forisOutputYUV,shouldConvert, and the conversion call.
bool shouldFlip = !isOutputYUV && targetOrientation != m_inputOrientation;
bool shouldConvert = m_frameProp.outputPixelFormat != PixelFormat::Unknown &&
newFrame->pixelFormat != m_frameProp.outputPixelFormat;
bool zeroCopy = !shouldConvert && !shouldFlip;
if (!zeroCopy) {
if (!newFrame->allocator) {
newFrame->allocator = m_allocatorFactory ? m_allocatorFactory() : std::make_shared<DefaultAllocator>();
}
zeroCopy = !inplaceConvertFrame(newFrame.get(), m_frameProp.outputPixelFormat, shouldFlip);
newFrame->sizeInBytes = newFrame->stride[0] * newFrame->height +
(newFrame->stride[1] + newFrame->stride[2]) * newFrame->height / 2;
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/ccap_file_reader_apple.mm (1)
384-404:⚠️ Potential issue | 🟠 MajorUnset
PixelFormatOutputstill forces a bogus conversion here.
setupAssetReader()already chooses BGRA/NV12 whenprop.outputPixelFormatisPixelFormat::Unknown, butshouldConvertstill compares againstUnknown. That sends the frame throughinplaceConvertFrame(..., PixelFormat::Unknown, ...), tags the copied buffer asUnknown, and disables the zero-copy fast path for the default configuration. Derive an effective target format before computingshouldConvertand callinginplaceConvertFrame().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ccap_file_reader_apple.mm` around lines 384 - 404, The code currently uses prop.outputPixelFormat directly even when it's PixelFormat::Unknown, causing unnecessary conversion and disabling zero-copy; compute an effective output pixel format before checking shouldConvert and before calling inplaceConvertFrame: derive effectiveOutput = (prop.outputPixelFormat == PixelFormat::Unknown) ? (isOutputYUV ? PixelFormat::NV12 : PixelFormat::BGRA) : prop.outputPixelFormat (matching setupAssetReader logic), then use effectiveOutput for shouldConvert and pass it to inplaceConvertFrame(newFrame.get(), effectiveOutput, shouldFlip), and ensure newFrame->pixelFormat/nativeHandle are set consistently based on that effective output.src/ccap_file_reader_windows.cpp (1)
449-466:⚠️ Potential issue | 🟠 Major
PixelFormatOutput == Unknownstill breaks the default file-reader path.When callers leave
outputPixelFormatunset,configureOutput()requests RGB32, whileProviderDirectShow::frameOrientation()still defaults toBottomToTop. That makes Line 465 callinplaceConvertFrame(newFrame.get(), PixelFormat::Unknown, true)on default RGB frames, so the copied buffer comes back taggedUnknowninstead of BGRA32. Use the actual reader output format wheneverprop.outputPixelFormatis unset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ccap_file_reader_windows.cpp` around lines 449 - 466, The code uses prop.outputPixelFormat even when it is PixelFormat::Unknown, causing wrong conversion/flip decisions; change the logic to compute an effective output format (e.g., effectiveOutput = prop.outputPixelFormat == PixelFormat::Unknown ? newFrame->pixelFormat : prop.outputPixelFormat) and use that effectiveOutput when computing isOutputYUV, targetOrientation, shouldConvert and when calling inplaceConvertFrame(newFrame.get(), effectiveOutput, ...); this ensures the reader's actual output format (newFrame->pixelFormat) is used whenever prop.outputPixelFormat is unset.src/ccap_imp_windows.cpp (1)
845-857:⚠️ Potential issue | 🟠 MajorRestore orientation from the actual retained format after fallback.
newFrame->orientationis fixed fromm_frameProp.outputPixelFormatbefore we know whether conversion will happen. On DirectShow that is observable:outputPixelFormat == Unknownon a YUV camera now returns a zero-copy YUV frame markedBottomToTop, and RGB→YUV failures return the original RGB sample with aTopToBottomorientation. Use an effective target format for the conversion attempt, and ifzeroCopysurvives, resetorientationfrom the retained input format instead of the requested one.Also applies to: 949-953
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ccap_imp_windows.cpp` around lines 845 - 857, newFrame->orientation is being set from the requested output format before we know if conversion actually occurs; compute an effective target format (e.g. effectiveOutput = (m_frameProp.outputPixelFormat == PixelFormat::Unknown ? m_frameProp.cameraPixelFormat : m_frameProp.outputPixelFormat) or otherwise determine the retained format after a failed conversion) before setting newFrame->orientation, then evaluate shouldConvert/zeroCopy against that effectiveOutput; if zeroCopy remains true, reset newFrame->orientation from the retained input orientation (m_frameOrientation or derived from m_frameProp.cameraPixelFormat) instead of the requested format. Apply the same change to the equivalent block around the other occurrence (the block referenced at 949-953) so orientation is restored from the actual retained format when conversion/fallback logic results in zero-copy.src/ccap_imp_windows_msmf.cpp (1)
750-752:⚠️ Potential issue | 🟠 MajorAvoid calling
inplaceConvertFrame()withPixelFormat::Unknown.The new guard only fixes
shouldConvert. IfoutputPixelFormatis unset and this path is entered because ofshouldFlip, Line 813 still passesPixelFormat::UnknownintoinplaceConvertFrame(), which yields copied frames taggedUnknownand can hit the BGR24swapRBassert. Compute an effective target format first (output != Unknown ? output : newFrame->pixelFormat) and use it forisOutputYUV,shouldConvert, and the conversion call.Also applies to: 803-814
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ccap_imp_windows_msmf.cpp` around lines 750 - 752, The code computes isOutputYUV and shouldConvert using m_frameProp.outputPixelFormat directly, but if outputPixelFormat is Unknown and we still call inplaceConvertFrame we end up passing PixelFormat::Unknown into inplaceConvertFrame (causing copied frames tagged Unknown and downstream asserts). Fix by first computing an effective target pixel format (e.g. PixelFormat effectiveOutput = (m_frameProp.outputPixelFormat != PixelFormat::Unknown) ? m_frameProp.outputPixelFormat : newFrame->pixelFormat), then use effectiveOutput for isOutputYUV, for shouldConvert checks, for determining targetOrientation, and pass effectiveOutput (or ensure inplaceConvertFrame is called with newFrame->pixelFormat updated to a concrete format) when calling inplaceConvertFrame and related conversion logic (apply same change to the logic covering the 803-814 block).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/ccap_file_reader_apple.mm`:
- Around line 384-404: The code currently uses prop.outputPixelFormat directly
even when it's PixelFormat::Unknown, causing unnecessary conversion and
disabling zero-copy; compute an effective output pixel format before checking
shouldConvert and before calling inplaceConvertFrame: derive effectiveOutput =
(prop.outputPixelFormat == PixelFormat::Unknown) ? (isOutputYUV ?
PixelFormat::NV12 : PixelFormat::BGRA) : prop.outputPixelFormat (matching
setupAssetReader logic), then use effectiveOutput for shouldConvert and pass it
to inplaceConvertFrame(newFrame.get(), effectiveOutput, shouldFlip), and ensure
newFrame->pixelFormat/nativeHandle are set consistently based on that effective
output.
In `@src/ccap_file_reader_windows.cpp`:
- Around line 449-466: The code uses prop.outputPixelFormat even when it is
PixelFormat::Unknown, causing wrong conversion/flip decisions; change the logic
to compute an effective output format (e.g., effectiveOutput =
prop.outputPixelFormat == PixelFormat::Unknown ? newFrame->pixelFormat :
prop.outputPixelFormat) and use that effectiveOutput when computing isOutputYUV,
targetOrientation, shouldConvert and when calling
inplaceConvertFrame(newFrame.get(), effectiveOutput, ...); this ensures the
reader's actual output format (newFrame->pixelFormat) is used whenever
prop.outputPixelFormat is unset.
In `@src/ccap_imp_windows_msmf.cpp`:
- Around line 750-752: The code computes isOutputYUV and shouldConvert using
m_frameProp.outputPixelFormat directly, but if outputPixelFormat is Unknown and
we still call inplaceConvertFrame we end up passing PixelFormat::Unknown into
inplaceConvertFrame (causing copied frames tagged Unknown and downstream
asserts). Fix by first computing an effective target pixel format (e.g.
PixelFormat effectiveOutput = (m_frameProp.outputPixelFormat !=
PixelFormat::Unknown) ? m_frameProp.outputPixelFormat : newFrame->pixelFormat),
then use effectiveOutput for isOutputYUV, for shouldConvert checks, for
determining targetOrientation, and pass effectiveOutput (or ensure
inplaceConvertFrame is called with newFrame->pixelFormat updated to a concrete
format) when calling inplaceConvertFrame and related conversion logic (apply
same change to the logic covering the 803-814 block).
In `@src/ccap_imp_windows.cpp`:
- Around line 845-857: newFrame->orientation is being set from the requested
output format before we know if conversion actually occurs; compute an effective
target format (e.g. effectiveOutput = (m_frameProp.outputPixelFormat ==
PixelFormat::Unknown ? m_frameProp.cameraPixelFormat :
m_frameProp.outputPixelFormat) or otherwise determine the retained format after
a failed conversion) before setting newFrame->orientation, then evaluate
shouldConvert/zeroCopy against that effectiveOutput; if zeroCopy remains true,
reset newFrame->orientation from the retained input orientation
(m_frameOrientation or derived from m_frameProp.cameraPixelFormat) instead of
the requested format. Apply the same change to the equivalent block around the
other occurrence (the block referenced at 949-953) so orientation is restored
from the actual retained format when conversion/fallback logic results in
zero-copy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 085631ae-50ad-485b-bb86-f7ee4390c26c
📒 Files selected for processing (9)
include/ccap_def.hsrc/ccap_convert_frame.cppsrc/ccap_file_reader_apple.mmsrc/ccap_file_reader_windows.cppsrc/ccap_imp_apple.mmsrc/ccap_imp_linux.hsrc/ccap_imp_windows.cppsrc/ccap_imp_windows.hsrc/ccap_imp_windows_msmf.cpp
Owner
Author
|
@Auggie review |
- Derive effectiveOutputFormat to avoid passing PixelFormat::Unknown to inplaceConvertFrame across all providers (DirectShow, MSMF, V4L2, AVFoundation) and file readers (Windows MF, Apple AVAssetReader). When outputPixelFormat is Unknown, treat it as 'keep input format' so isOutputYUV, shouldConvert, shouldFlip, and the conversion call all use the camera/input format instead. - Use log-once pattern for unsupported YUV-to-YUV (without libyuv) and RGB-to-YUV conversion warnings to prevent flooding stderr in long-running captures. - Update PixelFormatOutput documentation to clarify Unknown semantics and unsupported conversion limitations.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ccap_convert_frame.cpp`:
- Around line 233-240: Change the one-time warning flags in
inplaceConvertFrame() from unprotected static bool to std::atomic_bool and use
an atomic compare-and-exchange to set the flag and decide whether to log, e.g.,
declare static std::atomic_bool sLoggedYuv2YuvUnsupported{false} and replace the
current if-check/assignment with a compare_exchange_strong-based operation so
only the thread that successfully flips false->true emits CCAP_LOG_W; do the
same for the other static warning flag(s), and add `#include` <atomic>.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67c2375a-b458-4842-9311-359b3f82ddc9
📒 Files selected for processing (8)
include/ccap_def.hsrc/ccap_convert_frame.cppsrc/ccap_file_reader_apple.mmsrc/ccap_file_reader_windows.cppsrc/ccap_imp_apple.mmsrc/ccap_imp_linux.cppsrc/ccap_imp_windows.cppsrc/ccap_imp_windows_msmf.cpp
✅ Files skipped from review due to trivial changes (2)
- src/ccap_file_reader_apple.mm
- include/ccap_def.h
🚧 Files skipped from review as they are similar to previous changes (4)
- src/ccap_imp_windows_msmf.cpp
- src/ccap_file_reader_windows.cpp
- src/ccap_imp_apple.mm
- src/ccap_imp_windows.cpp
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.
Summary
This PR fixes 11 critical and medium-priority bugs across zero-copy buffer handling, pixel format conversion, and API consistency. All issues were identified through comprehensive code audit and verified through automated testing.
Bugs Fixed
Critical (HIGH) — Memory Safety
Double CVPixelBufferUnlockBaseAddress in Apple camera (ccap_imp_apple.mm:927)
Double CVPixelBufferUnlockBaseAddress in Apple file reader (ccap_file_reader_apple.mm:401)
Use-after-free in Windows file reader (ccap_file_reader_windows.cpp:465)
Dangling Pointer (MEDIUM) — Resource Management
4-6. Dangling nativeHandle after conversion
Format Conversion (MEDIUM) — Edge Cases
8-9. Silent format conversion failures (ccap_convert_frame.cpp:235)
Documentation (LOW) — Clarity & Accuracy
Testing & Verification
✅ All 907 functional tests pass
✅ AddressSanitizer memory checks enabled - no violations
✅ Build succeeds on macOS (Debug and Release)
✅ Code formatted with clang-format
✅ No regressions in existing test suite
Files Changed
Summary by CodeRabbit
Bug Fixes
Documentation