Add maxSize downsampling option to APNGImage#151
Conversation
Large APNGs (e.g. 10240x7680) allocate a full-resolution RGBA canvas (~300 MB) plus output/previous CGImages per displayed image. Several in a list exceed the per-app memory limit on iPhone and get jetsam-killed. Add an optional `maxSize: CGSize?` to every `APNGImage` initializer. When set, the decoder derives a `renderScale = min(1, fit)` and the whole compositing pipeline runs in that scaled space: the canvas buffer, every cached frame, and all frame rects shrink to fit, bounding memory. Frames still decode natively and Core Graphics downsamples them as they are drawn into the smaller destination rect. `maxSize: nil` (the default) keeps the native size and produces byte-identical geometry to before. The cache-size estimate also uses the scaled dimensions so a downsampled loop can regain caching. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
@onevclaw @onevpaw /argue Review this PR for correctness and merge readiness. Look for possible regressions, hidden risk points, edge cases, compatibility issues, performance problems, missing tests, and behavior changes that may not be intended. For each finding, include the severity, the affected code path, why it matters, and a concrete recommendation. Also summarize what looks safe or well-implemented, then propose the next steps before merge: tests to run, manual verification, follow-up changes, or documentation updates. |
|
@plateaukao 你怎么开始转型干 iOS 了 😂.... 压力好大啊... |
There was a problem hiding this comment.
Pull request overview
This PR introduces an optional maxSize: CGSize? parameter to APNGImage creation so oversized APNGs can be downsampled during rendering/compositing, reducing the backing canvas and cached-frame memory footprint.
Changes:
- Add
maxSizeplumbing throughAPNGImage→APNGDecoderand derive arenderScaleplus render-space dimensions/rect transforms. - Update
APNGImageRendererto allocate and composite into a render-scaled CGContext and to scale frame regions/rects consistently. - Add unit tests validating downsampling behavior and “no upscaling” behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
Source/APNGKit/APNGDecoder.swift |
Adds renderScale/render-space sizing utilities and uses scaled dimensions for cache estimation. |
Source/APNGKit/APNGImageRenderer.swift |
Allocates the canvas at render dimensions and applies render-space rect scaling during dispose/blend/draw. |
Source/APNGKit/APNGImage.swift |
Exposes maxSize on public initializers and forwards it into the decoder. |
Tests/APNGKitTests/APNGImageRendererTests.swift |
Adds coverage for downsampling and “maxSize larger than native” behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // `previousOutputImage` is already at render scale, so crop it in render space too. | ||
| if let cropped = previousOutputImage.cropping(to: decoder.renderRect(displayingFrame.frameControl.cgRect)) { | ||
| outputBuffer.clear(displayingRegion) | ||
| outputBuffer.draw(cropped, in: displayingRegion) |
There was a problem hiding this comment.
Good catch — fixed in ff93124. The .previous crop rect is now integralized and clamped to the image bounds before cropping(to:):
let imageBounds = CGRect(x: 0, y: 0, width: previousOutputImage.width, height: previousOutputImage.height)
let cropRect = decoder.renderRect(displayingFrame.frameControl.cgRect).integral.intersection(imageBounds)
if let cropped = previousOutputImage.cropping(to: cropRect) { ... }so a fractional or slightly-out-of-bounds scaled rect can no longer make it return nil and break disposal.
| /// - maxSize: The maximum pixel size the image should be rendered at. If the image is larger than this size, it is | ||
| /// downsampled to fit, which bounds its memory footprint. Pass `nil` (the default) to render at native size. |
There was a problem hiding this comment.
Clarified the docs rather than changing size, in ff93124. Keeping size (and APNGImageView.intrinsicContentSize) at the native logical dimensions is intentional: maxSize is a memory / rendering-resolution control, not a layout change — the downsampled bitmap is scaled up to fill the same logical size when displayed. The doc block now states this explicitly:
This affects only the rendering resolution and memory use: the image's logical
size(and anAPNGImageView'sintrinsicContentSize) still reflects the native dimensions, and the downsampled output is scaled up to fill that size when displayed.
| /// - maxSize: The maximum pixel size the image should be rendered at. If the image is larger than this size, it is | ||
| /// downsampled to fit, which bounds its memory footprint. Pass `nil` (the default) to render at native size. |
There was a problem hiding this comment.
Clarified the docs rather than changing size, in ff93124. Keeping size (and APNGImageView.intrinsicContentSize) at the native logical dimensions is intentional: maxSize is a memory / rendering-resolution control, not a layout change — the downsampled bitmap is scaled up to fill the same logical size when displayed. The doc block now states this explicitly:
This affects only the rendering resolution and memory use: the image's logical
size(and anAPNGImageView'sintrinsicContentSize) still reflects the native dimensions, and the downsampled output is scaled up to fill that size when displayed.
| /// - maxSize: The maximum pixel size the image should be rendered at. If the image is larger than this size, it is | ||
| /// downsampled to fit, which bounds its memory footprint. Pass `nil` (the default) to render at native size. |
There was a problem hiding this comment.
Clarified the docs rather than changing size, in ff93124. Keeping size (and APNGImageView.intrinsicContentSize) at the native logical dimensions is intentional: maxSize is a memory / rendering-resolution control, not a layout change — the downsampled bitmap is scaled up to fill the same logical size when displayed. The doc block now states this explicitly:
This affects only the rendering resolution and memory use: the image's logical
size(and anAPNGImageView'sintrinsicContentSize) still reflects the native dimensions, and the downsampled output is scaled up to fill that size when displayed.
| /// - maxSize: The maximum pixel size the image should be rendered at. If the image is larger than this size, it is | ||
| /// downsampled to fit, which bounds its memory footprint. Pass `nil` (the default) to render at native size. |
There was a problem hiding this comment.
Clarified the docs rather than changing size, in ff93124. Keeping size (and APNGImageView.intrinsicContentSize) at the native logical dimensions is intentional: maxSize is a memory / rendering-resolution control, not a layout change — the downsampled bitmap is scaled up to fill the same logical size when displayed. The doc block now states this explicitly:
This affects only the rendering resolution and memory use: the image's logical
size(and anAPNGImageView'sintrinsicContentSize) still reflects the native dimensions, and the downsampled output is scaled up to fill that size when displayed.
| /// - maxSize: The maximum pixel size the image should be rendered at. If the image is larger than this size, it is | ||
| /// downsampled to fit, which bounds its memory footprint. Pass `nil` (the default) to render at native size. |
There was a problem hiding this comment.
Clarified the docs rather than changing size, in ff93124. Keeping size (and APNGImageView.intrinsicContentSize) at the native logical dimensions is intentional: maxSize is a memory / rendering-resolution control, not a layout change — the downsampled bitmap is scaled up to fill the same logical size when displayed. The doc block now states this explicitly:
This affects only the rendering resolution and memory use: the image's logical
size(and anAPNGImageView'sintrinsicContentSize) still reflects the native dimensions, and the downsampled output is scaled up to fill that size when displayed.
| // a minimum of one pixel so the canvas is never degenerate. | ||
| private func scaledLength(_ value: Int) -> Int { | ||
| guard renderScale < 1.0 else { return value } | ||
| return max(1, Int((CGFloat(value) * renderScale).rounded())) |
There was a problem hiding this comment.
Although not likely to be a real case, when value == 0, this method returns 1 as its result, which is not good.
There was a problem hiding this comment.
Fixed in ff93124. A zero length now stays zero — only positive lengths are clamped to a minimum of one pixel:
static func scaledLength(_ value: Int, scale: CGFloat) -> Int {
guard scale < 1.0, value > 0 else { return value }
return max(1, Int((CGFloat(value) * scale).rounded()))
}Also added a testScaledLength unit test covering the zero / clamp / rounding cases.
🤖 Argue 研判结果
Representative
SummaryThe PR is a good direction but not merge-ready yet: default behavior appears safe, but maxSize currently has unclear public sizing semantics, does not fully bound transient native frame decode memory, and needs stronger partial-frame/rounding coverage. All claimsReport
|
| // are computed inline rather than via `renderWidth`/`renderHeight` because `self` is not yet fully | ||
| // initialized here. | ||
| let scaledWidth = renderScale < 1.0 | ||
| ? max(1, Int((CGFloat(imageHeader.width) * renderScale).rounded())) |
There was a problem hiding this comment.
Maybe we should use the scaledLength just defined above.
There was a problem hiding this comment.
Done in ff93124. I made scaledLength a static method so it can be called during init (before self is fully initialized), and the cache estimate now reuses it instead of the duplicated arithmetic:
let scaledWidth = Self.scaledLength(imageHeader.width, scale: renderScale)
let scaledHeight = Self.scaledLength(imageHeader.height, scale: renderScale)| ? max(1, Int((CGFloat(imageHeader.width) * renderScale).rounded())) | ||
| : imageHeader.width | ||
| let scaledHeight = renderScale < 1.0 | ||
| ? max(1, Int((CGFloat(imageHeader.height) * renderScale).rounded())) |
There was a problem hiding this comment.
Same fix as above — the estimate now uses Self.scaledLength(...) for both width and height in ff93124.
| // Blend & Draw the new frame | ||
| // Blend & Draw the new frame. The frame's destination rectangle is scaled into render space; drawing the | ||
| // natively-decoded `nextFrameImage` into a smaller rectangle lets Core Graphics downsample it for us. | ||
| let frameRenderRect = decoder.renderRect(frame.normalizedRect(fullHeight: decoder.imageHeader.height)) |
我都看不懂,但 App crash 了,還是要來解決一下 XD 總不行老是叫後端不要亂送大圖來。 😅 |
|
这个确实没做,大体看起来很好。我之后接手继续改一下吧~ |
感謝~~ |
- scaledLength: keep a zero length at zero instead of clamping it up to one; extract a static core so it can be reused during init. - Reuse the static scaledLength for the cache-size estimate instead of duplicating the scaling arithmetic inline. - .previous disposal: integralize and clamp the render-space crop rect to the image bounds before CGImage.cropping(to:), which returns nil for a non-integral or out-of-bounds rectangle. - Clarify in the maxSize docs that it affects only rendering resolution and memory; the logical `size` / intrinsicContentSize stays native. - Add a unit test for scaledLength covering the zero, clamp, and rounding cases. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
Thanks for the review! Pushed
|
Motivation
Large APNGs render at full native resolution today — there is no downsampling or max-dimension guard. A single 10240×7680 image allocates a full-resolution RGBA canvas (~300 MB) plus the output/previous
CGImages per displayed image (~600 MB+ in steady state). A few such images in a list (e.g. aUICollectionView) exceed the per-app memory limit on iPhone and the app gets jetsam-killed.What this does
Adds an optional
maxSize: CGSize?to everyAPNGImageinitializer. When set, the decoder derives arenderScale = min(1, fit)and the entire compositing pipeline runs in that scaled space — the canvas buffer, every cached frame, and all frame rects shrink to fitmaxSize, which bounds memory.Key points:
CGContextis allocated atrenderWidth × renderHeight, and the dispose region,.previouscrop, and blend draw rects all pass through a singleCGAffineTransform(scaleX:y:)so frame boundaries stay coincident.CGContext.draw(_, in: scaledRect)resamples them as they're composited.maxSize: nilkeepsrenderScale = 1.0and produces byte-identical geometry to before.maxSizeis left at native size (never upscaled).maximumCacheSizecan regain caching.Caveat
Fractional frame offsets in partial-update APNGs can show minor edge-resampling artifacts when scaled; full-frame APNGs (the common case) downsample cleanly.
Tests
APNGImageRendererTests:testRenderDownsamplesToMaxSize(asserts a 100×100 image → 50×50 across multiple composited frames, plus a native-path regression guard) andtestMaxSizeLargerThanNativeDoesNotUpscale.swift buildclean; all 87 tests pass (85 existing + 2 new).🤖 Generated with Claude Code