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

Returning frames as PIL images #37

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Returning frames as PIL images #37

wants to merge 11 commits into from

Conversation

timmarkhuff
Copy link
Contributor

Previously, FrameGrab has always returned numpy arrays. This PR changes FrameGrab to return PIL images instead.

numpy arrays can be challenging because they don't contain any metadata about color space, and therefore it's easy to mix up RGB and BGR. PIL images contain a mode attribute that fixes this usability problem.

We believe users will appreciate receiving PIL images. If they need a numpy array, they can easily convert.

This will be a breaking change, and I'm not sure if there is a way to avoid this. We could leave the old grab method the way it is, returning numpy arrays, and then create a new method called grab_frame (or something similar) that returns PIL images, but I don't believe this works towards our ultimate goal of deprecating numpy images; people will gravitate towards grab because it's simpler and probably the first thing autocomplete serves them. Maybe we could log a warning when people use grab and deprecate it in the future? However this leaves us with grab_frame as our main method, which is not as elegant as just grab.

@timmarkhuff timmarkhuff marked this pull request as draft March 22, 2024 20:58
@timmarkhuff timmarkhuff marked this pull request as ready for review March 22, 2024 21:16
Copy link
Member

@robotrapta robotrapta left a comment

Choose a reason for hiding this comment

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

The more I think about this the more I think we need to do this in a backwards compatible way. I think instead we just offer a grabimg() method which returns a PIL image. Too much stuff would break.

To deprecate, I think the best way is to update the docs - aren't there any in here we should update for this? Or are they all in the SDK?

We could more aggressively deprecate - keep the _grab_impl method (maybe call it _grab_np instead?) and have the grab() method issue a warning saying "please switch to grab_img instead" but I don't know that really does much except annoy us.

@timmarkhuff
Copy link
Contributor Author

ns a PIL image. Too much stuff would break.

Also aren't there some docs we should update for this? I think we should recommend to people to star tusing the grabimg method.

Sure, I can update the docs. Should we log a warning when someone uses grab and then eventually deprecate it? Or maybe we just offer two methods?

@robotrapta
Copy link
Member

I think we just offer two, and recommend the PIL image as being simple and more reliable.


cv2.destroyAllWindows()
for camera_name, grabber in grabbers.items():
frame = grabber.grabimg()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updating to use grabimg as that is our preferred method

@@ -13,15 +11,8 @@

grabber = FrameGrabber.create_grabber(config)

while True:
frame = grabber.grab()
frame = grabber.grabimg()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changing to grabimg again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants