feat: add video file writing support for macOS and Windows#55
Open
wysaid wants to merge 13 commits 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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds cross-platform video writing: C++ and C public APIs, internal Impl interface with NV12 conversion helpers, macOS (AVAssetWriter) and Windows (Media Foundation) implementations, CMake/test/CLI/example integration, and comprehensive tests. ChangesVideo Writer Feature Implementation
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Lib as ccap::VideoWriter
participant Impl as VideoWriter::Impl (WriterWindows/WriterApple)
participant OS as IMFSinkWriter / AVAssetWriter
App->>Lib: open(filePath, WriterConfig)
Lib->>Impl: open(filePath, WriterConfig)
Impl->>OS: create/configure writer (BeginWriting / start session)
App->>Lib: writeFrame(frame, timestampNs)
Lib->>Impl: writeFrame(frame, timestampNs)
Impl->>OS: append sample / append pixel buffer
App->>Lib: close()
Lib->>Impl: close()
Impl->>OS: finalize/finishWriting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 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 |
![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
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;
… comments Major changes: - Replace ccap_writer.mm with ccap_writer.cpp (pure C++ dispatch layer) using factory function pattern instead of #include of .mm/.cpp files - Compile platform implementations as separate source files (Apple-only .mm, Windows-only .cpp) matching the existing file reader pattern - Rewrite Windows implementation to use MFCreateSinkWriterFromURL (correct API) instead of non-existent MFCreateMediaSinkForURL - Guard #pragma comment(lib) behind _MSC_VER for MinGW compatibility Review comment fixes: - Respect WriterConfig::codec preference (try requested codec first) - Replace usleep() with std::this_thread::sleep_for (portability) - Use high-precision timescale (600000) for CMTime instead of truncating to integer fps - Add reportError() calls throughout (not just CCAP_LOG_E) - Validate frame dimensions match configured output in writeFrame() - Require even dimensions in open() for NV12 encoding - Check m_mfInitialized in Windows open() before calling MF APIs - Fix C API timestamp: pass resolved timestamp to writeFrame() - Share NV12 conversion code via inline helpers in ccap_writer_imp.h - Support I420/BGRA32 conversion on Windows (was BGR24/NV12 only) - Guard playback validation in tests behind CCAP_ENABLE_FILE_PLAYBACK - Zero-initialize VideoFrame structs in tests - Use std::error_code overloads in test TearDown() for robustness - Update frameRate docs to reflect actual behavior (default 30fps) - Set sample duration on Windows for better seek/playback behavior - Run format_all.sh Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/ccap_writer.cpp (2)
54-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReport error when writeFrame is called on unavailable writer.
Line 55 silently returns
falsewhenm_implis null, but line 40 inopen()callsreportError()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); }🤖 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.cpp` around lines 54 - 57, The writeFrame method currently returns false silently when m_impl is null; make it mirror open() by calling reportError(...) before returning so callers get a consistent error message. In VideoWriter::writeFrame, check m_impl as now but if null call reportError with a clear message (e.g. indicating the writer is unavailable/not opened) and then return false; keep the subsequent call to impl(m_impl)->writeFrame unchanged when m_impl is present.
92-92: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReport error when writeFrame is called in disabled build.
The stub
writeFrameon line 92 silently returnsfalse, but the stubopen()on line 87 callsreportError()to inform users why the feature is unavailable. Apply the same pattern here for consistency.🔧 Proposed fix for consistent error reporting
-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.cpp` at line 92, The stub VideoWriter::writeFrame currently returns false silently; change it to call the same error reporting used in open() by invoking reportError() with a clear message indicating the feature is unavailable in this build before returning false. Locate VideoWriter::writeFrame and add a reportError(...) call (matching the message/format used in VideoWriter::open or the existing reportError usage) so callers receive the same diagnostic feedback as when open() fails.
🤖 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 `@include/ccap_writer_c.h`:
- Around line 42-50: The struct CcapWriterConfig lists frameRate as "default
30fps" but C has no defaults — add an explicit initializer or normalization:
introduce a CCAP_WRITER_CONFIG_INIT macro (or a helper function) that sets
codec, container, width, height, frameRate = 30.0, bitRate = 0, and update call
sites to use it; alternatively, document and enforce normalization in
ccap_video_writer_open() by treating frameRate==0 as 30.0 and add a comment
referencing CcapWriterConfig and frameRate so callers know the canonical
behavior.
- Around line 100-105: The API currently forces
ccap_video_writer_actual_codec(const CcapVideoWriter* writer) to return a
concrete CcapVideoCodec even when the writer is unopened; update the public
contract by adding an explicit UNKNOWN (or NONE) value to the CcapVideoCodec
enum and return that for unopened/failed writers, and/or document that
ccap_video_writer_actual_codec is only valid after ccap_video_writer_open()
succeeds; implement the enum change in the header where CcapVideoCodec is
defined, update the function's return behavior to return CcapVideoCodec::UNKNOWN
for uninitialized writers, and update the function comment to state the new
behavior.
In `@src/ccap_writer_imp.h`:
- Around line 69-118: The convertFrameToNv12 function must validate that
frame.width and frame.height are even before attempting NV12 conversion; add an
early check in convertFrameToNv12 (using symbols frame.width/frame.height and
function convertFrameToNv12) that returns false (or otherwise fails fast) if (w
% 2 != 0 || h % 2 != 0) or if dimensions are less than 2, to avoid out-of-bounds
access in paths that compute half-width/half-height and call bgrToNv12; keep
this check before computing yBuf/uvBuf sizes or copying any plane data.
---
Duplicate comments:
In `@src/ccap_writer.cpp`:
- Around line 54-57: The writeFrame method currently returns false silently when
m_impl is null; make it mirror open() by calling reportError(...) before
returning so callers get a consistent error message. In VideoWriter::writeFrame,
check m_impl as now but if null call reportError with a clear message (e.g.
indicating the writer is unavailable/not opened) and then return false; keep the
subsequent call to impl(m_impl)->writeFrame unchanged when m_impl is present.
- Line 92: The stub VideoWriter::writeFrame currently returns false silently;
change it to call the same error reporting used in open() by invoking
reportError() with a clear message indicating the feature is unavailable in this
build before returning false. Locate VideoWriter::writeFrame and add a
reportError(...) call (matching the message/format used in VideoWriter::open or
the existing reportError usage) so callers receive the same diagnostic feedback
as when open() fails.
🪄 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: 7f6f425b-7401-4867-bac5-41fb1e4bbdf2
📒 Files selected for processing (12)
CMakeLists.txtinclude/ccap_writer.hinclude/ccap_writer_c.hsrc/ccap_imp_linux.hsrc/ccap_imp_windows.cppsrc/ccap_imp_windows.hsrc/ccap_writer.cppsrc/ccap_writer_apple.mmsrc/ccap_writer_c.cppsrc/ccap_writer_imp.hsrc/ccap_writer_windows.cpptests/test_video_writer.cpp
✅ Files skipped from review due to trivial changes (3)
- src/ccap_imp_linux.h
- src/ccap_imp_windows.h
- src/ccap_imp_windows.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- CMakeLists.txt
- include/ccap_writer.h
- src/ccap_writer_c.cpp
- tests/test_video_writer.cpp
MinGW's codecapi.h has UUID template issues that cause compilation errors. The header was unused - no codec API types are needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- examples/desktop/6-record_video.cpp: new demo that opens camera and records ~5 seconds of frames to an MP4 file using ccap::VideoWriter. Guarded with #ifdef CCAP_ENABLE_VIDEO_WRITER for non-supported platforms. - cli: add --record <file> option to record camera frames to a video file. Integrated into captureFrames() guarded by #ifdef CCAP_ENABLE_VIDEO_WRITER. Warns if used without -c/--count or --timeout (would run indefinitely). Warns if used with --video mode (not supported). - .vscode/tasks.json: add 'Run ccap CLI --record camera (Debug/Release)' tasks that record 5 seconds from the default camera to camera_capture.mp4. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…e pause Camera hardware PTS can have large startup gaps, causing the first frame to display for ~1 second. Use std::chrono::steady_clock wall-clock time instead of camera timestamps to generate PTS, ensuring frame intervals reflect real grab timing. Also add transcode duration verification tests. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tests/test_video_writer.cpp (1)
393-401: ⚡ Quick winConsider making root-reach handling explicit in
findTestVideo.The current code is safe on standard-compliant C++ filesystem implementations—
has_parent_path()returns false at the filesystem root, so the loop terminates normally. However, adding an explicit guard improves code clarity and makes the fallback behavior obvious. If sentinel files aren't found by the root, return an empty path instead of a path to a non-existent file.Suggested enhancement
static fs::path findTestVideo() { fs::path projectRoot = fs::current_path(); - while (projectRoot.has_parent_path()) { + while (true) { if (fs::exists(projectRoot / "CMakeLists.txt") && fs::exists(projectRoot / "tests")) { break; } - projectRoot = projectRoot.parent_path(); + fs::path parent = projectRoot.parent_path(); + if (parent.empty() || parent == projectRoot) { + return {}; + } + projectRoot = parent; } return projectRoot / "tests" / "test-data" / "test.mp4"; }🤖 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 393 - 401, findTestVideo should explicitly detect when the repository root wasn't found and return an empty path instead of constructing a non-existent test file path; modify the loop in findTestVideo to break when you reach filesystem root (use !projectRoot.has_parent_path() or compare parent_path()==projectRoot) and after the loop check whether the sentinel files were found — if not, return an empty fs::path() rather than projectRoot / "tests" / "test-data" / "test.mp4" to make the fallback explicit and avoid returning a bogus path.
🤖 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 `@cli/args_parser.cpp`:
- Around line 402-405: When handling the "--record" argument in args_parser.cpp,
validate that a non-flag value follows and refuse/emit an error if the next
token is missing or starts with '-' (so "--record --timeout 5" is not accepted);
implement this check around the existing branch that assigns
opts.recordVideoPath and increment i only when argv[i+1] exists and does not
begin with '-'. Also guard the record-path behavior with
CCAP_ENABLE_VIDEO_WRITER: if that macro is not defined, emit a clear error/usage
message (or fail) instead of silently accepting the flag; update the branch that
sets opts.recordVideoPath to perform the macro check and error handling
accordingly.
In `@examples/desktop/6-record_video.cpp`:
- Line 53: The condition dereferences exeDir[0] without checking that exeDir
(from argv[0]) is non-empty; update the if to guard against an empty argv[0] by
checking exeDir.empty() (or !exeDir.empty()) before accessing exeDir[0] — e.g.,
change the condition to test exeDir.empty() (or combine with the existing check)
so the expression becomes safe when argv[0] is an empty string while keeping the
existing find_last_of logic for exeDir.
In `@src/ccap_writer_windows.cpp`:
- Around line 104-107: The log prints HRESULT using std::to_string(hr) but
prepends "0x", producing misleading decimal output; change the call in the
m_sinkWriter->Finalize() error path (where hr is declared and
reportError(ErrorCode::WriterCloseFailed, "IMFSinkWriter::Finalize failed: 0x" +
std::to_string(hr)) is invoked) to format hr as a hexadecimal value (e.g.,
produce "0x80070002") — replace the std::to_string(hr) usage with a hex
formatter (sprintf/stringstream/std::format) and cast hr to an unsigned integer
type so negative HRESULTs render as the expected 0x... hex code.
In `@tests/test_video_writer.cpp`:
- Around line 59-67: The TearDown currently sweeps all files matching
"ccap_writer_test_*" in fs::temp_directory_path(), which can delete other tests'
outputs; instead add a member container (e.g.,
std::vector<std::filesystem::path> created_files_) to the test fixture and
record each temporary file path when the test creates it (where the writer or
helper generates files, push_back the path). Modify TearDown to iterate only
over created_files_ and remove those paths with fs::remove(path, ec). Ensure
creation sites use the same unique prefix (ccap_writer_test_) and push the exact
paths into created_files_ so only this test instance's files are cleaned up.
- Around line 276-296: The test VideoWriterTest::WriteAfterCloseFails currently
sets frame.data[0] = nullptr so the failure could be due to invalid frame
validation rather than the writer being closed; change the test to construct a
syntactically valid VideoFrame (proper width/height/pixelFormat and a real data
buffer pointer with enough bytes) and then call writer.writeFrame(frame) after
writer.close(), asserting EXPECT_FALSE on the result so the failure is
attributable solely to VideoWriter::writeFrame behavior when the writer is
closed.
---
Nitpick comments:
In `@tests/test_video_writer.cpp`:
- Around line 393-401: findTestVideo should explicitly detect when the
repository root wasn't found and return an empty path instead of constructing a
non-existent test file path; modify the loop in findTestVideo to break when you
reach filesystem root (use !projectRoot.has_parent_path() or compare
parent_path()==projectRoot) and after the loop check whether the sentinel files
were found — if not, return an empty fs::path() rather than projectRoot /
"tests" / "test-data" / "test.mp4" to make the fallback explicit and avoid
returning a bogus path.
🪄 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: dd216c98-a557-43e9-9a1f-eddc415e0d87
📒 Files selected for processing (8)
.vscode/tasks.jsoncli/args_parser.cppcli/args_parser.hcli/ccap_cli.cppcli/ccap_cli_utils.cppexamples/desktop/6-record_video.cppsrc/ccap_writer_windows.cpptests/test_video_writer.cpp
✅ Files skipped from review due to trivial changes (1)
- cli/ccap_cli.cpp
…ersion - Shared convertFrameToNv12 helper now honors frame.orientation (TopToBottom/BottomToTop) - DirectShow commonly yields BottomToTop RGB frames; writer was ignoring orientation and producing vertically flipped video - Added comprehensive regression tests: - SharedNv12ConversionRespectsBottomToTopOrientation (unit test for conversion helper) - BottomToTopFramesRoundTripUpright (C++ API end-to-end test) - BottomToTopFramesRoundTripUpright (C API end-to-end test) - Fixed Windows writer close() to clear configuration state (fixes GetPropertiesAfterOpen test) - Validated with live Windows camera capture: recorded video now matches original frame orientation Files changed: - src/ccap_writer_imp.h: orientation-aware row indexing for all frame formats (BGR24, BGRA32, NV12, I420) - src/ccap_writer_windows.cpp: reset config on close() - tests/test_video_writer.cpp: new orientation regression tests - tests/CMakeLists.txt: expose src headers to writer tests
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_windows.cpp`:
- Around line 51-97: open() currently overwrites m_config and attempts to create
a new writer even when an active session exists; ensure you first block or
cleanly close any existing session before mutating state: if m_isOpened (or if
m_sinkWriter != nullptr) call the class cleanup/finalize routine (e.g.,
close()/finalize/release of m_sinkWriter and reset m_isOpened/m_frameCount) or
return an error rather than proceeding, then set m_config and call
tryCreateWriter; alternatively, if you want reopen-with-rollback semantics,
snapshot the old m_config and m_sinkWriter, set the new m_config, attempt
tryCreateWriter, and on failure restore the previous m_config and reattach the
previous sinkWriter (ensuring proper refcounts/release) so you never leave an
active writer running with a mismatched m_config.
🪄 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: 69c60fcd-9018-49d9-b2e2-c93e23d57f9f
📒 Files selected for processing (4)
src/ccap_writer_imp.hsrc/ccap_writer_windows.cpptests/CMakeLists.txttests/test_video_writer.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/CMakeLists.txt
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ccap_file_reader_windows.cpp (1)
339-349:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against self-join in
stop().
m_readThread.join()is unsafe ifstop()is reached from the read thread context; that can deadlock (or throw on self-join). Add aget_id()check before joining.Suggested fix
void FileReaderWindows::stop() { m_shouldStop = true; m_isStarted = false; if (m_sourceReader) { m_sourceReader->Flush(MF_SOURCE_READER_FIRST_VIDEO_STREAM); } if (m_readThread.joinable()) { - m_readThread.join(); + if (m_readThread.get_id() != std::this_thread::get_id()) { + m_readThread.join(); + } } }🤖 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_file_reader_windows.cpp` around lines 339 - 349, In FileReaderWindows::stop, avoid joining the read thread from itself by checking the thread id before calling join: after verifying m_readThread.joinable(), compare m_readThread.get_id() to std::this_thread::get_id() and only call m_readThread.join() when they differ; leave existing sets of m_shouldStop and m_isStarted and the Flush call unchanged so the read thread can exit normally before being joined.
🧹 Nitpick comments (1)
tests/test_ccap_cli.cpp (1)
645-649: ⚡ Quick winMake JSON prefix detection whitespace-tolerant.
The current extraction only matches exact
{"schema_version"and can miss valid JSON if stdout starts with whitespace/newlines before{, causing flaky parse behavior in some environments.Suggested test-hardening patch
- constexpr std::string_view kJsonEnvelopePrefix = "{\"schema_version\""; - size_t jsonPos = result.output.find(kJsonEnvelopePrefix); + // Trim leading logs/noise by locating the first JSON object start. + size_t jsonPos = result.output.find('{'); if (jsonPos != std::string::npos) { result.output = result.output.substr(jsonPos); }🤖 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_ccap_cli.cpp` around lines 645 - 649, The JSON prefix detection using kJsonEnvelopePrefix currently only matches an exact "{\"schema_version\"", which fails if stdout has leading whitespace/newlines; update the extraction around result.output (use the kJsonEnvelopePrefix check) to be whitespace-tolerant by either trimming leading whitespace from result.output before searching or searching for the first '{' (skipping whitespace) and then checking for the schema key (e.g., find '{' then verify "schema_version" follows, or use a simple regex to locate '\{\s*"schema_version"'); modify the logic that sets jsonPos and the subsequent substr call so it correctly handles leading whitespace/newlines.
🤖 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 `@docs/content/c-interface.md`:
- Around line 154-187: The video-writing example uses
provider/ccap_provider_grab after the documented cleanup step, breaking linear
flow; either move the entire example block (including
ccap_video_writer_create/ccap_video_writer_open/ccap_video_writer_write_frame/ccap_video_writer_close/ccap_video_writer_destroy)
to appear before the provider cleanup section or make the snippet self-contained
by adding the required provider initialization and not calling any earlier
cleanup routines; ensure references to ccap_provider_grab,
ccap_video_frame_get_info, ccap_video_writer_write_frame and frame release
remain consistent with the surrounding text.
In `@docs/content/documentation.md`:
- Line 220: Update the writer input formats list so it includes the full-range
variants: modify the sentence that currently reads "Writer input supports
`NV12`, `I420`, `BGR24`, and `BGRA32`" to also include `NV12f` and `I420f`
(e.g., "Writer input supports `NV12`, `NV12f`, `I420`, `I420f`, `BGR24`, and
`BGRA32`") to reflect announced support for full-range formats.
In `@tests/test_video_writer.cpp`:
- Around line 693-700: The while loop in findTestVideo() can spin forever if the
repo markers aren't found and projectRoot.parent_path() == projectRoot
(filesystem root); update the loop to check for this condition and break (or
return an appropriate failure) when parent_path() equals the current path to
ensure progress; modify findTestVideo() to compare projectRoot and
projectRoot.parent_path() each iteration and stop the search when they are
equal, then handle the not-found case after the loop.
---
Outside diff comments:
In `@src/ccap_file_reader_windows.cpp`:
- Around line 339-349: In FileReaderWindows::stop, avoid joining the read thread
from itself by checking the thread id before calling join: after verifying
m_readThread.joinable(), compare m_readThread.get_id() to
std::this_thread::get_id() and only call m_readThread.join() when they differ;
leave existing sets of m_shouldStop and m_isStarted and the Flush call unchanged
so the read thread can exit normally before being joined.
---
Nitpick comments:
In `@tests/test_ccap_cli.cpp`:
- Around line 645-649: The JSON prefix detection using kJsonEnvelopePrefix
currently only matches an exact "{\"schema_version\"", which fails if stdout has
leading whitespace/newlines; update the extraction around result.output (use the
kJsonEnvelopePrefix check) to be whitespace-tolerant by either trimming leading
whitespace from result.output before searching or searching for the first '{'
(skipping whitespace) and then checking for the schema key (e.g., find '{' then
verify "schema_version" follows, or use a simple regex to locate
'\{\s*"schema_version"'); modify the logic that sets jsonPos and the subsequent
substr call so it correctly handles leading whitespace/newlines.
🪄 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: cdde9fa9-cf39-4c40-8d0a-98724e245f5e
📒 Files selected for processing (25)
BUILD_AND_INSTALL.mdREADME.mdREADME.zh-CN.mdcli/args_parser.cppcli/args_parser.hcli/ccap_cli.cppcli/ccap_cli_utils.cppdocs/content/c-interface.mddocs/content/cli.mddocs/content/cmake-options.mddocs/content/documentation.mdexamples/desktop/6-record_video.cppinclude/ccap_writer.hinclude/ccap_writer_c.hskills/ccap/SKILL.mdsrc/ccap_file_reader_windows.cppsrc/ccap_file_reader_windows.hsrc/ccap_writer.cppsrc/ccap_writer_apple.mmsrc/ccap_writer_c.cppsrc/ccap_writer_imp.hsrc/ccap_writer_windows.cpptests/test_ccap_cli.cpptests/test_cli_args_parser.cpptests/test_video_writer.cpp
🚧 Files skipped from review as they are similar to previous changes (11)
- cli/args_parser.h
- src/ccap_writer.cpp
- cli/ccap_cli.cpp
- include/ccap_writer_c.h
- cli/args_parser.cpp
- src/ccap_writer_c.cpp
- src/ccap_writer_windows.cpp
- include/ccap_writer.h
- examples/desktop/6-record_video.cpp
- src/ccap_writer_apple.mm
- src/ccap_writer_imp.h
…rd support in runPreview
…or preview+record
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
New Features
Bug Fixes / Reliability
Tests
Documentation