fix: address review feedback for video writer and CLI parsing

Author: wysaid

cli/args_parser.cpp

@@ -404,9 +404,18 @@ CLIOptions parseArgs(int argc, char* argv[]) {
                 opts.videoFilePath = argv[++i];
             }
         } else if (arg == "--record") {
-            if (i + 1 < argc) {
-                opts.recordVideoPath = argv[++i];
+#ifdef CCAP_ENABLE_VIDEO_WRITER
+            if (i + 1 >= argc || argv[i + 1][0] == '-') {
+                std::cerr << "Error: --record requires an output file path.\n\n";
+                printUsage(argv[0]);
+                std::exit(1);
             }
+            opts.recordVideoPath = argv[++i];
+#else
+            std::cerr << "Error: --record is not supported in this build. Rebuild with CCAP_ENABLE_VIDEO_WRITER=ON.\n\n";
+            printUsage(argv[0]);
+            std::exit(1);
+#endif
         } else if (arg == "-w" || arg == "--width") {
             if (i + 1 < argc) {
                 opts.width = std::atoi(argv[++i]);

examples/desktop/6-record_video.cpp

@@ -50,7 +50,7 @@ int main(int argc, char** argv) {
         outputPath = commandLine.argv[1];
     } else {
         std::string exeDir = commandLine.argv[0];
-        if (auto pos = exeDir.find_last_of("/\\"); pos != std::string::npos && exeDir[0] != '.') {
+        if (auto pos = exeDir.find_last_of("/\\"); pos != std::string::npos && !exeDir.empty() && exeDir[0] != '.') {
             exeDir = exeDir.substr(0, pos);
         } else {
             exeDir = std::filesystem::current_path().string();

include/ccap_writer.h

@@ -70,6 +70,7 @@ class CCAP_EXPORT VideoWriter {
      * @brief Open writer to a file path.
      * @param filePath Output file path (e.g., "output.mp4")
      * @param config Writer configuration (width, height, codec, etc.)
+        * @note Call `close()` before reopening an existing writer instance.
      * @return true on success, false on failure.
      */
     bool open(std::string_view filePath, const WriterConfig& config);
@@ -87,6 +88,7 @@ class CCAP_EXPORT VideoWriter {
     bool writeFrame(const VideoFrame& frame, uint64_t timestampNs = 0);
 
     /// Query the actual codec being used (may differ from config due to fallback).
+    /// Only meaningful after `open()` succeeds.
     VideoCodec actualCodec() const;
 
     uint32_t width() const;

include/ccap_writer_c.h

@@ -39,16 +39,26 @@ typedef enum {
 
 /* ========== Data Structures ========== */
 
-/** @brief Video writer configuration */
+/**
+ * @brief Video writer configuration.
+ * @note Use `CCAP_WRITER_CONFIG_INIT` for codec/container/frameRate/bitRate defaults.
+ *       `width` and `height` must still be set before opening a writer.
+ */
 typedef struct {
     CcapVideoCodec codec;          ///< Preferred codec
     CcapVideoFormat container;     ///< Container format
     uint32_t width;                ///< Frame width
     uint32_t height;               ///< Frame height
-    double frameRate;              ///< Target frame rate (default 30fps)
+    double frameRate;              ///< Target frame rate; 0 lets open() normalize to 30fps
     uint64_t bitRate;              ///< Target bit rate in bits/s (0 = auto)
 } CcapWriterConfig;
 
+/**
+ * @brief Default initializer for `CcapWriterConfig`.
+ * @note `width` and `height` remain 0 and must be assigned by the caller.
+ */
+#define CCAP_WRITER_CONFIG_INIT { CCAP_VIDEO_CODEC_HEVC, CCAP_VIDEO_FORMAT_MP4, 0u, 0u, 30.0, 5000000ULL }
+
 /* ========== Writer Lifecycle ========== */
 
 /**
@@ -100,7 +110,8 @@ CCAP_EXPORT bool ccap_video_writer_write_frame(CcapVideoWriter* writer,
 /**
  * @brief Get the actual codec being used (may differ from config due to fallback)
  * @param writer Pointer to CcapVideoWriter instance
- * @return Actual codec enum value
+ * @return Actual codec enum value. Only meaningful after `ccap_video_writer_open()` succeeds.
+ *         Unopened or null writers return `CCAP_VIDEO_CODEC_H264` for ABI compatibility.
  */
 CCAP_EXPORT CcapVideoCodec ccap_video_writer_actual_codec(const CcapVideoWriter* writer);
 

src/ccap_writer.cpp

@@ -16,13 +16,15 @@ namespace ccap {
 static VideoWriter::Impl* impl(void* p) { return reinterpret_cast<VideoWriter::Impl*>(p); }
 static const VideoWriter::Impl* impl(const void* p) { return reinterpret_cast<const VideoWriter::Impl*>(p); }
 
-VideoWriter::VideoWriter() : m_impl(createVideoWriterImpl()) {}
+VideoWriter::VideoWriter() :
+    m_impl(createVideoWriterImpl()) {}
 
 VideoWriter::~VideoWriter() {
     delete impl(m_impl);
 }
 
-VideoWriter::VideoWriter(VideoWriter&& other) noexcept : m_impl(other.m_impl) {
+VideoWriter::VideoWriter(VideoWriter&& other) noexcept :
+    m_impl(other.m_impl) {
     other.m_impl = nullptr;
 }
 
@@ -40,6 +42,10 @@ bool VideoWriter::open(std::string_view filePath, const WriterConfig& config) {
         reportError(ErrorCode::WriterNotOpened, "VideoWriter not available on this platform");
         return false;
     }
+    if (impl(m_impl)->isOpened()) {
+        reportError(ErrorCode::WriterOpenFailed, "VideoWriter is already opened. Call close() before reopening.");
+        return false;
+    }
     return impl(m_impl)->open(filePath, config);
 }
 
@@ -81,7 +87,8 @@ double VideoWriter::frameRate() const {
 
 namespace ccap {
 
-VideoWriter::VideoWriter() : m_impl(nullptr) {}
+VideoWriter::VideoWriter() :
+    m_impl(nullptr) {}
 VideoWriter::~VideoWriter() = default;
 VideoWriter::VideoWriter(VideoWriter&&) noexcept = default;
 VideoWriter& VideoWriter::operator=(VideoWriter&&) noexcept = default;

src/ccap_writer_apple.mm

@@ -160,22 +160,24 @@ void close() override {
         if (!m_isOpened) return;
         m_isOpened = false;
 
+        AVAssetWriter* assetWriter = m_assetWriter;
+        AVAssetWriterInput* writerInput = m_writerInput;
+
         @try {
-            if (m_writerInput) {
-                [m_writerInput markAsFinished];
+            if (writerInput) {
+                [writerInput markAsFinished];
             }
-            if (m_assetWriter) {
-                dispatch_queue_t queue = dispatch_queue_create("com.ccap.writer.close", DISPATCH_QUEUE_SERIAL);
+            if (assetWriter) {
                 dispatch_semaphore_t sem = dispatch_semaphore_create(0);
-                dispatch_async(queue, ^{
-                    [m_assetWriter finishWritingWithCompletionHandler:^{
-                        dispatch_semaphore_signal(sem);
-                    }];
-                });
-                dispatch_semaphore_wait(sem, dispatch_time(DISPATCH_TIME_NOW, 10 * NSEC_PER_SEC));
-
-                if (m_assetWriter.error) {
-                    reportError(ErrorCode::WriterCloseFailed, "finishWriting failed: " + std::string(m_assetWriter.error.localizedDescription.UTF8String));
+                [assetWriter finishWritingWithCompletionHandler:^{
+                    dispatch_semaphore_signal(sem);
+                }];
+
+                const long waitResult = dispatch_semaphore_wait(sem, dispatch_time(DISPATCH_TIME_NOW, 10 * NSEC_PER_SEC));
+                if (waitResult != 0) {
+                    reportError(ErrorCode::WriterCloseFailed, "finishWriting timed out after 10 seconds");
+                } else if (assetWriter.error) {
+                    reportError(ErrorCode::WriterCloseFailed, "finishWriting failed: " + std::string(assetWriter.error.localizedDescription.UTF8String));
                 }
             }
         }
@@ -186,6 +188,8 @@ void close() override {
         m_pixelBufferAdaptor = nil;
         m_writerInput = nil;
         m_assetWriter = nil;
+        m_sessionStarted = NO;
+        m_frameCount = 0;
         std::memset(&m_config, 0, sizeof(m_config));
     }
 

src/ccap_writer_imp.h

@@ -20,7 +20,8 @@ namespace ccap {
 void reportError(ErrorCode errorCode, std::string_view description);
 
 struct VideoWriter::Impl {
-    Impl() : m_actualCodec(VideoCodec::H264) {}
+    Impl() :
+        m_actualCodec(VideoCodec::H264) {}
     virtual ~Impl() = default;
 
     virtual bool open(std::string_view filePath, const WriterConfig& config) = 0;
@@ -76,6 +77,9 @@ inline bool convertFrameToNv12(const VideoFrame& frame,
                                uint32_t& yStride, uint32_t& uvStride) {
     const int w = static_cast<int>(frame.width);
     const int h = static_cast<int>(frame.height);
+    if (w <= 0 || h <= 0 || (w % 2) != 0 || (h % 2) != 0) {
+        return false;
+    }
     const int w2 = w / 2;
     const int h2 = h / 2;
     const FrameOrientation orientation = frame.orientation;

src/ccap_writer_windows.cpp

@@ -14,11 +14,13 @@
 #define NOMINMAX
 #endif
 #include <atomic>
+#include <iomanip>
 #include <mfapi.h>
 #include <mferror.h>
 #include <mfidl.h>
 #include <mfreadwrite.h>
 #include <mutex>
+#include <sstream>
 #include <vector>
 #include <windows.h>
 
@@ -31,9 +33,22 @@
 
 namespace ccap {
 
+namespace {
+
+std::string formatHRESULT(HRESULT hr) {
+    std::ostringstream stream;
+    stream << "0x"
+           << std::uppercase << std::hex << std::setw(8) << std::setfill('0')
+           << static_cast<unsigned int>(hr);
+    return stream.str();
+}
+
+} // namespace
+
 class WriterWindows : public VideoWriter::Impl {
 public:
-    WriterWindows() : m_sinkWriter(nullptr), m_streamIndex(0), m_mfInitialized(false) {
+    WriterWindows() :
+        m_sinkWriter(nullptr), m_streamIndex(0), m_mfInitialized(false) {
         HRESULT hr = MFStartup(MF_VERSION, MFSTARTUP_FULL);
         m_mfInitialized = SUCCEEDED(hr);
         if (!m_mfInitialized) {
@@ -103,7 +118,7 @@ class WriterWindows : public VideoWriter::Impl {
         if (m_sinkWriter) {
             HRESULT hr = m_sinkWriter->Finalize();
             if (FAILED(hr)) {
-                reportError(ErrorCode::WriterCloseFailed, "IMFSinkWriter::Finalize failed: 0x" + std::to_string(hr));
+                reportError(ErrorCode::WriterCloseFailed, "IMFSinkWriter::Finalize failed: " + formatHRESULT(hr));
             }
             m_sinkWriter->Release();
             m_sinkWriter = nullptr;

tests/test_cli_args_parser.cpp

@@ -103,6 +103,50 @@ TEST(CLIArgsParserTest, RejectsMissingSchemaVersionValue) {
         "--schema-version requires a value");
 }
 
+#ifdef CCAP_ENABLE_VIDEO_WRITER
+TEST(CLIArgsParserTest, ParsesRecordOutputPath) {
+    char arg0[] = "ccap";
+    char arg1[] = "--record";
+    char arg2[] = "capture.mp4";
+    char* argv[] = { arg0, arg1, arg2, nullptr };
+
+    const ccap_cli::CLIOptions opts = ccap_cli::parseArgs(3, argv);
+
+    EXPECT_EQ(opts.recordVideoPath, "capture.mp4");
+}
+
+TEST(CLIArgsParserTest, RejectsMissingRecordValue) {
+    char arg0[] = "ccap";
+    char arg1[] = "--record";
+    char arg2[] = "--timeout";
+    char arg3[] = "5";
+    char* argv[] = { arg0, arg1, arg2, arg3, nullptr };
+
+    EXPECT_EXIT(
+        {
+            (void)ccap_cli::parseArgs(4, argv);
+            std::exit(0);
+        },
+        ::testing::ExitedWithCode(1),
+        "--record requires an output file path");
+}
+#else
+TEST(CLIArgsParserTest, RejectsRecordWhenWriterUnsupported) {
+    char arg0[] = "ccap";
+    char arg1[] = "--record";
+    char arg2[] = "capture.mp4";
+    char* argv[] = { arg0, arg1, arg2, nullptr };
+
+    EXPECT_EXIT(
+        {
+            (void)ccap_cli::parseArgs(3, argv);
+            std::exit(0);
+        },
+        ::testing::ExitedWithCode(1),
+        "--record is not supported in this build");
+}
+#endif
+
 #if defined(_WIN32) || defined(_WIN64)
 TEST(CLIArgsParserTest, ParsesWindowsCameraBackendOption) {
     char arg0[] = "ccap";