Support libavif loop count #69
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughDependency versions bumped to libavif 1.0.0 across manifests and podspec (pod version → 0.12.0). Added Changes
Sequence Diagram(s)(omitted — changes are limited to dependency bumps and internal encoder/decoder logic; no multi-component sequential flow requiring diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
This requires libavif 1.0.0+
dreampiggy
force-pushed
the
feature/avif_loop_count
branch
from
f4c63cd to
634f480
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@SDWebImageAVIFCoder/Classes/SDImageAVIFCoder.m`:
- Around line 364-365: The assignment in the animated image initializer
improperly converts a signed int to NSUInteger: when setting _loopCount from
decoder->repetitionCount do the same guard used earlier for the
signed-to-unsigned issue: check if decoder->repetitionCount is negative and set
_loopCount to 0 in that case, otherwise safely cast/assign the non-negative
decoder->repetitionCount to the NSUInteger _loopCount; update the block that
sets _frameCount and _loopCount (referencing _frameCount, _loopCount, and
decoder->repetitionCount) to include this guard.
🧹 Nitpick comments (1)
SDWebImageAVIFCoder/Classes/SDImageAVIFCoder.m (1)
320-326: Pre-existing: deprecated quantizer fields should be migrated toquality/qualityAlpha.libavif 1.0.0 deprecated
avifEncoder'sminQuantizer,maxQuantizer,minQuantizerAlpha, andmaxQuantizerAlphafields —qualityandqualityAlphashould be used instead.Since this PR bumps the minimum libavif to 1.0.0, these fields will still compile but may produce warnings and could be removed in a future release. Consider migrating in a follow-up.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@SDWebImageAVIFCoder/Classes/SDImageAVIFCoder.m`:
- Around line 338-351: The local variable scaledSize is being recomputed and
shadowed inside the if block; remove the inner "CGSize scaledSize" declaration
so the block uses the previously computed scaledSize from the call to
[SDImageCoderHelper
scaledSizeWithImageSize:scaleSize:preserveAspectRatio:shouldScaleUp:] and then
set encoder->scalingMode accordingly (assign scale.horizontal/vertical using
scaledSize and original width/height). Also replace the strict asserts
(assert(scaledSize.width < width); assert(scaledSize.height < height);) with
non-fatal checks (use <= or remove the asserts) to avoid debug aborts due to
floating-point rounding—ensure the code handles equality gracefully before
assigning encoder->scalingMode.
🧹 Nitpick comments (1)
SDWebImageAVIFCoder/Classes/SDImageAVIFCoder.m (1)
317-321: Consider clampingcompressionQualityto [0.0, 1.0].If the caller passes an out-of-range value via
SDImageCoderEncodeCompressionQuality, the computedqualitycould exceedAVIF_QUALITY_BESTor go negative. A simpleMIN(MAX(..., 0), 1)clamp would make this defensive.🛡️ Suggested clamp
double compressionQuality = 1; if (options[SDImageCoderEncodeCompressionQuality]) { compressionQuality = [options[SDImageCoderEncodeCompressionQuality] doubleValue]; } - int quality = compressionQuality * (AVIF_QUALITY_BEST - AVIF_QUALITY_WORST); + compressionQuality = MAX(MIN(compressionQuality, 1.0), 0.0); + int quality = (int)(compressionQuality * (AVIF_QUALITY_BEST - AVIF_QUALITY_WORST));
| // Check if need to scale pixel size | ||
| CGSize scaledSize = [SDImageCoderHelper scaledSizeWithImageSize:CGSizeMake(width, height) scaleSize:maxPixelSize preserveAspectRatio:YES shouldScaleUp:NO]; | ||
| if (!CGSizeEqualToSize(scaledSize, CGSizeMake(width, height))) { | ||
| // Thumbnail Encoding | ||
| CGSize scaledSize = [SDImageCoderHelper scaledSizeWithImageSize:CGSizeMake(width, height) scaleSize:maxPixelSize preserveAspectRatio:YES shouldScaleUp:NO]; | ||
| assert(scaledSize.width < width); | ||
| assert(scaledSize.height < height); | ||
| avifScalingMode scale; | ||
| scale.horizontal.n = (int)scaledSize.width; | ||
| scale.horizontal.d = (int)width; | ||
| scale.vertical.n = (int)scaledSize.height; | ||
| scale.vertical.d = (int)height; | ||
| encoder->scalingMode = scale; | ||
| } |
There was a problem hiding this comment.
Shadowed variable scaledSize on line 342 duplicates the computation from line 339.
Inside the if block, CGSize scaledSize is re-declared with the exact same expression already evaluated three lines above. This shadows the outer scaledSize and is dead/redundant code. Remove the inner declaration so the block uses the already-computed value.
Additionally, the assert on lines 343–344 uses strict <. With floating-point rounding in aspect-ratio-preserving scaling, one dimension could conceivably equal the original, causing a debug-mode abort. Consider <= or removing the asserts in favor of a graceful early-return.
🔧 Proposed fix — remove duplicate declaration
// Check if need to scale pixel size
CGSize scaledSize = [SDImageCoderHelper scaledSizeWithImageSize:CGSizeMake(width, height) scaleSize:maxPixelSize preserveAspectRatio:YES shouldScaleUp:NO];
if (!CGSizeEqualToSize(scaledSize, CGSizeMake(width, height))) {
// Thumbnail Encoding
- CGSize scaledSize = [SDImageCoderHelper scaledSizeWithImageSize:CGSizeMake(width, height) scaleSize:maxPixelSize preserveAspectRatio:YES shouldScaleUp:NO];
- assert(scaledSize.width < width);
- assert(scaledSize.height < height);
+ assert(scaledSize.width <= width);
+ assert(scaledSize.height <= height);
avifScalingMode scale;
scale.horizontal.n = (int)scaledSize.width;
scale.horizontal.d = (int)width;🤖 Prompt for AI Agents
In `@SDWebImageAVIFCoder/Classes/SDImageAVIFCoder.m` around lines 338 - 351, The
local variable scaledSize is being recomputed and shadowed inside the if block;
remove the inner "CGSize scaledSize" declaration so the block uses the
previously computed scaledSize from the call to [SDImageCoderHelper
scaledSizeWithImageSize:scaleSize:preserveAspectRatio:shouldScaleUp:] and then
set encoder->scalingMode accordingly (assign scale.horizontal/vertical using
scaledSize and original width/height). Also replace the strict asserts
(assert(scaledSize.width < width); assert(scaledSize.height < height);) with
non-fatal checks (use <= or remove the asserts) to avoid debug aborts due to
floating-point rounding—ensure the code handles equality gracefully before
assigning encoder->scalingMode.
462a8bd to
6247df1
Compare
dreampiggy
force-pushed
the
feature/avif_loop_count
branch
from
6247df1 to
2b09dfc
Compare
1 participant
This requires libavif 1.0.0+
This close #68
Summary by CodeRabbit
New Features
Bug Fixes
Chores