refactor(image-decoder): adopt RAII for FFmpeg resources

[pproenca]

Summary

Complete RAII adoption for ImageDecoder's FFmpeg resource management:

• ✅ Add AVIOContext, MemoryBufferContext, ImageFormatContext deleters
• ✅ Migrate ImageDecoder members from raw pointers to RAII wrappers
• ✅ Eliminate raw av_malloc calls
• ✅ Add instance counter for leak detection

Benefits

• Safety: Automatic cleanup prevents leaks even on exception paths
• Maintainability: Eliminates manual Cleanup() logic
• Consistency: Follows RAII pattern used elsewhere in codebase

Files Changed (11)

• src/ffmpeg_raii.h - New deleters for ImageDecoder-specific types
• src/image_decoder.cc - Converted to RAII members
• test/stress/memory-leak.test.ts - Added leak detection test

Test Plan

• ✅ All existing tests pass
• ✅ Memory leak tests added
• ✅ Build succeeds on macOS/Linux

Commits

1. feat(raii): add AVIOContext and MemoryBufferContext deleters
2. refactor(image-decoder): migrate members to RAII types
3. refactor(image-decoder): eliminate raw av_malloc calls
4. test(image-decoder): add memory leak detection

[claude]

PR Review: RAII Adoption for ImageDecoder

Thank you for this excellent refactoring! This PR demonstrates strong understanding of C++ RAII patterns and FFmpeg resource management. Here's my detailed review following the repository's code review standards.

---

✅ Strengths

1. Excellent RAII Implementation

• Correct deleter semantics: The new deleters properly handle FFmpeg's ownership model:

  • AVIOContextDeleter correctly frees both the context AND the av_malloc'd buffer (src/ffmpeg_raii.h:129-139)
  • ImageFormatContextDeleter uses avformat_close_input which handles both context and streams
  • Cleanup order is well-documented in Cleanup() (src/image_decoder.cc:254-259)

• Proper ownership transfer: The avformat_open_input handling is textbook-perfect:

  AVFormatContext* raw_fmt = format_context_.release();  // Release ownership
  int ret = avformat_open_input(&raw_fmt, nullptr, input_format, nullptr);
  if (ret < 0) {
    // avformat_open_input freed raw_fmt on failure
    return false;  // RAII handles cleanup
  }
  format_context_.reset(raw_fmt);  // Take ownership back on success

  This correctly handles FFmpeg's double-pointer semantics (src/image_decoder.cc:376-383).

2. Memory Leak Prevention

• Eliminated 8+ duplicate cleanup sequences in error paths (mentioned in commit message)
• Added instance counter for leak detection (counterImageDecoders) - critical for CI validation
• Comprehensive stress tests with proper GC behavior (IIFE wrapping, multiple GC cycles)

3. Test Coverage Excellence

• Unit test for RAII deleter behavior (test/unit/ffmpeg_raii.test.cc)
• Integration tests for both animated (GIF) and static (PNG) decode paths
• Stress tests with 50 iterations validate no accumulation
• Tests properly close VideoFrame images to prevent false positives

4. Clean Code Organization

• MemoryBufferContext moved to ffmpeg namespace for consistency with RAII wrappers
• Forward declaration in header prevents circular dependencies
• Clear comments explaining ownership and cleanup order

---

🔍 Issues & Recommendations

CRITICAL: Resource Cleanup Order Issue

Location: src/image_decoder.cc:254-259

The comment states "format_context_ first (may reference avio_context_->buffer)" but this is incorrect based on FFmpeg semantics:

// Reset animated image RAII members
// Order matters: format_context_ first (may reference avio_context_->buffer)
// then avio_context_ (frees buffer too via AVIOContextDeleter)
// then mem_ctx_ (no longer needed)
format_context_.reset();  // Calls ImageFormatContextDeleter
avio_context_.reset();    // Calls AVIOContextDeleter (frees buffer too)
mem_ctx_.reset();         // Calls MemoryBufferContextDeleter

Problem: avformat_close_input() (called by ImageFormatContextDeleter) does NOT free the custom AVIO context stored in ctx->pb. According to FFmpeg docs:

"If *s was opened with avio_open(), avio_closep() will be called on it. Otherwise the user is responsible for closing it."

Since we used a custom AVIO context (not avio_open), avformat_close_input will NOT call avio_context_free. The custom AVIO context must be freed separately.

However, the current order happens to work because:

1. format_context_.reset() calls avformat_close_input which sets ctx->pb = NULL internally (after finishing with it)
2. avio_context_.reset() then frees the AVIO context

Risk: If FFmpeg's implementation changes to access ctx->pb after the context is freed, we'll have a use-after-free bug.

Recommendation: The safest order is:

// Reset in reverse construction order for safety:
// 1. format_context_ (may read from avio_context_->buffer during close)
// 2. avio_context_ (frees buffer allocated with av_malloc)  
// 3. mem_ctx_ (opaque pointer no longer needed)
format_context_.reset();
avio_context_.reset();
mem_ctx_.reset();

The current order is correct, but the comment should be clarified:

// Reset animated image RAII members in reverse construction order
// 1. format_context_ first (avformat_close_input may read from pb but doesn't free it)
// 2. avio_context_ second (we own this custom context, must free it ourselves)
// 3. mem_ctx_ last (opaque pointer used by avio_context_)

---

Minor: Potential Double-Free in AVIOContextDeleter

Location: src/ffmpeg_raii.h:129-139

struct AVIOContextDeleter {
  void operator()(AVIOContext* ctx) const noexcept {
    if (ctx) {
      // Free the buffer allocated with av_malloc before freeing context
      if (ctx->buffer) {
        av_freep(&ctx->buffer);  // ⚠️ Potential issue
      }
      avio_context_free(&ctx);
    }
  }
};

Issue: According to FFmpeg source, avio_context_free already calls av_freep(&s->buffer) internally (see libavformat/aviobuf.c). Calling av_freep twice on the same pointer is safe (it sets to NULL), but it's redundant.

Recommendation:

struct AVIOContextDeleter {
  void operator()(AVIOContext* ctx) const noexcept {
    if (ctx) {
      // avio_context_free handles buffer cleanup internally
      avio_context_free(&ctx);
    }
  }
};

However, verify this with your FFmpeg version. If you're targeting older FFmpeg versions that don't auto-free the buffer, keep the explicit free.

Action: Add a comment documenting which FFmpeg versions need explicit buffer freeing, or test with minimum supported version (FFmpeg 5.0 per CLAUDE.md).

---

Code Style: Inconsistent RAII Ordering in Cleanup()

Location: src/image_decoder.cc:246-259

The cleanup order for static vs animated resources seems arbitrary:

void ImageDecoder::Cleanup() {
  // Reset RAII members (automatic cleanup)
  codec_context_.reset();  // Static image resources
  sws_context_.reset();
  frame_.reset();
  packet_.reset();

  // Reset animated image RAII members
  format_context_.reset();  // Animated image resources
  avio_context_.reset();
  mem_ctx_.reset();
  // ...
}

Recommendation: Add comments explaining why static resources are reset first, or group by dependency order:

void ImageDecoder::Cleanup() {
  // Reset codec resources (used by both static and animated paths)
  codec_context_.reset();
  sws_context_.reset();
  frame_.reset();
  packet_.reset();

  // Reset animated image format/IO resources (in reverse construction order)
  format_context_.reset();  // Uses avio_context_ during close
  avio_context_.reset();    // Uses mem_ctx_ as opaque pointer
  mem_ctx_.reset();         // No dependencies
  
  // Clear remaining state
  video_stream_index_ = -1;
  decoded_data_.clear();
  decoded_frames_.clear();
}

---

Testing: Missing Error Path Coverage

Location: test/stress/memory-leak.test.ts

The stress tests only cover success paths:

• Animated GIF decode succeeds
• Static PNG decode succeeds

Missing: Tests for error paths where RAII cleanup is critical:

it('ImageDecoder: RAII cleanup on invalid data', async () => {
  const before = getCounters();
  
  for (let i = 0; i < 50; i++) {
    await (async () => {
      const { ImageDecoder } = await import('@pproenca/node-webcodecs');
      let decoder: InstanceType<typeof ImageDecoder> | null = null;
      
      try {
        // Provide invalid GIF data to trigger error paths
        decoder = new ImageDecoder({
          type: 'image/gif',
          data: new Uint8Array([0x47, 0x49, 0x46]),  // Truncated header
        });
        
        // This should throw due to invalid data
        await decoder.decode({ frameIndex: 0 });
      } catch (e) {
        // Expected error
      } finally {
        if (decoder) {
          decoder.close();
          decoder = null;
        }
      }
    })();
  }
  
  // Force GC
  for (let i = 0; i < 5; i++) {
    forceGC();
    await new Promise(resolve => setTimeout(resolve, 100));
  }
  
  const after = getCounters();
  assertNoLeaks(before, after, 'ImageDecoder error path');
});

Rationale: RAII's main benefit is preventing leaks in error paths. The current tests don't verify this critical behavior.

---

Documentation: Missing FFmpeg Upstream References

Location: src/ffmpeg_raii.h:127-150

The new deleters lack references to FFmpeg documentation. Following CLAUDE.md guidelines:

"Comment FFmpeg quirks — Document non-obvious behavior"

Recommendation: Add upstream references:

// AVIOContext deleter (handles avio_context_free semantics)
// FFmpeg docs: https://ffmpeg.org/doxygen/trunk/group__lavf__io.html#ga3d50df9c77f408d2e5022c3e370a5813
// NOTE: avio_context_free auto-frees ctx->buffer in FFmpeg 5.0+
// For custom AVIO contexts, avformat_close_input does NOT free the context
struct AVIOContextDeleter {
  void operator()(AVIOContext* ctx) const noexcept {
    if (ctx) {
      avio_context_free(&ctx);  // Frees both context and buffer
    }
  }
};

---

📝 Nitpicks (Non-blocking)

1. test/unit/ffmpeg_raii.test.cc - Standalone C++ test is excellent! Consider adding to CI build/test pipeline if not already included.

2. Commit messages - Very clear! The "feat/refactor/test" prefixes follow conventional commits pattern.

3. src/image_decoder.cc:333-337 - C++20 designated initializers are nice, but verify binding.gyp sets -std=c++20 (or falls back to brace initialization for C++17 compatibility).

---

🎯 Action Items (Before Merge)

MUST FIX:

1. ✅ Clarify cleanup order comment in Cleanup() (src/image_decoder.cc:254)
2. ⚠️ Verify AVIOContextDeleter buffer freeing behavior with FFmpeg 5.0+ (or add version comment)

SHOULD FIX:

3. Add error path tests for RAII cleanup validation
4. Add FFmpeg upstream doc references to new deleters

CONSIDER:

5. Document designated initializer C++ version requirement
6. Add test/unit/ffmpeg_raii.test.cc to CI if not already included

---

✅ Review Checklist (Per CLAUDE.md)

• JSDoc updated (N/A - C++ only changes)
• Type tests (N/A - no TypeScript API changes)
• Integration tests added (test/stress/memory-leak.test.ts)
• Error path tests (MISSING - see recommendation above)
• W3C spec compliance (N/A - internal refactor)
• FFmpeg version compatibility (assumes 5.0+)
• Platform-specific code (N/A - uses portable FFmpeg APIs)
• No duplication (excellent refactoring!)
• Security: inputs validated at TS layer (no changes)
• npm run check passes (CANNOT VERIFY - please confirm CI status)
• Build produces valid native addon (assuming CI passes)
• RAII wrappers only (no raw FFmpeg allocations) ✅

---

🎉 Conclusion

Overall Assessment: Excellent work! This PR significantly improves code safety and maintainability.

Recommendation: Approve with minor changes

The RAII refactoring is well-executed with proper ownership semantics and comprehensive testing. The identified issues are minor (mostly documentation/comments), but should be addressed to prevent future maintenance headaches.

Specific Requests:

1. Update cleanup order comment (2 minutes)
2. Verify/document AVIOContextDeleter buffer behavior (5 minutes)
3. Consider adding error path tests (15 minutes)

Once these are addressed, this PR is ready to merge. Great contribution to the project's safety and maintainability!

---

Reviewed by: Claude Code Agent
Standards: node-webcodecs/CLAUDE.md Code Review Standards
Date: 2026-01-04

[pproenca]

Closing PR as requested