feat: add video file writing support for macOS and Windows#55
Open
wysaid wants to merge 1 commit into
Open
Conversation
Add VideoWriter class to encode frames into MP4/MOV video files using system frameworks only (AVAssetWriter on macOS, IMFMediaSink on Windows). - C++ API (VideoWriter) and pure C API (ccap_video_writer_*) - HEVC codec with automatic H.264 fallback - Supports BGR24, BGRA32, I420, NV12 pixel formats - New CMake option CCAP_ENABLE_VIDEO_WRITER (ON by default) - 15 unit tests covering lifecycle, frame writing, codec fallback Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
WalkthroughThis PR introduces a complete video writing capability to the camera capture library, supporting macOS (via AVAssetWriter) and Windows (via Media Foundation), with both C++ and C APIs, comprehensive error handling, and full test coverage. ChangesVideo Writer Feature Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
src/ccap_writer_imp.h (1)
27-28: 💤 Low valueConsider making data members protected.
m_actualCodecandm_configare currently public, which technically breaks encapsulation. While this is acceptable for an internal implementation interface, making themprotectedwith accessor methods would follow better OOP practices.🔒 Suggested encapsulation improvement
+ VideoCodec actualCodec() const { return m_actualCodec; } + const WriterConfig& config() const { return m_config; } + +protected: VideoCodec m_actualCodec; WriterConfig m_config; };Then update callers in
ccap_writer.mmto use the accessors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ccap_writer_imp.h` around lines 27 - 28, Change the public data members m_actualCodec and m_config in the CCAPWriterImp class to protected and add const and non-const accessor methods (e.g., VideoCodec& actualCodec()/const VideoCodec& actualCodec() const and WriterConfig& config()/const WriterConfig& config() const) so callers use accessors instead of direct member access; then update all uses in ccap_writer.mm to call actualCodec()/config() (or the const variants) rather than referencing m_actualCodec and m_config directly.include/ccap_writer.h (1)
96-99: ⚡ Quick winConsider using
Impl*instead ofvoid*for type safety.Line 96 forward-declares
struct Impl, which means you can useImpl* m_implinstead ofvoid* m_implon line 99. UsingImpl*provides:
- Compile-time type checking
- No casting required in implementation
- Clearer intent
📝 Suggested change
struct Impl; private: - void* m_impl; + Impl* m_impl; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/ccap_writer.h` around lines 96 - 99, The member m_impl is declared as void* while a forward-declared struct Impl exists; change the member type to Impl* (replace void* m_impl with Impl* m_impl) to gain type safety and avoid casts, and update any file-scope uses (constructors, destructor, methods in the class that currently cast m_impl) to use Impl* directly (adjust new/delete or static_cast usages accordingly) so all references use the named Impl type instead of void*.include/ccap_writer_c.h (1)
29-38: ⚡ Quick winAdd static_assert checks for C/C++ enum consistency.
The C enums
CcapVideoCodecandCcapVideoFormatshould have correspondingstatic_assertchecks insrc/ccap_c.cppto ensure they match the C++VideoCodecandVideoFormatenums, following the established pattern used forCcapPixelFormat,CcapPropertyName, andCcapErrorCode.📋 Suggested assertions to add in src/ccap_c.cpp
// VideoCodec enum consistency checks static_assert(static_cast<uint32_t>(CCAP_VIDEO_CODEC_HEVC) == static_cast<uint32_t>(ccap::VideoCodec::HEVC), "C and C++ VideoCodec::HEVC values must match"); static_assert(static_cast<uint32_t>(CCAP_VIDEO_CODEC_H264) == static_cast<uint32_t>(ccap::VideoCodec::H264), "C and C++ VideoCodec::H264 values must match"); // VideoFormat enum consistency checks static_assert(static_cast<uint32_t>(CCAP_VIDEO_FORMAT_MP4) == static_cast<uint32_t>(ccap::VideoFormat::MP4), "C and C++ VideoFormat::MP4 values must match"); static_assert(static_cast<uint32_t>(CCAP_VIDEO_FORMAT_MOV) == static_cast<uint32_t>(ccap::VideoFormat::MOV), "C and C++ VideoFormat::MOV values must match");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/ccap_writer_c.h` around lines 29 - 38, Add compile-time consistency checks in src/ccap_c.cpp to ensure the C enums match the C++ enums: add static_asserts comparing CCAP_VIDEO_CODEC_HEVC and CCAP_VIDEO_CODEC_H264 with ccap::VideoCodec::HEVC and ccap::VideoCodec::H264, and CCAP_VIDEO_FORMAT_MP4 and CCAP_VIDEO_FORMAT_MOV with ccap::VideoFormat::MP4 and ccap::VideoFormat::MOV, using static_cast<uint32_t> on both sides and clear error messages indicating which value must match (follow the pattern used for CcapPixelFormat, CcapPropertyName, and CcapErrorCode).tests/test_video_writer.cpp (1)
58-66: ⚡ Quick winUse error_code versions of filesystem operations in TearDown.
The directory iteration and file removal can throw exceptions if the temp directory is inaccessible, files are locked, or permissions are denied. Consider using
std::error_codeoverloads to handle errors gracefully.Additionally, iterating the entire temp directory may be slow if it contains many files. Consider tracking created file paths in a
std::vector<fs::path>member variable during test execution and removing only those files in TearDown.♻️ Example using error_code
void TearDown() override { - for (const auto& entry : fs::directory_iterator(fs::temp_directory_path())) { + std::error_code ec; + for (const auto& entry : fs::directory_iterator(fs::temp_directory_path(), ec)) { + if (ec) break; // Skip cleanup if iteration fails std::string filename = entry.path().filename().string(); if (filename.find("ccap_writer_test_") == 0) { - fs::remove(entry.path()); + fs::remove(entry.path(), ec); // Ignore errors - best effort cleanup } } }Also applies to: 78-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_video_writer.cpp` around lines 58 - 66, TearDown currently iterates the entire temp directory and uses throwing filesystem calls; change it to (1) record created test files during the test run by pushing their fs::path into a member std::vector<fs::path> (e.g., created_test_files_), and (2) in TearDown(), iterate that vector and remove each file using the std::error_code overloads (fs::remove(path, ec) and check ec) instead of fs::directory_iterator and throwing fs::remove; also apply the same error_code-based removal pattern to the similar cleanup block at lines 78-85 so teardown no longer throws and only touches files the test created.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ccap_writer_apple.mm`:
- Around line 280-286: Treat timestampNs as a sentinel (e.g., UINT64_MAX) so a
legitimate timestamp of 0 is accepted: if timestampNs != UINT64_MAX use CMTime
with a nanosecond timescale (timescale = 1_000_000_000) and set presentationTime
from timestampNs so zero is preserved; otherwise synthesize PTS from
m_frameCount using m_config.frameRate but avoid truncating fractional FPS by
computing frameDurationNs = round(1e9 / frameRate) (use a default frameRate if
<=0), then set presentationTime = CMTimeMake(static_cast<int64_t>(m_frameCount)
* frameDurationNs, 1000000000) so NTSC rates like 29.97/23.976 are represented
accurately (refer to timestampNs, m_config.frameRate, m_frameCount, and the
CMTimeMake usage).
- Around line 310-360: bgr24ToNv12 and bgra32ToNv12 perform unconditional writes
that overflow for odd heights and leave the rightmost column uninitialized for
odd widths: change w2 = width/2 to w2 = (width+1)/2 and in both functions guard
the writes that touch the bottom row (dstY[(y+1) * dstYStride + ...] and the
corresponding dstUV indices) with a check y+1 < height so you only write the
second-row pixels when present; additionally, handle the trailing odd column
inside the inner loop (or after it) by replicating the last source pixel (use
line0/line1 samples for the final column) so the final dstY and dstUV entries
are set instead of left garbage; apply these fixes to both bgr24ToNv12 and
bgra32ToNv12 and use dstYStride/dstUVStride offsets as already computed.
- Around line 149-184: In close() of WriterApple (ccap_writer_apple.mm) the
async block captures this via m_assetWriter and signals the stack-local sem,
causing use-after-free if dispatch_semaphore_wait times out and the instance is
destroyed; also queue is leaked. Fix by heap-allocating or otherwise owning the
semaphore and asset writer for the block (capture local strong references like
auto writer = m_assetWriter; dispatch_semaphore_t semRef = sem; or allocate sem
on the heap) so the block references detached objects, ensure the block always
releases/signals and call dispatch_release(queue) when the async completion
handler runs (or release after dispatch_semaphore_wait if you move to a
heap/owned sem), and replace the unsafe timed wait with a safer strategy (e.g.,
DISPATCH_TIME_FOREVER or cancelable workflow) so the block cannot access freed
WriterApple members; update uses of m_assetWriter/m_writerInput to use the
captured locals inside the dispatched block.
In `@src/ccap_writer_c.cpp`:
- Around line 84-89: The code computes a fallback timestamp into frame.timestamp
but still passes the original timestampNs to cppWriter->writeFrame; change the
call to pass the populated frame.timestamp instead of timestampNs so the
fallback is actually used (update the call to cppWriter->writeFrame(frame,
frame.timestamp) where frame, frameInfo, timestampNs and cppWriter->writeFrame
are referenced).
- Around line 24-28: Wrap the four exported C functions
(ccap_video_writer_destroy, ccap_video_writer_close,
ccap_video_writer_is_opened, ccap_video_writer_actual_codec) in the same
try/catch pattern used by ccap_video_writer_create/open/write to prevent C++
exceptions from escaping the C ABI: enclose the existing bodies in try { ... }
catch (const std::exception& e) { /* mirror existing handler: log the error and
return the same error/sentinel used in create/open/write */ } catch (...) { /*
mirror existing handler for unknown exceptions */ } — for destroy/close swallow
the exception after logging, for is_opened return the same false/error sentinel
used elsewhere on exception, and for actual_codec return the same
codec-sentinel/zero value used by the other functions on error so behavior is
consistent with the already-handled wrappers.
In `@src/ccap_writer_windows.cpp`:
- Around line 71-80: close() currently ignores the HRESULT from
m_sinkWriter->Finalize() and returns early without clearing state; capture the
return value (HRESULT hr = m_sinkWriter->Finalize()), check FAILED(hr) and log a
descriptive error including the HRESULT so finalize failures (e.g., disk full)
are visible; always Release() and null out m_sinkWriter afterwards. Also ensure
close() clears/zeros the instance state (set m_isOpened = false, reset m_config
and m_frameCount) even on early-return paths so a subsequent open() won't see
stale config/frame count — move/adjust the early-return or perform the resets
before returning.
- Around line 278-327: convertToNv12 currently handles only BGR24 and NV12/NV12f
and also reads/writes out-of-bounds on odd widths/heights; add BGRA32 and
I420/I420f conversion branches (reuse the conversion logic from the macOS
writeFrame for BGRA32 and I420/I420f) so the function supports the same formats
as the public API, and fix the odd-dimension OOB by clamping the per-2x2 loop so
you never read past the row end (compute a safe right pixel count instead of
assuming x*6 bytes exist) and only write the lower-row Y/UV when (y+1) < h;
reference symbols: convertToNv12, w2, h2, l0/l1, dstY/dstUV,
PixelFormat::BGRA32, PixelFormat::I420, PixelFormat::I420f, PixelFormat::NV12f.
- Line 52: The current conversion std::wstring widePath(filePath.begin(),
filePath.end()) corrupts UTF-8 multi-byte paths; replace this per-byte widening
with a proper UTF-8→UTF-16 conversion using MultiByteToWideChar(CP_UTF8, ...) to
produce widePath from filePath, allocate the correct buffer length (call
MultiByteToWideChar once to get required size, then again to fill the buffer)
and use that widePath when calling MFCreateMediaSinkForURL (or any other Windows
API expecting wchar_t*); ensure you free or use a std::wstring sized to the
returned length and handle conversion errors.
In `@src/ccap_writer.mm`:
- Around line 75-78: The writeFrame path silently returns false when m_impl is
null; update VideoWriter::writeFrame to mirror open() by calling
reportError(...) with a clear message when m_impl is nullptr before returning
false, and then proceed to call impl(m_impl)->writeFrame(...) when available;
also apply the same pattern to the stub implementation of writeFrame (the
fallback/stub class method) so it reports the same error via reportError(...)
instead of failing silently.
---
Nitpick comments:
In `@include/ccap_writer_c.h`:
- Around line 29-38: Add compile-time consistency checks in src/ccap_c.cpp to
ensure the C enums match the C++ enums: add static_asserts comparing
CCAP_VIDEO_CODEC_HEVC and CCAP_VIDEO_CODEC_H264 with ccap::VideoCodec::HEVC and
ccap::VideoCodec::H264, and CCAP_VIDEO_FORMAT_MP4 and CCAP_VIDEO_FORMAT_MOV with
ccap::VideoFormat::MP4 and ccap::VideoFormat::MOV, using static_cast<uint32_t>
on both sides and clear error messages indicating which value must match (follow
the pattern used for CcapPixelFormat, CcapPropertyName, and CcapErrorCode).
In `@include/ccap_writer.h`:
- Around line 96-99: The member m_impl is declared as void* while a
forward-declared struct Impl exists; change the member type to Impl* (replace
void* m_impl with Impl* m_impl) to gain type safety and avoid casts, and update
any file-scope uses (constructors, destructor, methods in the class that
currently cast m_impl) to use Impl* directly (adjust new/delete or static_cast
usages accordingly) so all references use the named Impl type instead of void*.
In `@src/ccap_writer_imp.h`:
- Around line 27-28: Change the public data members m_actualCodec and m_config
in the CCAPWriterImp class to protected and add const and non-const accessor
methods (e.g., VideoCodec& actualCodec()/const VideoCodec& actualCodec() const
and WriterConfig& config()/const WriterConfig& config() const) so callers use
accessors instead of direct member access; then update all uses in
ccap_writer.mm to call actualCodec()/config() (or the const variants) rather
than referencing m_actualCodec and m_config directly.
In `@tests/test_video_writer.cpp`:
- Around line 58-66: TearDown currently iterates the entire temp directory and
uses throwing filesystem calls; change it to (1) record created test files
during the test run by pushing their fs::path into a member
std::vector<fs::path> (e.g., created_test_files_), and (2) in TearDown(),
iterate that vector and remove each file using the std::error_code overloads
(fs::remove(path, ec) and check ec) instead of fs::directory_iterator and
throwing fs::remove; also apply the same error_code-based removal pattern to the
similar cleanup block at lines 78-85 so teardown no longer throws and only
touches files the test created.
🪄 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: f34140e1-3d75-4f4d-92aa-572398fbef1d
📒 Files selected for processing (14)
CMakeLists.txtinclude/ccap_c.hinclude/ccap_def.hinclude/ccap_writer.hinclude/ccap_writer_c.hsrc/ccap_c.cppsrc/ccap_utils.cppsrc/ccap_writer.mmsrc/ccap_writer_apple.mmsrc/ccap_writer_c.cppsrc/ccap_writer_imp.hsrc/ccap_writer_windows.cpptests/CMakeLists.txttests/test_video_writer.cpp
Comment on lines
+149
to
+184
| void close() override { | ||
| if (!m_isOpened) return; | ||
| m_isOpened = false; | ||
|
|
||
| @try { | ||
| if (m_writerInput) { | ||
| [m_writerInput markAsFinished]; | ||
| } | ||
| if (m_assetWriter) { | ||
| // Use a background queue to avoid blocking the calling thread | ||
| // which allows the completion handler to execute | ||
| dispatch_queue_t queue = dispatch_queue_create("com.ccap.writer.close", DISPATCH_QUEUE_SERIAL); | ||
| dispatch_semaphore_t sem = dispatch_semaphore_create(0); | ||
| dispatch_async(queue, ^{ | ||
| [m_assetWriter finishWritingWithCompletionHandler:^{ | ||
| dispatch_semaphore_signal(sem); | ||
| }]; | ||
| }); | ||
| // Wait for completion | ||
| dispatch_semaphore_wait(sem, dispatch_time(DISPATCH_TIME_NOW, 10 * NSEC_PER_SEC)); | ||
|
|
||
| if (m_assetWriter.error) { | ||
| CCAP_LOG_E("finishWriting failed: %s\n", | ||
| m_assetWriter.error.localizedDescription.UTF8String); | ||
| } | ||
| } | ||
| } | ||
| @catch (NSException* e) { | ||
| CCAP_LOG_E("Exception during writer close: %s\n", e.reason.UTF8String); | ||
| } | ||
|
|
||
| m_pixelBufferAdaptor = nil; | ||
| m_writerInput = nil; | ||
| m_assetWriter = nil; | ||
| std::memset(&m_config, 0, sizeof(m_config)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
find . -name "ccap_writer_apple.mm" -type f 2>/dev/null | head -5Repository: wysaid/CameraCapture
Length of output: 91
🏁 Script executed:
# Look for build configuration files
find . -name "*.xcconfig" -o -name "CMakeLists.txt" -o -name "*.pbxproj" | head -20Repository: wysaid/CameraCapture
Length of output: 232
🏁 Script executed:
# Search for ARC compiler flags in the codebase
rg -i "fobjc-arc|objc-arc" --type-list | head -20Repository: wysaid/CameraCapture
Length of output: 454
🏁 Script executed:
# Read the actual file to verify the code snippet
wc -l ./src/ccap_writer_apple.mmRepository: wysaid/CameraCapture
Length of output: 95
🏁 Script executed:
# Read the specific lines mentioned (149-184)
sed -n '149,184p' ./src/ccap_writer_apple.mmRepository: wysaid/CameraCapture
Length of output: 1490
🏁 Script executed:
# Search for ARC configuration in CMakeLists.txt
cat ./CMakeLists.txtRepository: wysaid/CameraCapture
Length of output: 16707
🏁 Script executed:
# Search for ARC flag in the project
rg "fobjc-arc" .Repository: wysaid/CameraCapture
Length of output: 228
🏁 Script executed:
# Find the WriterApple class definition to understand member types
rg "class WriterApple" -A 30 ./src/ccap_writer_apple.mmRepository: wysaid/CameraCapture
Length of output: 1254
🏁 Script executed:
# Check if there are any dispatch_release calls in the file
rg "dispatch_release" ./src/ccap_writer_apple.mmRepository: wysaid/CameraCapture
Length of output: 46
🏁 Script executed:
# Look for dispatch_queue_create or dispatch_semaphore patterns elsewhere
rg "dispatch_semaphore|dispatch_queue_create" ./src/ -A 5 -B 2Repository: wysaid/CameraCapture
Length of output: 3949
🏁 Script executed:
# Verify the class has a destructor and see the full close() context
rg "~WriterApple|void close\(\)" ./src/ccap_writer_apple.mm -B 2 -A 15Repository: wysaid/CameraCapture
Length of output: 1462
🏁 Script executed:
# Check if there are similar async patterns elsewhere in the codebase
rg "dispatch_async|@try" ./src/ --type mm --type cpp -lRepository: wysaid/CameraCapture
Length of output: 91
Timeout-induced use-after-free: async block will dereference destroyed instance and freed semaphore.
The async block implicitly captures this through m_assetWriter and signals the stack-local sem. If dispatch_semaphore_wait times out (10s), close() returns while the block is still pending. The caller can then destroy WriterApple, causing the async block to:
- Dereference a dangling
thispointer viam_assetWriter(use-after-free) - Signal a freed semaphore that left the stack scope (undefined behavior)
Additionally, queue is created but never released, causing a dispatch queue leak in all code paths.
With ARC enabled (-fobjc-arc), Objective-C object lifetimes are managed, but dispatch_queue_t and dispatch_semaphore_t are not covered by ARC and require explicit dispatch_release().
Recommendations:
- Capture both
writerandsemby value into the block to detach their lifetimes - Call
dispatch_release(queue)after the async block completes - Consider cancelling the async operation or using indefinite wait (
DISPATCH_TIME_FOREVER) instead of relying on timeout; timeout without cleanup is fundamentally unsafe
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ccap_writer_apple.mm` around lines 149 - 184, In close() of WriterApple
(ccap_writer_apple.mm) the async block captures this via m_assetWriter and
signals the stack-local sem, causing use-after-free if dispatch_semaphore_wait
times out and the instance is destroyed; also queue is leaked. Fix by
heap-allocating or otherwise owning the semaphore and asset writer for the block
(capture local strong references like auto writer = m_assetWriter;
dispatch_semaphore_t semRef = sem; or allocate sem on the heap) so the block
references detached objects, ensure the block always releases/signals and call
dispatch_release(queue) when the async completion handler runs (or release after
dispatch_semaphore_wait if you move to a heap/owned sem), and replace the unsafe
timed wait with a safer strategy (e.g., DISPATCH_TIME_FOREVER or cancelable
workflow) so the block cannot access freed WriterApple members; update uses of
m_assetWriter/m_writerInput to use the captured locals inside the dispatched
block.
Comment on lines
+280
to
+286
| CMTime presentationTime; | ||
| if (timestampNs > 0) { | ||
| presentationTime = CMTimeMake(static_cast<int64_t>(timestampNs), 1000000000); | ||
| } else { | ||
| double fps = m_config.frameRate > 0 ? m_config.frameRate : 30.0; | ||
| presentationTime = CMTimeMake(static_cast<int64_t>(m_frameCount), static_cast<int32_t>(fps)); | ||
| } |
There was a problem hiding this comment.
Timestamp handling: timestampNs > 0 rejects valid t=0, and fps cast loses fractional rates.
timestampNs > 0falls through to the synthesized PTS even when the caller passes a legitimate timestamp of0(e.g., the first frame anchored to session start). Most APIs in this domain use a sentinel likeUINT64_MAXor a separate flag.CMTimeMake(..., static_cast<int32_t>(fps))truncates29.97,23.976,59.94, etc. to the integer floor, producing wrong durations for NTSC rates. Prefer a fixed high timescale and scale by frames:
- } else {
- double fps = m_config.frameRate > 0 ? m_config.frameRate : 30.0;
- presentationTime = CMTimeMake(static_cast<int64_t>(m_frameCount), static_cast<int32_t>(fps));
- }
+ } else {
+ double fps = m_config.frameRate > 0 ? m_config.frameRate : 30.0;
+ // Use a high timescale to preserve fractional frame rates (e.g., 29.97).
+ const int32_t timescale = 600;
+ presentationTime = CMTimeMake(
+ static_cast<int64_t>(llround(m_frameCount * timescale / fps)), timescale);
+ }📝 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
| CMTime presentationTime; | |
| if (timestampNs > 0) { | |
| presentationTime = CMTimeMake(static_cast<int64_t>(timestampNs), 1000000000); | |
| } else { | |
| double fps = m_config.frameRate > 0 ? m_config.frameRate : 30.0; | |
| presentationTime = CMTimeMake(static_cast<int64_t>(m_frameCount), static_cast<int32_t>(fps)); | |
| } | |
| CMTime presentationTime; | |
| if (timestampNs > 0) { | |
| presentationTime = CMTimeMake(static_cast<int64_t>(timestampNs), 1000000000); | |
| } else { | |
| double fps = m_config.frameRate > 0 ? m_config.frameRate : 30.0; | |
| // Use a high timescale to preserve fractional frame rates (e.g., 29.97). | |
| const int32_t timescale = 600; | |
| presentationTime = CMTimeMake( | |
| static_cast<int64_t>(llround(m_frameCount * timescale / fps)), timescale); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ccap_writer_apple.mm` around lines 280 - 286, Treat timestampNs as a
sentinel (e.g., UINT64_MAX) so a legitimate timestamp of 0 is accepted: if
timestampNs != UINT64_MAX use CMTime with a nanosecond timescale (timescale =
1_000_000_000) and set presentationTime from timestampNs so zero is preserved;
otherwise synthesize PTS from m_frameCount using m_config.frameRate but avoid
truncating fractional FPS by computing frameDurationNs = round(1e9 / frameRate)
(use a default frameRate if <=0), then set presentationTime =
CMTimeMake(static_cast<int64_t>(m_frameCount) * frameDurationNs, 1000000000) so
NTSC rates like 29.97/23.976 are represented accurately (refer to timestampNs,
m_config.frameRate, m_frameCount, and the CMTimeMake usage).
Comment on lines
+310
to
+360
| void bgr24ToNv12(const uint8_t* src, int srcStride, | ||
| uint8_t* dstY, int dstYStride, | ||
| uint8_t* dstUV, int dstUVStride, | ||
| int width, int height) { | ||
| int w2 = width / 2; | ||
| const uint8_t* line = src; | ||
| for (int y = 0; y < height; y += 2) { | ||
| const uint8_t* line0 = line; | ||
| const uint8_t* line1 = (y + 1 < height) ? line + srcStride : line; | ||
| for (int x = 0; x < w2; x++) { | ||
| int b0 = line0[x*6+0], g0 = line0[x*6+1], r0 = line0[x*6+2]; | ||
| int b1 = line0[x*6+3], g1 = line0[x*6+4], r1 = line0[x*6+5]; | ||
| int b2 = line1[x*6+0], g2 = line1[x*6+1], r2 = line1[x*6+2]; | ||
| int b3 = line1[x*6+3], g3 = line1[x*6+4], r3 = line1[x*6+5]; | ||
| dstY[y * dstYStride + x*2] = static_cast<uint8_t>((66*r0+129*g0+25*b0+128)>>8)+16; | ||
| dstY[y * dstYStride + x*2+1] = static_cast<uint8_t>((66*r1+129*g1+25*b1+128)>>8)+16; | ||
| dstY[(y+1) * dstYStride + x*2] = static_cast<uint8_t>((66*r2+129*g2+25*b2+128)>>8)+16; | ||
| dstY[(y+1) * dstYStride + x*2+1] = static_cast<uint8_t>((66*r3+129*g3+25*b3+128)>>8)+16; | ||
| int bAvg = (b0+b1+b2+b3)/4, rAvg = (r0+r1+r2+r3)/4, gAvg = (g0+g1+g2+g3)/4; | ||
| dstUV[(y/2) * dstUVStride + x*2] = static_cast<uint8_t>((-38*rAvg-74*gAvg+112*bAvg+128)>>8)+128; | ||
| dstUV[(y/2) * dstUVStride + x*2+1] = static_cast<uint8_t>((112*rAvg-94*gAvg-18*bAvg+128)>>8)+128; | ||
| } | ||
| line += srcStride * 2; | ||
| } | ||
| } | ||
|
|
||
| void bgra32ToNv12(const uint8_t* src, int srcStride, | ||
| uint8_t* dstY, int dstYStride, | ||
| uint8_t* dstUV, int dstUVStride, | ||
| int width, int height) { | ||
| int w2 = width / 2; | ||
| const uint8_t* line = src; | ||
| for (int y = 0; y < height; y += 2) { | ||
| const uint8_t* line0 = line; | ||
| const uint8_t* line1 = (y + 1 < height) ? line + srcStride : line; | ||
| for (int x = 0; x < w2; x++) { | ||
| int b0 = line0[x*8+0], g0 = line0[x*8+1], r0 = line0[x*8+2]; | ||
| int b1 = line0[x*8+4], g1 = line0[x*8+5], r1 = line0[x*8+6]; | ||
| int b2 = line1[x*8+0], g2 = line1[x*8+1], r2 = line1[x*8+2]; | ||
| int b3 = line1[x*8+4], g3 = line1[x*8+5], r3 = line1[x*8+6]; | ||
| dstY[y * dstYStride + x*2] = static_cast<uint8_t>((66*r0+129*g0+25*b0+128)>>8)+16; | ||
| dstY[y * dstYStride + x*2+1] = static_cast<uint8_t>((66*r1+129*g1+25*b1+128)>>8)+16; | ||
| dstY[(y+1) * dstYStride + x*2] = static_cast<uint8_t>((66*r2+129*g2+25*b2+128)>>8)+16; | ||
| dstY[(y+1) * dstYStride + x*2+1] = static_cast<uint8_t>((66*r3+129*g3+25*b3+128)>>8)+16; | ||
| int bAvg = (b0+b1+b2+b3)/4, rAvg = (r0+r1+r2+r3)/4, gAvg = (g0+g1+g2+g3)/4; | ||
| dstUV[(y/2) * dstUVStride + x*2] = static_cast<uint8_t>((-38*rAvg-74*gAvg+112*bAvg+128)>>8)+128; | ||
| dstUV[(y/2) * dstUVStride + x*2+1] = static_cast<uint8_t>((112*rAvg-94*gAvg-18*bAvg+128)>>8)+128; | ||
| } | ||
| line += srcStride * 2; | ||
| } | ||
| } |
There was a problem hiding this comment.
Out-of-bounds write in bgr24ToNv12/bgra32ToNv12 for odd dimensions.
Two related defects in the 2x2 block loop:
- The destination
dstYonly hasheightrows, but the inner body unconditionally writesdstY[(y+1) * dstYStride + ...]. Whenheightis odd, the last iteration (y = height - 1) writes to rowheight, which is past the end ofyBuf(sizedyStride * heightin the caller). Same overrun applies todstUV[(y/2) * dstUVStride + ...]for very narrow odd heights via the inferredh2 = (h + 1) / 2allocation inwriteFrame. w2 = width / 2(vs.(width + 1) / 2used to size buffers inwriteFramelines 206-216) truncates for odd widths and silently leaves the rightmost column ofdstY/dstUVuninitialized — Apple's AVAssetWriter will then encode garbage at that column.
Gate the bottom-row writes on y + 1 < height and handle the trailing odd column (e.g., replicate the last source pixel). Same fix needed in bgra32ToNv12.
🩹 Sketch of the guard for the bottom row (apply to both helpers)
for (int x = 0; x < w2; x++) {
int b0 = line0[x*6+0], g0 = line0[x*6+1], r0 = line0[x*6+2];
int b1 = line0[x*6+3], g1 = line0[x*6+4], r1 = line0[x*6+5];
int b2 = line1[x*6+0], g2 = line1[x*6+1], r2 = line1[x*6+2];
int b3 = line1[x*6+3], g3 = line1[x*6+4], r3 = line1[x*6+5];
dstY[y * dstYStride + x*2] = static_cast<uint8_t>((66*r0+129*g0+25*b0+128)>>8)+16;
dstY[y * dstYStride + x*2+1] = static_cast<uint8_t>((66*r1+129*g1+25*b1+128)>>8)+16;
- dstY[(y+1) * dstYStride + x*2] = static_cast<uint8_t>((66*r2+129*g2+25*b2+128)>>8)+16;
- dstY[(y+1) * dstYStride + x*2+1] = static_cast<uint8_t>((66*r3+129*g3+25*b3+128)>>8)+16;
+ if (y + 1 < height) {
+ dstY[(y+1) * dstYStride + x*2] = static_cast<uint8_t>((66*r2+129*g2+25*b2+128)>>8)+16;
+ dstY[(y+1) * dstYStride + x*2+1] = static_cast<uint8_t>((66*r3+129*g3+25*b3+128)>>8)+16;
+ }
int bAvg = (b0+b1+b2+b3)/4, rAvg = (r0+r1+r2+r3)/4, gAvg = (g0+g1+g2+g3)/4;
dstUV[(y/2) * dstUVStride + x*2] = static_cast<uint8_t>((-38*rAvg-74*gAvg+112*bAvg+128)>>8)+128;
dstUV[(y/2) * dstUVStride + x*2+1] = static_cast<uint8_t>((112*rAvg-94*gAvg-18*bAvg+128)>>8)+128;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ccap_writer_apple.mm` around lines 310 - 360, bgr24ToNv12 and
bgra32ToNv12 perform unconditional writes that overflow for odd heights and
leave the rightmost column uninitialized for odd widths: change w2 = width/2 to
w2 = (width+1)/2 and in both functions guard the writes that touch the bottom
row (dstY[(y+1) * dstYStride + ...] and the corresponding dstUV indices) with a
check y+1 < height so you only write the second-row pixels when present;
additionally, handle the trailing odd column inside the inner loop (or after it)
by replicating the last source pixel (use line0/line1 samples for the final
column) so the final dstY and dstUV entries are set instead of left garbage;
apply these fixes to both bgr24ToNv12 and bgra32ToNv12 and use
dstYStride/dstUVStride offsets as already computed.
Comment on lines
+24
to
+28
| void ccap_video_writer_destroy(CcapVideoWriter* writer) { | ||
| if (writer) { | ||
| delete reinterpret_cast<ccap::VideoWriter*>(writer); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== VideoWriter API declarations =="
fd -i "ccap_writer.h|ccap_writer_imp.h" | xargs -r rg -n -C2 '\b(class VideoWriter|~VideoWriter|close\s*\(|isOpened\s*\(|actualCodec\s*\(|writeFrame\s*\()'
echo
echo "== VideoWriter method definitions (if present in reviewed sources) =="
fd -i "ccap_writer.mm|ccap_writer_apple.mm|ccap_writer_windows.cpp" | xargs -r rg -n -C2 '\b(VideoWriter::|close\s*\(|isOpened\s*\(|actualCodec\s*\(|~VideoWriter\s*\()'Repository: wysaid/CameraCapture
Length of output: 7443
🏁 Script executed:
cat -n src/ccap_writer_c.cppRepository: wysaid/CameraCapture
Length of output: 4967
🏁 Script executed:
cat -n include/ccap_writer.hRepository: wysaid/CameraCapture
Length of output: 3654
Guard all extern "C" wrappers against exceptions.
These exports call C++ methods without try/catch. Since the underlying VideoWriter methods lack noexcept guarantees, any exception escaping across the C ABI boundary is unsafe (typically process termination).
Four functions require protection:
ccap_video_writer_destroy()(lines 24–28)ccap_video_writer_close()(lines 53–57)ccap_video_writer_is_opened()(lines 59–62)ccap_video_writer_actual_codec()(lines 94–99)
Note that ccap_video_writer_create(), ccap_video_writer_open(), and ccap_video_writer_write_frame() already have proper exception handling; apply the same pattern to the remaining functions.
Proposed hardening
void ccap_video_writer_destroy(CcapVideoWriter* writer) {
- if (writer) {
- delete reinterpret_cast<ccap::VideoWriter*>(writer);
- }
+ try {
+ if (writer) {
+ delete reinterpret_cast<ccap::VideoWriter*>(writer);
+ }
+ } catch (...) {}
}
void ccap_video_writer_close(CcapVideoWriter* writer) {
- if (writer) {
- reinterpret_cast<ccap::VideoWriter*>(writer)->close();
- }
+ try {
+ if (writer) {
+ reinterpret_cast<ccap::VideoWriter*>(writer)->close();
+ }
+ } catch (...) {}
}
bool ccap_video_writer_is_opened(const CcapVideoWriter* writer) {
if (!writer) return false;
- return reinterpret_cast<const ccap::VideoWriter*>(writer)->isOpened();
+ try {
+ return reinterpret_cast<const ccap::VideoWriter*>(writer)->isOpened();
+ } catch (...) {
+ return false;
+ }
}
CcapVideoCodec ccap_video_writer_actual_codec(const CcapVideoWriter* writer) {
if (!writer) return CCAP_VIDEO_CODEC_H264;
- auto* cppWriter = reinterpret_cast<const ccap::VideoWriter*>(writer);
- return (cppWriter->actualCodec() == ccap::VideoCodec::HEVC)
- ? CCAP_VIDEO_CODEC_HEVC : CCAP_VIDEO_CODEC_H264;
+ try {
+ auto* cppWriter = reinterpret_cast<const ccap::VideoWriter*>(writer);
+ return (cppWriter->actualCodec() == ccap::VideoCodec::HEVC)
+ ? CCAP_VIDEO_CODEC_HEVC : CCAP_VIDEO_CODEC_H264;
+ } catch (...) {
+ return CCAP_VIDEO_CODEC_H264;
+ }
}📝 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
| void ccap_video_writer_destroy(CcapVideoWriter* writer) { | |
| if (writer) { | |
| delete reinterpret_cast<ccap::VideoWriter*>(writer); | |
| } | |
| } | |
| void ccap_video_writer_destroy(CcapVideoWriter* writer) { | |
| try { | |
| if (writer) { | |
| delete reinterpret_cast<ccap::VideoWriter*>(writer); | |
| } | |
| } catch (...) {} | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ccap_writer_c.cpp` around lines 24 - 28, Wrap the four exported C
functions (ccap_video_writer_destroy, ccap_video_writer_close,
ccap_video_writer_is_opened, ccap_video_writer_actual_codec) in the same
try/catch pattern used by ccap_video_writer_create/open/write to prevent C++
exceptions from escaping the C ABI: enclose the existing bodies in try { ... }
catch (const std::exception& e) { /* mirror existing handler: log the error and
return the same error/sentinel used in create/open/write */ } catch (...) { /*
mirror existing handler for unknown exceptions */ } — for destroy/close swallow
the exception after logging, for is_opened return the same false/error sentinel
used elsewhere on exception, and for actual_codec return the same
codec-sentinel/zero value used by the other functions on error so behavior is
consistent with the already-handled wrappers.
Comment on lines
+84
to
+89
| frame.timestamp = timestampNs > 0 ? timestampNs : frameInfo->timestamp; | ||
| frame.frameIndex = frameInfo->frameIndex; | ||
| frame.orientation = static_cast<ccap::FrameOrientation>(static_cast<uint32_t>(frameInfo->orientation)); | ||
|
|
||
| return cppWriter->writeFrame(frame, timestampNs); | ||
| } catch (...) { |
There was a problem hiding this comment.
Timestamp fallback is computed but not actually used in writeFrame.
Line 84 resolves a fallback timestamp into frame.timestamp, but Line 88 still passes the original timestampNs. If timestampNs == 0, fallback may be skipped depending on VideoWriter::writeFrame behavior.
Proposed fix
- frame.timestamp = timestampNs > 0 ? timestampNs : frameInfo->timestamp;
+ const uint64_t resolvedTimestamp = timestampNs > 0 ? timestampNs : frameInfo->timestamp;
+ frame.timestamp = resolvedTimestamp;
frame.frameIndex = frameInfo->frameIndex;
frame.orientation = static_cast<ccap::FrameOrientation>(static_cast<uint32_t>(frameInfo->orientation));
- return cppWriter->writeFrame(frame, timestampNs);
+ return cppWriter->writeFrame(frame, resolvedTimestamp);📝 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
| frame.timestamp = timestampNs > 0 ? timestampNs : frameInfo->timestamp; | |
| frame.frameIndex = frameInfo->frameIndex; | |
| frame.orientation = static_cast<ccap::FrameOrientation>(static_cast<uint32_t>(frameInfo->orientation)); | |
| return cppWriter->writeFrame(frame, timestampNs); | |
| } catch (...) { | |
| const uint64_t resolvedTimestamp = timestampNs > 0 ? timestampNs : frameInfo->timestamp; | |
| frame.timestamp = resolvedTimestamp; | |
| frame.frameIndex = frameInfo->frameIndex; | |
| frame.orientation = static_cast<ccap::FrameOrientation>(static_cast<uint32_t>(frameInfo->orientation)); | |
| return cppWriter->writeFrame(frame, resolvedTimestamp); | |
| } catch (...) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ccap_writer_c.cpp` around lines 84 - 89, The code computes a fallback
timestamp into frame.timestamp but still passes the original timestampNs to
cppWriter->writeFrame; change the call to pass the populated frame.timestamp
instead of timestampNs so the fallback is actually used (update the call to
cppWriter->writeFrame(frame, frame.timestamp) where frame, frameInfo,
timestampNs and cppWriter->writeFrame are referenced).
| m_config = config; | ||
|
|
||
| // Convert path to wide string | ||
| std::wstring widePath(filePath.begin(), filePath.end()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find relevant source files
git ls-files | grep -E "\.(h|hpp|mm|cpp)$" | head -20Repository: wysaid/CameraCapture
Length of output: 688
🏁 Script executed:
# Read the Windows writer file around line 52
head -70 src/ccap_writer_windows.cpp | tail -30Repository: wysaid/CameraCapture
Length of output: 970
🏁 Script executed:
# Check the header/API for documentation on filePath parameter
find . -name "*.h" -o -name "*.hpp" | xargs grep -l "filePath\|CcapWriter" 2>/dev/null | head -5Repository: wysaid/CameraCapture
Length of output: 201
🏁 Script executed:
# Check the Apple implementation for comparison
grep -n "stringWithUTF8String" src/ccap_writer_apple.mmRepository: wysaid/CameraCapture
Length of output: 159
🏁 Script executed:
# Look for public API definitions
git ls-files | grep -E "(api|interface|public|header)" | head -10Repository: wysaid/CameraCapture
Length of output: 123
🏁 Script executed:
# Read the public API header to see filePath documentation
cat -n ./include/ccap_writer.h | head -100Repository: wysaid/CameraCapture
Length of output: 3580
🏁 Script executed:
# Check if there's any documentation about path encoding
grep -r "UTF-8\|utf8\|encoding\|path" ./include/ | head -20Repository: wysaid/CameraCapture
Length of output: 2016
🏁 Script executed:
# Look at the full open method signature and comments
sed -n '40,80p' src/ccap_writer_windows.cppRepository: wysaid/CameraCapture
Length of output: 1216
🏁 Script executed:
# Check the base class/interface definition
cat -n ./include/ccap_writer.h | grep -A 10 "open"Repository: wysaid/CameraCapture
Length of output: 588
🏁 Script executed:
# Search for any existing MultiByteToWideChar usage in the codebase
rg "MultiByteToWideChar|CP_UTF8" --type cppRepository: wysaid/CameraCapture
Length of output: 1264
🏁 Script executed:
# Let's check the full context of ccap_file_reader_windows.cpp to understand the pattern
sed -n '1,50p' src/ccap_file_reader_windows.cppRepository: wysaid/CameraCapture
Length of output: 1326
🏁 Script executed:
# Verify the complete implementation pattern in file reader
grep -B 3 -A 8 "MultiByteToWideChar" src/ccap_file_reader_windows.cppRepository: wysaid/CameraCapture
Length of output: 582
🏁 Script executed:
# Check if there's any encoding documentation in C interface
grep -A 5 "filePath" ./include/ccap_writer_c.h | head -15Repository: wysaid/CameraCapture
Length of output: 471
std::wstring(filePath.begin(), filePath.end()) corrupts non-ASCII UTF-8 paths.
The range constructor performs per-byte widening from char to wchar_t. Any multi-byte UTF-8 sequence (CJK directories, accented user folders, emoji, etc.) becomes two or more separate wchar_t units instead of the correct UTF-16 code units, causing MFCreateMediaSinkForURL to fail or write to the wrong location.
This is inconsistent with the Apple implementation (which explicitly uses stringWithUTF8String) and the Windows file reader (which correctly uses MultiByteToWideChar(CP_UTF8, ...)). The API accepts UTF-8 paths per the standard C++ std::string contract.
Use MultiByteToWideChar with CP_UTF8 to match the file reader pattern:
🩹 Proposed fix
- // Convert path to wide string
- std::wstring widePath(filePath.begin(), filePath.end());
+ // Convert UTF-8 path to UTF-16 for Media Foundation.
+ std::wstring widePath;
+ if (!filePath.empty()) {
+ int len = MultiByteToWideChar(CP_UTF8, 0, filePath.data(),
+ static_cast<int>(filePath.size()),
+ nullptr, 0);
+ if (len <= 0) {
+ CCAP_LOG_E("Invalid UTF-8 file path\n");
+ return false;
+ }
+ widePath.resize(static_cast<size_t>(len));
+ MultiByteToWideChar(CP_UTF8, 0, filePath.data(),
+ static_cast<int>(filePath.size()),
+ widePath.data(), len);
+ }📝 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
| std::wstring widePath(filePath.begin(), filePath.end()); | |
| // Convert UTF-8 path to UTF-16 for Media Foundation. | |
| std::wstring widePath; | |
| if (!filePath.empty()) { | |
| int len = MultiByteToWideChar(CP_UTF8, 0, filePath.data(), | |
| static_cast<int>(filePath.size()), | |
| nullptr, 0); | |
| if (len <= 0) { | |
| CCAP_LOG_E("Invalid UTF-8 file path\n"); | |
| return false; | |
| } | |
| widePath.resize(static_cast<size_t>(len)); | |
| MultiByteToWideChar(CP_UTF8, 0, filePath.data(), | |
| static_cast<int>(filePath.size()), | |
| widePath.data(), len); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ccap_writer_windows.cpp` at line 52, The current conversion std::wstring
widePath(filePath.begin(), filePath.end()) corrupts UTF-8 multi-byte paths;
replace this per-byte widening with a proper UTF-8→UTF-16 conversion using
MultiByteToWideChar(CP_UTF8, ...) to produce widePath from filePath, allocate
the correct buffer length (call MultiByteToWideChar once to get required size,
then again to fill the buffer) and use that widePath when calling
MFCreateMediaSinkForURL (or any other Windows API expecting wchar_t*); ensure
you free or use a std::wstring sized to the returned length and handle
conversion errors.
Comment on lines
+71
to
+80
| void close() override { | ||
| if (!m_isOpened) return; | ||
| m_isOpened = false; | ||
|
|
||
| if (m_sinkWriter) { | ||
| m_sinkWriter->Finalize(); | ||
| m_sinkWriter->Release(); | ||
| m_sinkWriter = nullptr; | ||
| } | ||
| } |
There was a problem hiding this comment.
close() silently discards Finalize() failure and leaks state on early-return paths.
m_sinkWriter->Finalize()returns anHRESULTthat is currently ignored, so a failed finalize (e.g., disk full, encoder error) produces a corrupt/zero-byte output without any log. Consider logging at minimum:
- if (m_sinkWriter) {
- m_sinkWriter->Finalize();
- m_sinkWriter->Release();
- m_sinkWriter = nullptr;
- }
+ if (m_sinkWriter) {
+ HRESULT hr = m_sinkWriter->Finalize();
+ if (FAILED(hr)) {
+ CCAP_LOG_E("Finalize failed: 0x%08X\n", hr);
+ }
+ m_sinkWriter->Release();
+ m_sinkWriter = nullptr;
+ }- Symmetry nit: the macOS
close()resetsm_configandm_frameCount; this path does not. Ifopen()can be re-invoked on the same instance, stale config/frame-count from a previous session may leak in.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ccap_writer_windows.cpp` around lines 71 - 80, close() currently ignores
the HRESULT from m_sinkWriter->Finalize() and returns early without clearing
state; capture the return value (HRESULT hr = m_sinkWriter->Finalize()), check
FAILED(hr) and log a descriptive error including the HRESULT so finalize
failures (e.g., disk full) are visible; always Release() and null out
m_sinkWriter afterwards. Also ensure close() clears/zeros the instance state
(set m_isOpened = false, reset m_config and m_frameCount) even on early-return
paths so a subsequent open() won't see stale config/frame count — move/adjust
the early-return or perform the resets before returning.
Comment on lines
+278
to
+327
| bool convertToNv12(const VideoFrame& frame, | ||
| std::vector<uint8_t>& yBuf, std::vector<uint8_t>& uvBuf, | ||
| uint32_t& yStride, uint32_t& uvStride) { | ||
| int w = static_cast<int>(frame.width); | ||
| int h = static_cast<int>(frame.height); | ||
| int w2 = (w + 1) / 2; | ||
| int h2 = (h + 1) / 2; | ||
|
|
||
| yStride = static_cast<uint32_t>(w); | ||
| uvStride = static_cast<uint32_t>(w2 * 2); | ||
|
|
||
| yBuf.resize(static_cast<size_t>(yStride) * h); | ||
| uvBuf.resize(static_cast<size_t>(uvStride) * h2); | ||
|
|
||
| if (frame.pixelFormat == PixelFormat::BGR24) { | ||
| const uint8_t* src = frame.data[0]; | ||
| int srcStride = static_cast<int>(frame.stride[0]); | ||
| uint8_t* dstY = yBuf.data(); | ||
| uint8_t* dstUV = uvBuf.data(); | ||
| for (int y = 0; y < h; y += 2) { | ||
| const uint8_t* l0 = src + y * srcStride; | ||
| const uint8_t* l1 = src + (y + 1 < h ? (y + 1) * srcStride : y * srcStride); | ||
| for (int x = 0; x < w2; x++) { | ||
| int b0 = l0[x*6+0], g0 = l0[x*6+1], r0 = l0[x*6+2]; | ||
| int b1 = l0[x*6+3], g1 = l0[x*6+4], r1 = l0[x*6+5]; | ||
| int b2 = l1[x*6+0], g2 = l1[x*6+1], r2 = l1[x*6+2]; | ||
| int b3 = l1[x*6+3], g3 = l1[x*6+4], r3 = l1[x*6+5]; | ||
| dstY[y*yStride + x*2] = static_cast<uint8_t>((66*r0+129*g0+25*b0+128)>>8)+16; | ||
| dstY[y*yStride + x*2+1] = static_cast<uint8_t>((66*r1+129*g1+25*b1+128)>>8)+16; | ||
| dstY[(y+1)*yStride + x*2] = static_cast<uint8_t>((66*r2+129*g2+25*b2+128)>>8)+16; | ||
| dstY[(y+1)*yStride + x*2+1] = static_cast<uint8_t>((66*r3+129*g3+25*b3+128)>>8)+16; | ||
| int bA=(b0+b1+b2+b3)/4, rA=(r0+r1+r2+r3)/4, gA=(g0+g1+g2+g3)/4; | ||
| dstUV[(y/2)*uvStride + x*2] = static_cast<uint8_t>((-38*rA-74*gA+112*bA+128)>>8)+128; | ||
| dstUV[(y/2)*uvStride + x*2+1] = static_cast<uint8_t>((112*rA-94*gA-18*bA+128)>>8)+128; | ||
| } | ||
| } | ||
| } else if (frame.pixelFormat == PixelFormat::NV12 || frame.pixelFormat == PixelFormat::NV12f) { | ||
| for (int y = 0; y < h; y++) { | ||
| memcpy(yBuf.data() + y * yStride, frame.data[0] + y * frame.stride[0], static_cast<size_t>(w)); | ||
| } | ||
| for (int y = 0; y < h2; y++) { | ||
| memcpy(uvBuf.data() + y * uvStride, frame.data[1] + y * frame.stride[1], static_cast<size_t>(w2) * 2); | ||
| } | ||
| } else { | ||
| CCAP_LOG_E("Unsupported pixel format for writer on Windows: %d\n", | ||
| static_cast<int>(frame.pixelFormat)); | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
convertToNv12 is missing BGRA32 / I420 / I420f support and has the same odd-dimension OOB as the macOS path.
Two issues that should be fixed together:
-
Cross-platform format gap. The macOS
writeFrameacceptsBGR24,BGRA32,I420,I420f,NV12,NV12f, but this routine only handlesBGR24andNV12/NV12f. ABGRA32frame (the most common DirectShow/MF capture output on Windows) will hit theUnsupported pixel formatbranch and the entire writer will fail. The public contract advertised in the PR description ("Supports input pixel formats BGR24, BGRA32, I420, I420f, NV12, NV12f") is not satisfied on Windows. -
OOB on odd dimensions (matches the Apple defect).
w2 = (w + 1) / 2combined withl0[x*6+5]causes an out-of-bounds read for odd widths (the last 2×2 block straddles past the row).- The body always writes
dstY[(y+1)*yStride + ...]without checkingy + 1 < h, so odd heights write pastyBuf.
For #1 you can lift the I420 and BGRA32 conversion blocks directly from the Apple implementation. For #2 apply the same guard discussed on the Apple file.
🩹 Sketch for the odd-dimension guard and BGRA32 branch
for (int y = 0; y < h; y += 2) {
const uint8_t* l0 = src + y * srcStride;
const uint8_t* l1 = src + (y + 1 < h ? (y + 1) * srcStride : y * srcStride);
for (int x = 0; x < w2; x++) {
- int b0 = l0[x*6+0], g0 = l0[x*6+1], r0 = l0[x*6+2];
- int b1 = l0[x*6+3], g1 = l0[x*6+4], r1 = l0[x*6+5];
- int b2 = l1[x*6+0], g2 = l1[x*6+1], r2 = l1[x*6+2];
- int b3 = l1[x*6+3], g3 = l1[x*6+4], r3 = l1[x*6+5];
+ const bool hasX1 = (x*2 + 1) < w;
+ int b0 = l0[x*6+0], g0 = l0[x*6+1], r0 = l0[x*6+2];
+ int b1 = hasX1 ? l0[x*6+3] : b0, g1 = hasX1 ? l0[x*6+4] : g0, r1 = hasX1 ? l0[x*6+5] : r0;
+ int b2 = l1[x*6+0], g2 = l1[x*6+1], r2 = l1[x*6+2];
+ int b3 = hasX1 ? l1[x*6+3] : b2, g3 = hasX1 ? l1[x*6+4] : g2, r3 = hasX1 ? l1[x*6+5] : r2;
dstY[y*yStride + x*2] = static_cast<uint8_t>((66*r0+129*g0+25*b0+128)>>8)+16;
- dstY[y*yStride + x*2+1] = static_cast<uint8_t>((66*r1+129*g1+25*b1+128)>>8)+16;
- dstY[(y+1)*yStride + x*2] = static_cast<uint8_t>((66*r2+129*g2+25*b2+128)>>8)+16;
- dstY[(y+1)*yStride + x*2+1] = static_cast<uint8_t>((66*r3+129*g3+25*b3+128)>>8)+16;
+ if (hasX1) dstY[y*yStride + x*2+1] = static_cast<uint8_t>((66*r1+129*g1+25*b1+128)>>8)+16;
+ if (y + 1 < h) {
+ dstY[(y+1)*yStride + x*2] = static_cast<uint8_t>((66*r2+129*g2+25*b2+128)>>8)+16;
+ if (hasX1) dstY[(y+1)*yStride + x*2+1] = static_cast<uint8_t>((66*r3+129*g3+25*b3+128)>>8)+16;
+ }
int bA=(b0+b1+b2+b3)/4, rA=(r0+r1+r2+r3)/4, gA=(g0+g1+g2+g3)/4;
dstUV[(y/2)*uvStride + x*2] = static_cast<uint8_t>((-38*rA-74*gA+112*bA+128)>>8)+128;
dstUV[(y/2)*uvStride + x*2+1] = static_cast<uint8_t>((112*rA-94*gA-18*bA+128)>>8)+128;
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ccap_writer_windows.cpp` around lines 278 - 327, convertToNv12 currently
handles only BGR24 and NV12/NV12f and also reads/writes out-of-bounds on odd
widths/heights; add BGRA32 and I420/I420f conversion branches (reuse the
conversion logic from the macOS writeFrame for BGRA32 and I420/I420f) so the
function supports the same formats as the public API, and fix the odd-dimension
OOB by clamping the per-2x2 loop so you never read past the row end (compute a
safe right pixel count instead of assuming x*6 bytes exist) and only write the
lower-row Y/UV when (y+1) < h; reference symbols: convertToNv12, w2, h2, l0/l1,
dstY/dstUV, PixelFormat::BGRA32, PixelFormat::I420, PixelFormat::I420f,
PixelFormat::NV12f.
Comment on lines
+75
to
+78
| bool VideoWriter::writeFrame(const VideoFrame& frame, uint64_t timestampNs) { | ||
| if (!m_impl) return false; | ||
| return impl(m_impl)->writeFrame(frame, timestampNs); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Report error when writeFrame is called on unavailable writer.
Line 76 silently returns false when m_impl is null, but line 61 in open() calls reportError() for the same condition. This inconsistency makes debugging harder for users.
🔧 Proposed fix for consistent error reporting
bool VideoWriter::writeFrame(const VideoFrame& frame, uint64_t timestampNs) {
- if (!m_impl) return false;
+ if (!m_impl) {
+ reportError(ErrorCode::WriterNotOpened, "VideoWriter not available on this platform");
+ return false;
+ }
return impl(m_impl)->writeFrame(frame, timestampNs);
}Also apply the same pattern to the stub implementation on line 113:
-bool VideoWriter::writeFrame(const VideoFrame&, uint64_t) { return false; }
+bool VideoWriter::writeFrame(const VideoFrame&, uint64_t) {
+ reportError(ErrorCode::WriterNotOpened, "Video writer not enabled in this build");
+ return false;
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ccap_writer.mm` around lines 75 - 78, The writeFrame path silently
returns false when m_impl is null; update VideoWriter::writeFrame to mirror
open() by calling reportError(...) with a clear message when m_impl is nullptr
before returning false, and then proceed to call impl(m_impl)->writeFrame(...)
when available; also apply the same pattern to the stub implementation of
writeFrame (the fallback/stub class method) so it reports the same error via
reportError(...) instead of failing silently.
There was a problem hiding this comment.
Pull request overview
This PR introduces a cross-platform (macOS + Windows) video writing feature to ccap, adding both a C++ ccap::VideoWriter API and a pure C wrapper, plus new build options, error codes, and unit tests to validate basic encoding/output behavior.
Changes:
- Added
ccap::VideoWriterC++ API andccap_video_writer_*C API for encoding frames into MP4/MOV. - Implemented platform backends using AVFoundation (macOS) and Media Foundation sink writer (Windows).
- Integrated a new
CCAP_ENABLE_VIDEO_WRITERCMake option, extended error codes, and addedccap_video_writer_test.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
CMakeLists.txt |
Adds CCAP_ENABLE_VIDEO_WRITER option and wires writer sources/defines into the library build. |
include/ccap_writer.h |
Introduces the public C++ VideoWriter API and configuration types. |
include/ccap_writer_c.h |
Adds the public pure-C ccap_video_writer_* API surface. |
include/ccap_def.h |
Adds writer-related ErrorCode values. |
include/ccap_c.h |
Adds C error code equivalents for writer errors. |
src/ccap_writer.mm |
Implements the VideoWriter dispatch layer and stubs when the feature is disabled. |
src/ccap_writer_imp.h |
Defines the internal VideoWriter::Impl interface for platform backends. |
src/ccap_writer_apple.mm |
macOS AVFoundation backend implementation (NV12 conversion + AVAssetWriter). |
src/ccap_writer_windows.cpp |
Windows Media Foundation backend implementation (NV12 conversion + sink writer). |
src/ccap_writer_c.cpp |
Implements the C wrapper over the C++ VideoWriter. |
src/ccap_utils.cpp |
Extends errorCodeToString() with writer-specific error strings. |
src/ccap_c.cpp |
Adds static_asserts ensuring C/C++ writer error codes remain aligned. |
tests/test_video_writer.cpp |
Adds 15 unit tests for writer lifecycle, encoding, fallback behavior, and C API. |
tests/CMakeLists.txt |
Adds the ccap_video_writer_test target and CTest discovery gating. |
Comments suppressed due to low confidence (12)
src/ccap_writer_windows.cpp:196
MFCreateMediaSinkForURLis called with 3 parameters here, but the Media Foundation API expects an output type/MIME type parameter as well (or alternatively you can useMFCreateSinkWriterFromURL). As written this is very likely a compile-time error on Windows SDK headers. Please adjust the call to the correct API/signature.
IMFMediaSink* pSink = nullptr;
hr = MFCreateMediaSinkForURL(filePath.c_str(), pAttributes, &pSink);
if (FAILED(hr)) {
CCAP_LOG_E("MFCreateMediaSinkForURL failed: 0x%08X\n", hr);
pAttributes->Release();
if (pEncodingAttributes) pEncodingAttributes->Release();
return false;
src/ccap_writer_windows.cpp:64
open()always tries HEVC first and may select HEVC even whenconfig.codecis explicitly set to H.264. That makes theWriterConfig::codecsetting ineffective (it should at least try the requested codec first, with fallback to the other codec only if creation fails).
// Try HEVC first, fallback to H.264
GUID videoCodec = MFVideoFormat_HEVC;
m_actualCodec = VideoCodec::HEVC;
if (!tryCreateWriter(widePath, videoCodec, config)) {
videoCodec = MFVideoFormat_H264;
m_actualCodec = VideoCodec::H264;
if (!tryCreateWriter(widePath, videoCodec, config)) {
CCAP_LOG_E("Failed to create video writer with H.264 or HEVC\n");
return false;
}
}
src/ccap_writer_windows.cpp:53
- Windows implementation never uses
config.containerto control the output container type; it relies entirely on the path passed to Media Foundation. This meansWriterConfig::container = MOVmay be silently ignored or fail depending on the file extension/OS support. Please either enforce/validate supported containers on Windows (and reportUnsupportedVideoFormat/writer-specific error) or implement container selection explicitly so the API behaves consistently with macOS.
bool open(std::string_view filePath, const WriterConfig& config) override {
m_config = config;
// Convert path to wide string
std::wstring widePath(filePath.begin(), filePath.end());
src/ccap_writer_windows.cpp:53
std::wstring widePath(filePath.begin(), filePath.end())is not a valid UTF-8/ACP→UTF-16 conversion; it will corrupt non-ASCII paths on Windows. Use a proper conversion (e.g.,MultiByteToWideCharwith a defined codepage) or take astd::filesystem::pathand use.wstring()to preserve Unicode paths.
// Convert path to wide string
std::wstring widePath(filePath.begin(), filePath.end());
src/ccap_writer_windows.cpp:220
tryCreateWriter()ignores the HRESULTs from most media type attribute setters (e.g.,MFSetAttributeRatio,MFSetAttributeSize, and the variousSet*calls). This can makeopen()succeed even with invalid config (e.g., width/height == 0) and then fail later in harder-to-diagnose ways. Please validateconfigup-front and check/propagate failures from these attribute calls (ideally viareportError(ErrorCode::WriterOpenFailed, ...)).
pOutputType->SetGUID(MF_MT_MAJOR_TYPE, MFMediaType_Video);
pOutputType->SetGUID(MF_MT_SUBTYPE, videoCodec);
pOutputType->SetUINT32(MF_MT_AVG_BITRATE, static_cast<UINT32>(config.bitRate > 0 ? config.bitRate : config.width * config.height * 4));
pOutputType->SetUINT32(MF_MT_INTERLACE_MODE, MFVideoInterlace_Progressive);
// Set frame rate
UINT32 fpsNum = static_cast<UINT32>(config.frameRate * 1000);
UINT32 fpsDen = 1000;
if (config.frameRate <= 0) { fpsNum = 30000; fpsDen = 1000; }
MFSetAttributeRatio(pOutputType, MF_MT_FRAME_RATE, fpsNum, fpsDen);
MFSetAttributeSize(pOutputType, MF_MT_FRAME_SIZE, config.width, config.height);
src/ccap_writer_windows.cpp:325
convertToNv12()only supportsBGR24andNV12/NV12f, but the PR description and public API imply support forBGRA32,I420/I420f, etc. Either implement the missing conversions on Windows (or reuse an existing conversion library if available) or explicitly document/validate supported pixel formats and return a clearUnsupportedPixelFormat/writer error.
if (frame.pixelFormat == PixelFormat::BGR24) {
const uint8_t* src = frame.data[0];
int srcStride = static_cast<int>(frame.stride[0]);
uint8_t* dstY = yBuf.data();
uint8_t* dstUV = uvBuf.data();
for (int y = 0; y < h; y += 2) {
const uint8_t* l0 = src + y * srcStride;
const uint8_t* l1 = src + (y + 1 < h ? (y + 1) * srcStride : y * srcStride);
for (int x = 0; x < w2; x++) {
int b0 = l0[x*6+0], g0 = l0[x*6+1], r0 = l0[x*6+2];
int b1 = l0[x*6+3], g1 = l0[x*6+4], r1 = l0[x*6+5];
int b2 = l1[x*6+0], g2 = l1[x*6+1], r2 = l1[x*6+2];
int b3 = l1[x*6+3], g3 = l1[x*6+4], r3 = l1[x*6+5];
dstY[y*yStride + x*2] = static_cast<uint8_t>((66*r0+129*g0+25*b0+128)>>8)+16;
dstY[y*yStride + x*2+1] = static_cast<uint8_t>((66*r1+129*g1+25*b1+128)>>8)+16;
dstY[(y+1)*yStride + x*2] = static_cast<uint8_t>((66*r2+129*g2+25*b2+128)>>8)+16;
dstY[(y+1)*yStride + x*2+1] = static_cast<uint8_t>((66*r3+129*g3+25*b3+128)>>8)+16;
int bA=(b0+b1+b2+b3)/4, rA=(r0+r1+r2+r3)/4, gA=(g0+g1+g2+g3)/4;
dstUV[(y/2)*uvStride + x*2] = static_cast<uint8_t>((-38*rA-74*gA+112*bA+128)>>8)+128;
dstUV[(y/2)*uvStride + x*2+1] = static_cast<uint8_t>((112*rA-94*gA-18*bA+128)>>8)+128;
}
}
} else if (frame.pixelFormat == PixelFormat::NV12 || frame.pixelFormat == PixelFormat::NV12f) {
for (int y = 0; y < h; y++) {
memcpy(yBuf.data() + y * yStride, frame.data[0] + y * frame.stride[0], static_cast<size_t>(w));
}
for (int y = 0; y < h2; y++) {
memcpy(uvBuf.data() + y * uvStride, frame.data[1] + y * frame.stride[1], static_cast<size_t>(w2) * 2);
}
} else {
CCAP_LOG_E("Unsupported pixel format for writer on Windows: %d\n",
static_cast<int>(frame.pixelFormat));
return false;
}
src/ccap_writer_windows.cpp:309
- The BGR24→NV12 conversion writes
dstY[(y+1)*yStride + ...]for everyy += 2iteration. Whenhis odd andy == h-1, this writes past the end ofyBuf(OOB). Additionally,w2 = (w+1)/2combined with reading two pixels per loop (x*6+...) can read past the end of the source row whenwis odd. Please either reject odd dimensions up-front (NV12 typically requires even width/height) or handle the last row/column safely.
for (int y = 0; y < h; y += 2) {
const uint8_t* l0 = src + y * srcStride;
const uint8_t* l1 = src + (y + 1 < h ? (y + 1) * srcStride : y * srcStride);
for (int x = 0; x < w2; x++) {
int b0 = l0[x*6+0], g0 = l0[x*6+1], r0 = l0[x*6+2];
int b1 = l0[x*6+3], g1 = l0[x*6+4], r1 = l0[x*6+5];
int b2 = l1[x*6+0], g2 = l1[x*6+1], r2 = l1[x*6+2];
int b3 = l1[x*6+3], g3 = l1[x*6+4], r3 = l1[x*6+5];
dstY[y*yStride + x*2] = static_cast<uint8_t>((66*r0+129*g0+25*b0+128)>>8)+16;
dstY[y*yStride + x*2+1] = static_cast<uint8_t>((66*r1+129*g1+25*b1+128)>>8)+16;
dstY[(y+1)*yStride + x*2] = static_cast<uint8_t>((66*r2+129*g2+25*b2+128)>>8)+16;
dstY[(y+1)*yStride + x*2+1] = static_cast<uint8_t>((66*r3+129*g3+25*b3+128)>>8)+16;
int bA=(b0+b1+b2+b3)/4, rA=(r0+r1+r2+r3)/4, gA=(g0+g1+g2+g3)/4;
src/ccap_writer_windows.cpp:63
- On failure paths the Windows writer only logs via
CCAP_LOG_Eand returnsfalse; it never callsreportError()with the newly added writer-relatedErrorCodes. This means applications using the global error callback won't get actionable error codes/descriptions for writer failures. Please propagate failures viareportError(ErrorCode::WriterOpenFailed/WriterWriteFailed/WriterCloseFailed, ...)similarly to the existing file reader implementations.
if (!tryCreateWriter(widePath, videoCodec, config)) {
CCAP_LOG_E("Failed to create video writer with H.264 or HEVC\n");
return false;
}
src/ccap_writer_apple.mm:327
- Both
bgr24ToNv12andbgra32ToNv12write todstY[(y+1) * dstYStride + ...]for everyy += 2iteration. Ifheightis odd andy == height-1, this writes past the end of the destination buffer (OOB). Please either reject odd dimensions up-front (NV12 typically requires even width/height) or add bounds handling for the last row/column.
int w2 = width / 2;
const uint8_t* line = src;
for (int y = 0; y < height; y += 2) {
const uint8_t* line0 = line;
const uint8_t* line1 = (y + 1 < height) ? line + srcStride : line;
for (int x = 0; x < w2; x++) {
int b0 = line0[x*6+0], g0 = line0[x*6+1], r0 = line0[x*6+2];
int b1 = line0[x*6+3], g1 = line0[x*6+4], r1 = line0[x*6+5];
int b2 = line1[x*6+0], g2 = line1[x*6+1], r2 = line1[x*6+2];
int b3 = line1[x*6+3], g3 = line1[x*6+4], r3 = line1[x*6+5];
dstY[y * dstYStride + x*2] = static_cast<uint8_t>((66*r0+129*g0+25*b0+128)>>8)+16;
dstY[y * dstYStride + x*2+1] = static_cast<uint8_t>((66*r1+129*g1+25*b1+128)>>8)+16;
dstY[(y+1) * dstYStride + x*2] = static_cast<uint8_t>((66*r2+129*g2+25*b2+128)>>8)+16;
dstY[(y+1) * dstYStride + x*2+1] = static_cast<uint8_t>((66*r3+129*g3+25*b3+128)>>8)+16;
tests/test_video_writer.cpp:84
- Same as above:
directory_iterator/fs::removemay throw fromTearDown(). Usingstd::error_codeoverloads would avoid cleanup-related crashes and reduce flakiness.
void TearDown() override {
for (const auto& entry : fs::directory_iterator(fs::temp_directory_path())) {
std::string filename = entry.path().filename().string();
if (filename.find("ccap_writer_test_") == 0) {
fs::remove(entry.path());
}
}
src/ccap_writer_windows.cpp:80
VideoWriter::width()/height()/frameRate()read backImpl::m_configeven afterclose(). The macOS implementation clearsm_configon close, but the Windowsclose()does not, sowriter.width()etc will keep returning the previous values after close (and theGetPropertiesAfterOpentest expects them to be 0). Please resetm_config(and potentiallym_actualCodec) in the Windowsclose()implementation for consistent behavior.
void close() override {
if (!m_isOpened) return;
m_isOpened = false;
if (m_sinkWriter) {
m_sinkWriter->Finalize();
m_sinkWriter->Release();
m_sinkWriter = nullptr;
}
}
src/ccap_writer_windows.cpp:93
writeFrame()usesframe.width/heightto size/convert the input, but the sink writer was configured withm_config.width/heightinopen(). There is currently no validation that the incoming frame dimensions match the configured output size, which can lead to invalid samples or encoder failures. Please validateframe.width/heightagainstm_config(and return/reportWriterWriteFailedon mismatch).
bool writeFrame(const VideoFrame& frame, uint64_t timestampNs) override {
if (!m_isOpened || !m_sinkWriter) return false;
int w = static_cast<int>(frame.width);
int h = static_cast<int>(frame.height);
int w2 = (w + 1) / 2;
int h2 = (h + 1) / 2;
Comment on lines
+138
to
+146
| # Exclude writer sources from main glob to avoid double-compilation (platform impl included via #include) | ||
| list(FILTER LIB_SOURCE EXCLUDE REGEX ".*ccap_writer_apple.*") | ||
| list(FILTER LIB_SOURCE EXCLUDE REGEX ".*ccap_writer_windows.*") | ||
| list(FILTER LIB_SOURCE EXCLUDE REGEX ".*ccap_writer_c\..*$") | ||
| list(FILTER LIB_SOURCE EXCLUDE REGEX ".*ccap_writer\..*$") | ||
| list(APPEND LIB_SOURCE | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/src/ccap_writer.mm | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/src/ccap_writer_c.cpp | ||
| ) |
Comment on lines
+36
to
+39
| WriterWindows() : m_sinkWriter(nullptr), m_streamIndex(0), m_mfInitialized(false) { | ||
| HRESULT hr = MFStartup(MF_VERSION, MFSTARTUP_FULL); | ||
| m_mfInitialized = SUCCEEDED(hr); | ||
| } |
Comment on lines
+47
to
+56
| // Try HEVC first, fallback to H.264 | ||
| AVVideoCodecType codecs[] = { AVVideoCodecTypeHEVC, AVVideoCodecTypeH264 }; | ||
| VideoCodec cppCodecs[] = { VideoCodec::HEVC, VideoCodec::H264 }; | ||
|
|
||
| for (int i = 0; i < 2; i++) { | ||
| if (tryOpen(filePath, fileType, pathStr, codecs[i])) { | ||
| m_actualCodec = cppCodecs[i]; | ||
| return true; | ||
| } | ||
| } |
Comment on lines
+194
to
+198
| // Wait for writer input to be ready (with 2 second timeout) | ||
| int waitMs = 0; | ||
| while (![m_writerInput isReadyForMoreMediaData]) { | ||
| usleep(1000); // 1ms | ||
| if (++waitMs > 2000) { |
Comment on lines
+279
to
+286
| // Calculate timestamp | ||
| CMTime presentationTime; | ||
| if (timestampNs > 0) { | ||
| presentationTime = CMTimeMake(static_cast<int64_t>(timestampNs), 1000000000); | ||
| } else { | ||
| double fps = m_config.frameRate > 0 ? m_config.frameRate : 30.0; | ||
| presentationTime = CMTimeMake(static_cast<int64_t>(m_frameCount), static_cast<int32_t>(fps)); | ||
| } |
Comment on lines
+232
to
+238
| ccap::VideoFrame frame; | ||
| frame.data[0] = frameData.data(); | ||
| frame.stride[0] = static_cast<uint32_t>(stride); | ||
| frame.pixelFormat = ccap::PixelFormat::BGR24; | ||
| frame.width = static_cast<uint32_t>(w); | ||
| frame.height = static_cast<uint32_t>(h); | ||
| frame.sizeInBytes = static_cast<uint32_t>(stride * h); |
Comment on lines
+58
to
+65
| void TearDown() override { | ||
| // Clean up any test output files | ||
| for (const auto& entry : fs::directory_iterator(fs::temp_directory_path())) { | ||
| std::string filename = entry.path().filename().string(); | ||
| if (filename.find("ccap_writer_test_") == 0) { | ||
| fs::remove(entry.path()); | ||
| } | ||
| } |
Comment on lines
+48
to
+51
| uint32_t width = 0; ///< Frame width in pixels | ||
| uint32_t height = 0; ///< Frame height in pixels | ||
| double frameRate = 30.0; ///< Target frame rate; 0 = variable rate | ||
| uint64_t bitRate = 5'000'000; ///< Target bit rate in bits/s; 0 = auto |
Comment on lines
+19
to
+25
| #ifdef CCAP_ENABLE_VIDEO_WRITER | ||
|
|
||
| #if __APPLE__ | ||
| #include "ccap_writer_apple.mm" | ||
| #elif defined(_WIN32) || defined(_MSC_VER) | ||
| #include "ccap_writer_windows.cpp" | ||
| #endif |
Comment on lines
+204
to
+208
| int w = static_cast<int>(frame.width); | ||
| int h = static_cast<int>(frame.height); | ||
| int w2 = (w + 1) / 2; | ||
| int h2 = (h + 1) / 2; | ||
|
|
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
VideoWriterclass to encode frames into MP4/MOV video files using system frameworks onlyAVAssetWriter+AVAssetWriterInputPixelBufferAdaptor(HEVC with H.264 fallback)IMFSinkWritervia Media Foundation (same codec strategy)ccap_video_writer_*)CCAP_ENABLE_VIDEO_WRITER(ON by default)Test plan
ccap_video_writer_testpasses all 15 tests on macOSProvider.open()CCAP_ENABLE_VIDEO_WRITER=OFFcompiles cleanly with stub implementations🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests