GPUImagePicture: check that the memory layout is valid #1298

Merged
merged 1 commit into from Nov 11, 2013

Conversation

Projects
None yet
2 participants
Contributor

karlvr commented Nov 10, 2013

Hi Brad,

I'm sorry, my previous GPUImagePicture pull request overlooked CGImages that have a padded row layout (i.e. row length > width * 4). I have added some additional checks for row length, bits per pixel and bits per component. These should all fit together to this time give us a fast path in the event of CGImages without padding and in the correct format.

See the discussion in this commit about this issue:
b662c2e#commitcomment-4557862

I have tested this with images that both fail this new check, and were previously used incorrectly, and images that pass it and continue to work correctly. I haven't yet heard back from the @keremerkan, but hopefully he'll be able to confirm the fix too.

Best regards,
Karl

@karlvr karlvr GPUImagePicture: check that the memory layout is valid for using the …
…CGImage directly in GL

This corrects the fault introduced by my previous commit enabling CGImages to be used directly in GL.
dd82b99
Owner

BradLarson commented Nov 11, 2013

Yeah, this was probably one of the cases I heard people complaining about when researching this approach. If we can catch this, then that should handle that class of issues.

I may also need to revisit some of the shader code, because some of the shaders expect premultiplied alpha, which won't be correct with images pulled in via this. I want to completely eliminate premultiplied alpha from the inputs, because my shaders don't output premultiplied alpha (nor do I want them to), so chaining filters that expect that kind of input cause problems. However, Core Graphics doesn't let you draw to a bitmap context that doesn't use premultiplied alpha, so the only way to do this would be to use a path like this that directly accesses the bitmap data and by scaling the bitmap data for cases that need it outside of Core Graphics (perhaps with Accelerate).

Anyway, thanks for identifying this and fixing it.

BradLarson merged commit a863966 into BradLarson:master Nov 11, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment