Skip to content
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

Fix save() in the quartz backend #645

Merged
merged 3 commits into from
Mar 4, 2021
Merged

Fix save() in the quartz backend #645

merged 3 commits into from
Mar 4, 2021

Conversation

jwiggins
Copy link
Member

@jwiggins jwiggins commented Feb 28, 2021

save() doesn't work at all currently. Implement the buffer interface and use PIL.Image.frombuffer()

Implement the buffer interface and use PIL.Image.frombuffer()
@jwiggins
Copy link
Member Author

jwiggins commented Mar 1, 2021

We didn't have drawing tests for the quartz backend, so I added them. The drawing tests rely on save() to work at all.

@rahulporuri rahulporuri added this to In progress in Enthought OSS Q1 2021 Mar 1, 2021
@rahulporuri
Copy link
Contributor

Closing and opening the PR to trigger CI jobs. Not sure why but the PR has been stuck on Yellow for a while now i.e. "Waiting for status to be reported"

@rahulporuri rahulporuri closed this Mar 1, 2021
@rahulporuri rahulporuri reopened this Mar 1, 2021
@rahulporuri rahulporuri self-requested a review March 1, 2021 13:30
@jwiggins jwiggins requested review from aaronayres35 and removed request for rahulporuri March 3, 2021 14:33
Copy link
Contributor

@aaronayres35 aaronayres35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I can confirm the changes in this branch fixed the issues I was having with the quartz backend when running the benchmarking script.

kiva/quartz/ABCGI.pyx Outdated Show resolved Hide resolved

# Previously this method checked for red pixels. However this is
# not [currently] possible with the quartz backend because it writes
# out image with premultiplied alpha and none of its pixels are the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is from the if 'A' in mode: block in the above file correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not. The premultiplied alpha setting happens in the __init__ method of the graphics context. I couldn't find a way to have normal alpha, so I changed the test and added this note.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there are a few constraints here:

  • alpha_info can only be kCGImageAlphaPremultipliedLast or kCGImageAlphaPremultipliedFirst if save() is called on a graphics context.
  • From the "Supported Pixel Formats" table on this page, you can see that only 8, 16, and 32bit pixel formats are supported, so we can't even make a packed 24bit pixel buffer which would make alpha-free saving possible.

We can definitely implement saving RGB/BGR images, but it's a question of time allocation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thank you!

Comment on lines +1549 to +1558
def __releasebuffer__(self, Py_buffer *buffer):
""" When PyBuffer_Release is called on buffers from __getbuffer__.
"""
# Just deallocate the shape and strides allocated by __getbuffer__.
# Since buffer.obj is a referenced counted reference to this object
# (thus keeping this object alive as long a connected buffer exists)
# and we don't mutate `self.data` outside of __init__ and __dealloc__,
# we have nothing further to do here.
PyMem_Free(buffer.shape)
PyMem_Free(buffer.strides)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaronayres35 A bit more detail for future readers 😉

@jwiggins
Copy link
Member Author

jwiggins commented Mar 4, 2021

Thanks for the feedback

@jwiggins jwiggins merged commit 0815065 into master Mar 4, 2021
Enthought OSS Q1 2021 automation moved this from In progress to Done Mar 4, 2021
@jwiggins jwiggins deleted the fix/quartz-save-image branch March 4, 2021 13:29
@rahulporuri rahulporuri moved this from Done to Sprint 4 : March 1 - March 12 2021 in Enthought OSS Q1 2021 Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Enthought OSS Q1 2021
Sprint 4 : March 1 - March 12 2021
Development

Successfully merging this pull request may close these issues.

None yet

3 participants