Add lossy WebP animation encode (managed VP8)

[PrzemyslawKlys]

Summary\n- add managed lossy animation encode using VP8 intra with ALPH support\n- fall back to VP8L + quantization when VP8 encode fails\n- update WebP animation tests and docs/roadmap\n\n## Testing\n- dotnet build CodeGlyphX.Tests/CodeGlyphX.Tests.csproj -f net8.0\n- dotnet test CodeGlyphX.Tests/CodeGlyphX.Tests.csproj -f net8.0 --no-build\n\n## Notes\n- full solution build on Linux still requires .NET Framework 4.7.2 targeting pack

[claude]

Claude finished @PrzemyslawKlys's task —— View job

---

WebP Animation Encoding Review

I've completed a thorough analysis of this PR that adds managed lossy WebP animation encoding with VP8 support. Here's my comprehensive feedback:

Todo List:

• Read and analyze the modified files
• Review code quality and best practices
• Identify potential bugs and issues
• Analyze performance considerations
• Check for security concerns
• Evaluate test coverage
• Provide constructive feedback

---

Code Quality & Best Practices

Positive aspects:

• Clean, well-documented API with clear method purposes
• Good error handling with descriptive exceptions and validation
• Consistent coding style following .NET conventions
• Proper use of ReadOnlySpan<byte> for memory efficiency
• Well-structured fallback mechanism (VP8 → VP8L + quantization)

Areas for improvement:

• Some magic numbers could be constants (e.g., 0x1000000 in WebpWriter.cs:72,126)
• The quantization algorithm is simplistic and could benefit from more sophisticated dithering

Performance Issues

Critical performance problems identified:

1. Inefficient alpha detection (WebpWriter.cs:137-139):

   if (!alphaUsed) {
       alphaUsed = ComputeAlphaUsed(frame.Rgba, frame.Width, frame.Height, frame.Stride);
   }

   The current code continues scanning every frame even after alpha is detected. This should short-circuit once alphaUsed becomes true.

2. Unnecessary memory allocations (WebpWriter.cs:375-383):
   The WriteChunk method always creates a new byte array copy of the payload, which is wasteful for large VP8/VP8L payloads in animations. This could significantly increase memory pressure.

3. Redundant pixel copying (WebpManagedDecoder.cs:115-116):
   Canvas snapshots are created via Buffer.BlockCopy for each frame, which could be optimized with buffer pooling for large animations.

Potential Bugs & Issues

Medium-severity issues:

1. Integer overflow risk (WebpWriter.cs:183):

   var bits = 1 + (quality * 7 / 100);

   While unlikely with quality 0-100, this could be made more robust with explicit bounds checking.

2. Inconsistent error handling: Some methods return bool + out parameters while others throw exceptions. Consider standardizing the error handling pattern.

3. Missing validation: The BuildAnmfPayload method doesn't validate that imageFourCc is exactly 4 characters.

Security Concerns

Low-to-medium severity:

• The WebP decoder processes untrusted input but appears to have appropriate bounds checking
• Memory allocations are well-bounded with the 256MB limit
• No obvious buffer overflow or injection vulnerabilities detected
• Integer overflow protections are generally adequate

Test Coverage Analysis

Strengths:

• Good basic functionality coverage with lossy animation test
• Tests verify alpha preservation correctly
• Chunk presence validation ensures VP8 + ALPH chunks are written

Gaps identified:

1. Missing VP8 path verification: The test doesn't confirm that VP8 encoding was actually used vs. the VP8L fallback
2. No error condition testing: Missing tests for invalid inputs, oversized frames, etc.
3. No performance regression tests: Large animation scenarios aren't covered
4. Limited edge case coverage: No tests for quality=0, quality=100, or frame validation edge cases

Recommendations

High Priority

1. Fix alpha detection performance: Implement short-circuit logic to stop scanning once alpha is found
2. Optimize WriteChunk: Add overload to avoid unnecessary payload copying
3. Add VP8 path verification: Ensure tests confirm VP8 encoding was used, not just fallback

Medium Priority

1. Improve test coverage: Add error condition tests and edge cases
2. Consider buffer pooling: For canvas operations in animation compositing
3. Add bounds constants: Replace magic numbers with named constants

Documentation & Clarity

1. Clarify VP8 limitations: The README/roadmap mentions "VP8 inter" but WebP still images only use intraframes - this is confusing
2. Performance guidance: Document memory usage implications for large animations

SonarQube Issues

The 5.3% code duplication (exceeding 3% threshold) is likely from similar chunk parsing logic. Consider extracting common patterns into helper methods.

Overall Assessment

This is a solid implementation that adds valuable VP8 lossy animation support. The core logic is sound, but there are several performance optimizations and test improvements that would strengthen the implementation. The fallback mechanism is well-designed and the API is clean.

Recommendation: Approve with performance fixes, particularly the alpha detection optimization which could significantly impact large animations.

---

[PrzemyslawKlys]

Update: added managed lossy animation encode and merged latest master.\n\nWhat changed:\n- New WebpWriter.WriteAnimationRgba32Lossy (managed VP8 intra + ALPH, VP8L+quantize fallback).\n- ANMF payload builder now supports VP8/VP8L + optional ALPH.\n- New lossy animation test verifies alpha preservation; README/ROADMAP updated.\n\nValidation:\n- dotnet build CodeGlyphX.Tests/CodeGlyphX.Tests.csproj -f net8.0\n- dotnet test CodeGlyphX.Tests/CodeGlyphX.Tests.csproj -f net8.0 --no-build\n- dotnet build CodeGlyphX.sln (fails on Linux: missing .NETFramework 4.7.2 targeting pack)

[Copilot]

Pull request overview

Adds managed lossy animated WebP encoding by embedding per-frame VP8 (intra/keyframe) payloads with optional ALPH chunks, with a VP8L+quantization fallback path.

Changes:

• Add WebpWriter.WriteAnimationRgba32Lossy(...) for VP8 intra animated encode with VP8L fallback.
• Extend ANMF frame payload building to support VP8 + optional ALPH (in addition to VP8L).
• Update docs/roadmap and add a lossy animation encode test.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
ROADMAP.md	Updates WebP milestone text to reflect lossy animation progress.
README.md	Updates WebP format support notes and adds an explicit limitation note.
CodeGlyphX/Rendering/Webp/WebpWriter.cs	Implements lossy animated WebP encoding and VP8 payload extraction; updates ANMF payload construction.
CodeGlyphX.Tests/WebpAnimationEncodeTests.cs	Adds a test for lossy animation encode alpha preservation.

[Copilot]

“VP8 inter (lossy) frames are not supported yet” is potentially misleading, since VP8 interframes are not valid for WebP still images and are currently rejected by the decoder (CodeGlyphX/Rendering/Webp/WebpVp8Decoder.cs:279). Consider rewording this to describe the real limitation (e.g., encoder is intra-only / keyframe-only, or missing other VP8 features that are applicable to WebP) instead of suggesting VP8 interframes are a planned WebP feature.

Suggested change

	- VP8 inter (lossy) frames are not supported yet
	- VP8 inter (lossy) frames are not valid for WebP still images and are rejected by the decoder

[Copilot]

alphaUsed |= ComputeAlphaUsed(...) will still scan every subsequent frame even after alphaUsed becomes true because |= does not short-circuit. For large animations this adds a full extra pass over pixel data per frame. Consider using a short-circuiting form (e.g., alphaUsed = alphaUsed || ComputeAlphaUsed(...), or guard with if (!alphaUsed) ...) so the scan stops once alpha has been detected.

Suggested change

	alphaUsed |= ComputeAlphaUsed(frame.Rgba, frame.Width, frame.Height, frame.Stride);
	if (!alphaUsed) {
	alphaUsed = ComputeAlphaUsed(frame.Rgba, frame.Width, frame.Height, frame.Stride);
	}

[Copilot]

BuildAnmfPayload now writes the embedded image chunks via WriteChunk(...), but WriteChunk always allocates and copies the entire payload into a new byte[] before writing. For animation frames this duplicates potentially large VP8/VP8L payloads and can significantly increase allocations. Consider adding a WriteChunk(Stream, string, byte[]) overload (or updating WriteChunk to write directly when the payload is already a byte[]) and use it here to avoid the extra copy.

[Copilot]

This test validates alpha round-tripping, but it doesn't assert that the lossy VP8 animation path was actually used. If WebpVp8Encoder.TryEncodeLossyRgba32 fails, the code falls back to quantized VP8L, and the test would likely still pass (VP8L preserves alpha too). Consider also asserting that the encoded animation frame contains the expected embedded chunks (e.g., ANMF contains a VP8 chunk and, for non-opaque input, an ALPH chunk) so the new VP8+ALPH animation encode behavior is exercised.

[Copilot]

The roadmap line says the remaining work is “VP8 inter”, but the VP8 decoder currently rejects interframes as invalid for WebP still images (CodeGlyphX/Rendering/Webp/WebpVp8Decoder.cs:279). Since WebP frames are still images, “VP8 inter” reads as either inaccurate or ambiguous. Consider clarifying what capability is actually missing (e.g., additional intra features, container-level frame differencing/cropping, etc.) rather than implying future support for VP8 interframes.

Suggested change

	- WebP decode + encode (lossless + VP8 lossy intra complete; remaining: VP8 inter). [formats][webp]
	- WebP decode + encode (lossless + VP8 lossy intra complete; remaining: animated WebP container features like multi‑frame, frame differencing/cropping, and timing). [formats][webp]

[PrzemyslawKlys]

Update: added animated WebP frame decode API (managed).\n\nWhat changed:\n- WebpReader.TryDecodeAnimationFrames/DecodeAnimationFrames returns frame buffers + canvas/animation options.\n- WebpManagedDecoder parses ANIM/ANMF into frame metadata (duration, blend, dispose).\n- Tests cover multi-frame decode metadata + payload.\n- README updated to note ImageReader first-frame vs WebpReader frame decode.\n\nValidation:\n- dotnet build CodeGlyphX.Tests/CodeGlyphX.Tests.csproj -f net8.0\n- dotnet test CodeGlyphX.Tests/CodeGlyphX.Tests.csproj -f net8.0 --no-build\n- dotnet build CodeGlyphX.sln still fails on Linux (missing .NETFramework 4.7.2 targeting pack)

[PrzemyslawKlys]

Update: composited WebP animation decode + perf/test fixes.\n\nWhat changed:\n- Added WebpReader.TryDecodeAnimationCanvasFrames/DecodeAnimationCanvasFrames for full-canvas composited frames; handles blend + dispose-to-background.\n- Refactored animation frame parsing helper; added compositing tests for canvas frames + disposal.\n- WebP animation encode test now asserts VP8 + ALPH chunk presence.\n- WebpWriter perf: short-circuit alpha scan and avoid chunk payload copy.\n- README updated with composited frame decode note.\n\nValidation:\n- dotnet restore CodeGlyphX.Tests/CodeGlyphX.Tests.csproj -p:TargetFramework=net8.0\n- dotnet build CodeGlyphX.Tests/CodeGlyphX.Tests.csproj -f net8.0\n- dotnet test CodeGlyphX.Tests/CodeGlyphX.Tests.csproj -f net8.0 --no-build\n- dotnet build CodeGlyphX.sln (fails on Linux: missing .NETFramework 4.7.2 targeting pack)

[PrzemyslawKlys]

Update: clarified VP8 interframe support wording in docs.\n\nWhat changed:\n- README now notes managed VP8 decode supports keyframes only; interframes (if present) are not supported.\n- ROADMAP wording adjusted to 'VP8 inter if encountered'.\n\nValidation:\n- dotnet build CodeGlyphX.Tests/CodeGlyphX.Tests.csproj -f net8.0\n- dotnet test CodeGlyphX.Tests/CodeGlyphX.Tests.csproj -f net8.0 --no-build\n- dotnet build CodeGlyphX.sln (fails on Linux: missing .NETFramework 4.7.2 targeting pack)

[sonarqubecloud]

Quality Gate failed

Failed conditions
5.3% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud