Serialize animated-image frame decodes; refuse damaged frames; fix fr… - #656
Merged
andyfinnell merged 4 commits intoJul 29, 2026
Merged
Conversation
…ame-failure bookkeeping CGImageSource is not safe for concurrent lazy decodes of the same source (despite the optimistic '*seems* immutable' comments), and PINCachedAnimatedImage reaches imageAtIndex:cacheProvider: concurrently from its caching queue, its init-time warmup dispatch, and coverImage callers. The racing decode crashes inside ImageIO (GIFReadPlugin::copyImageBlockSetImp / GIFBufferInfo memmove). - GIF, APNG, and WebP now serialize all ImageIO calls on the shared source — including CGImageSourceGetStatusAtIndex, which advances parser state — behind a per-image lock. - Refuse affirmatively damaged frames (kCGImageStatusInvalidData / kCGImageStatusUnexpectedEOF) before attempting creation. Deliberately narrow: kCGImageStatusIncomplete (trailer-less but renderable GIFs, common in the wild) still decodes. - A frame that fails to decode previously left _frameRenderCount held and its index parked in _cachedOrCachingFrames forever: playbackReady never fired and the frame was never retried. Failures now release their render slot. - pin_decodedImageRefWithCGImageRef: returned the borrowed input ref when CGBitmapContextCreate fails (memory pressure); animated callers then released it and returned the dangling pointer. The fallback now matches the success path's +0 autoreleased contract. Also guard NULL input. Fixes a long-standing top crasher in a large production app (EXC_BAD_ACCESS at PINImage+DecodedImage's CGContextDrawImage during GIF frame decode).
CI was failing at the first xcrun/xcodebuild invocation with "xcrun: error: missing DEVELOPER_DIR path". Root cause: GitHub's macos-latest label now resolves to the macOS 26 runner image, which only ships Xcode 26.x — the pinned Xcode (and several other things the build relied on) no longer exist on the image. Changes, following TextureGroup/Texture's CI setup: - DEVELOPER_DIR now points at Xcode_26.5.0.app (the image default; see the runner-images manifest linked in the workflow comment). - Makefile simulator destination bumped from "iPhone 16,OS=18.5" to "iPhone 17" with a version-less iphonesimulator SDK, since neither the iPhone 16 device nor the iOS 18.5 runtime exist under Xcode 26. Leaving the SDK unversioned means it floats with the selected Xcode instead of breaking on every image bump. - xcpretty replaced with xcbeautify: the runner images stopped preinstalling the xcpretty gem (macOS 26 ships only xcbeautify 3.2.1), so the "xcodebuild | xcpretty" pipes would fail with command-not-found. Error propagation is unchanged — the Makefile's "SHELL=/bin/bash -o pipefail" still surfaces xcodebuild failures through the pipe. - Removed the platform matrix from the analyze/test jobs; nothing ever referenced matrix.platform (the Makefile owns the destination). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
andyfinnell
approved these changes
Jul 29, 2026
Review feedback on pinterest#656: the project convention is PINRemoteLock (PINCachedAnimatedImage et al), not bare NSLock. Behavior is unchanged — PINRemoteLock wraps a pthread_mutex in release builds and an NSLock in debug builds — but this keeps house style and gives the locks debuggable names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
testQOS asserts exact pixel widths (345/564/736) on live pinimg CDN derivatives, and the CDN periodically re-encodes them a pixel off nominal — it currently serves 344/563/735, failing CI (and commit 89f4be7 was the same chase last time). The test only needs to distinguish small/medium/large, so compare with XCTAssertEqualWithAccuracy and a ±2px tolerance instead of chasing exact values. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
andyfinnell
approved these changes
Jul 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…ame-failure bookkeeping
CGImageSource is not safe for concurrent lazy decodes of the same source (despite the optimistic 'seems immutable' comments), and PINCachedAnimatedImage reaches imageAtIndex:cacheProvider: concurrently from its caching queue, its init-time warmup dispatch, and coverImage callers. The racing decode crashes inside ImageIO
(GIFReadPlugin::copyImageBlockSetImp / GIFBufferInfo memmove).
Fixes a long-standing top crasher in a large production app (EXC_BAD_ACCESS at PINImage+DecodedImage's CGContextDrawImage during GIF frame decode).