-
Notifications
You must be signed in to change notification settings - Fork 511
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set bpp(bits per pixel) to 32 bit for GIF #511
Conversation
Please add a test. |
A test would definitely be good in this case. I wonder if the OS is working around our error in this case or we're getting lucky by using an unsupported but available format? |
Test added. |
Tests/PINAnimatedImageTests.swift
Outdated
let pinCachedAnimatedImage = PINGIFAnimatedImage(animatedImageData: animatedData as Data)! | ||
let bytesSize = UInt32((pinCachedAnimatedImage.image(at: 0, cacheProvider: nil)!.takeUnretainedValue() as CGImage).bytesPerRow) * pinCachedAnimatedImage.height | ||
|
||
XCTAssert(pinCachedAnimatedImage.bytesPerFrame <= bytesSize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be == ? I think this test succeeds with the old bytesPerFrame
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding a test! As @garrettmoon mentions, please ensure the test fails without the code change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, pinCachedAnimatedImage.bytesPerFrame
should always <=
bytesSize
, because CGBitmapContextCreate
may add padding and alignment when allocated memory, but in this sample url, seems we can use ==
. Let's try it. :)
🚫 CI failed with log |
You mind rebasing against master? I think I fixed a flakey test. |
@garrettmoon I triggered the CI, but seems all are pending. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the fix!
For 8bpc and RGB color space, it's always 32bits per pixel. We can also use
CGImageGetBytesPerRow(imageRef) * _height
.