Refactor: extract shared WebPEncoder, unify error handling, improve API#1
Conversation
- Extract shared WebP encoding logic into WebPEncoder.swift, eliminating ~200 lines of duplicated code between UIImage+WebP and NSImage+WebP - Add WebPEncodingStatistics API for tracking compression metrics (original size, encoded size, compression ratio, encoding duration) - Unify NSImage diffing to return descriptive String? error messages matching UIImage behavior (was returning uninformative Bool) - Replace fatalError in NSView/NSViewController with graceful error images when view size is zero, preventing test runner crashes - Add value clamping (0.0...1.0) for CompressionQuality.custom() in both init(rawValue:) and rawValue getter - Document recommended precision thresholds per compression quality level with CIE Lab Delta E explanation https://claude.ai/code/session_01C2v2vuCbmKoHnXZyAAPtRg
Summary of ChangesHello @alexey1312, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the WebP snapshot testing library by centralizing image encoding logic, improving error reporting, and enhancing API robustness. The changes aim to reduce code duplication, provide clearer feedback during snapshot comparisons, and prevent runtime crashes due to invalid input, while also offering new insights into compression performance. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly refactors the codebase by extracting shared WebP encoding logic, which eliminates a large amount of duplicated code. The error handling is also greatly improved by providing descriptive error messages instead of booleans or fatal errors. The addition of clamping for compression quality and replacing fatalError with graceful error images are also welcome improvements to robustness and user experience.
My main feedback concerns the new WebPEncodingStatistics API. The current implementation relies on shared global state (WebPEncoder.lastStatistics), which is an anti-pattern that can lead to race conditions in concurrent environments. I've provided suggestions to refactor this into a stateless API by having the encode function return the statistics directly. This will make the API safer and more predictable.
Overall, great work on this refactoring!
| static var lastStatistics: WebPEncodingStatistics? { | ||
| get { lock.lock(); defer { lock.unlock() }; return _lastStatistics } | ||
| set { lock.lock(); defer { lock.unlock() }; _lastStatistics = newValue } | ||
| } | ||
|
|
||
| private static var _lastStatistics: WebPEncodingStatistics? | ||
| private static let lock = NSLock() |
There was a problem hiding this comment.
The use of a static, mutable lastStatistics property introduces global state, which can lead to race conditions and unpredictable behavior in concurrent environments, such as when running tests in parallel. A function's output should not depend on when it was called relative to other calls.
This property and the associated lock should be removed. The encode function should be modified to return the statistics directly.
| static func encode(_ cgImage: CGImage, compressionQuality: CGFloat) -> Data? { | ||
| let width = cgImage.width | ||
| let height = cgImage.height | ||
| let bytesPerPixel = 4 | ||
| let bytesPerRow = bytesPerPixel * width | ||
|
|
||
| guard let pixelData = extractPixelData(from: cgImage) else { | ||
| return nil | ||
| } | ||
| defer { pixelData.deallocate() } | ||
|
|
||
| let qualityValue = Float(min(max(compressionQuality, 0), 1)) | ||
| let originalSize = width * height * bytesPerPixel | ||
| let start = CFAbsoluteTimeGetCurrent() | ||
|
|
||
| guard let data = encodeWebP( | ||
| pixelData: pixelData, | ||
| width: width, | ||
| height: height, | ||
| bytesPerRow: bytesPerRow, | ||
| quality: qualityValue | ||
| ) else { | ||
| return nil | ||
| } | ||
|
|
||
| let duration = CFAbsoluteTimeGetCurrent() - start | ||
| lastStatistics = WebPEncodingStatistics( | ||
| originalSize: originalSize, | ||
| encodedSize: data.count, | ||
| encodingDuration: duration | ||
| ) | ||
|
|
||
| return data | ||
| } |
There was a problem hiding this comment.
To eliminate global state and make the API safer for concurrent use, this function should be modified to return the statistics as part of its result, rather than setting a global variable as a side effect.
static func encode(_ cgImage: CGImage, compressionQuality: CGFloat) -> (data: Data, statistics: WebPEncodingStatistics)? {
let width = cgImage.width
let height = cgImage.height
let bytesPerPixel = 4
let bytesPerRow = bytesPerPixel * width
guard let pixelData = extractPixelData(from: cgImage) else {
return nil
}
defer { pixelData.deallocate() }
let qualityValue = Float(min(max(compressionQuality, 0), 1))
let originalSize = width * height * bytesPerPixel
let start = CFAbsoluteTimeGetCurrent()
guard let data = encodeWebP(
pixelData: pixelData,
width: width,
height: height,
bytesPerRow: bytesPerRow,
quality: qualityValue
) else {
return nil
}
let duration = CFAbsoluteTimeGetCurrent() - start
let statistics = WebPEncodingStatistics(
originalSize: originalSize,
encodedSize: data.count,
encodingDuration: duration
)
return (data: data, statistics: statistics)
}| } | ||
|
|
||
| return Data(bytes: writer.mem, count: writer.size) | ||
| return WebPEncoder.encode(cgImage, compressionQuality: compressionQuality) |
There was a problem hiding this comment.
Following the suggested change in WebPEncoder.swift to make it stateless, this call site needs to be updated to handle the new return type (a tuple (data: Data, statistics: WebPEncodingStatistics)?). Since this function only needs the data, you can access the .data property of the tuple.
| return WebPEncoder.encode(cgImage, compressionQuality: compressionQuality) | |
| return WebPEncoder.encode(cgImage, compressionQuality: compressionQuality)?.data |
| } | ||
|
|
||
| return Data(bytes: writer.mem, count: writer.size) | ||
| return WebPEncoder.encode(cgImage, compressionQuality: compressionQuality) |
There was a problem hiding this comment.
Following the suggested change in WebPEncoder.swift to make it stateless, this call site needs to be updated to handle the new return type (a tuple (data: Data, statistics: WebPEncodingStatistics)?). Since this function only needs the data, you can access the .data property of the tuple.
| return WebPEncoder.encode(cgImage, compressionQuality: compressionQuality) | |
| return WebPEncoder.encode(cgImage, compressionQuality: compressionQuality)?.data |
2 participants
~200 lines of duplicated code between UIImage+WebP and NSImage+WebP
(original size, encoded size, compression ratio, encoding duration)
matching UIImage behavior (was returning uninformative Bool)
images when view size is zero, preventing test runner crashes
both init(rawValue:) and rawValue getter
level with CIE Lab Delta E explanation
https://claude.ai/code/session_01C2v2vuCbmKoHnXZyAAPtRg