Add export methods for PIL and popular array libraries to the ScreenShot object#480
Draft
jholveck wants to merge 15 commits into
Draft
Add export methods for PIL and popular array libraries to the ScreenShot object#480jholveck wants to merge 15 commits into
jholveck wants to merge 15 commits into
Conversation
Currently, this shares memory with the .raw bytearray in some cases. We should consider whether we want to change that to always copy, or just wait until we have the full shared buffer support. This implementation always does the array manipulation on the CPU. For PyTorch or TensorFlow, the user is likely to want to do the array manipulation on the GPU for performance. This is very easy to implement.
Owner
|
Python 3.9 will be dropped in the next major release, so no need to take care of test failures. I think we can do a release without this patch, right? |
Contributor
Author
|
Certainly! |
Python 3.9 doesn't define TypeAlias, and it's not required in this case, so we can just remove it.
Contributor
Author
|
We can still wait to merge this, but the fix for Python 3.9 was simple, so I went ahead and committed it. |
Owner
|
You can revert the Python 3.9 fix commit ;) |
Contributor
Author
|
I've updated this a bit, but should probably review it before committing. It's been a while! |
The reverted change was from a temporary test, but I accidentally included it in the commit. It's not relevant to the issue at hand.
Contributor
Author
|
This is on hold pending a resolution to #536, which will affect how we should document these methods and their sharing semantics. |
The docstrings weren't all in Sphinx format, and could use a little clarification in places. TensorFlow itself allows NumPy dtypes and DataType enums, so expand to_tensorflow to accept those too.
The main point of this commit is to be explicit about the memory-sharing semantics: mostly, that we don't guarantee memory sharing one way or another. More to the point, that if they modify the pixel data in one of the exported objects, it may or may not affect the other exported objects. Memory sharing isn't expected to affect most users, since they are unlikely to access the pixel data with two different exported objects, but it's best to be explicit. To have a good place to put the memory-sharing discussion, I added a new section to the usage.rst file, listing the ways to access the pixel data. This is also where I documented the NumPy array interface protocol, which previously didn't show up in our Sphinx docs. (It might have been in the docs for 10.1, which didn't use autodoc.) I also put a discussion of the alpha channel in this new section: one thing I keep forgetting is to delete the alpha channel when using Matplotlib, and wondering why it's blank. TODO: We probably should document `grab` in the usage.rst file for the new section to make sense. In the process, I've also added intersphinx links to libraries like PIL and NumPy, so that our Sphinx docs will link to them. While checking that, I fixed a few other references (like referring to bytes with :py:type:, when it's indexed under :py:class:).
Contributor
Author
|
My last commit added a lot to the docs. There's a couple more things I want to add to the docs before closing this, even if they're somewhat out of scope for this PR. |
Owner
|
Great! The documentation is very very good since you both started to work on the project with @halldorfannar 🤟 Go ahead for out-of-scope commits, I'll merge when you're ready. |
Doc fixes. Speed up converting to a NumPy array in RGB order. That sort of thing.
While I was writing this, I realized something else. We may want to change the to_torch and to_tensorflow methods to accept device and stream arguments, and do the channel and layout shuffling on the GPU instead of the CPU. This wouldn't require us to natively support CUDA or anything like that; we'd just be using PyTorch / TensorFlow's existing CUDA support, which is easy. This demo still (as before) shows the GPU-focused way to do the channel and layout shuffling, presented as an alternative to using to_torch. A quick test shows about 107 ms / frame (to_torch) vs. about 62 ms / frame (GPU-focused routine), and that's including the CNN itself too. So there's a very significant benefit there. (That's not quite a fair comparison, since the GPU-focused routine is exactly what this program needs, but not so it'd be a big difference.)
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.
Currently, this shares memory with the .raw bytearray in some cases. We should consider whether we want to change that to always copy, or just wait until we have the full shared buffer support.
This implementation always does the array manipulation on the CPU. For PyTorch or TensorFlow, the user is likely to want to do the array manipulation on the GPU for performance. This is very easy to implement; I just wanted to get this draft out for now.
Changes proposed in this PR
Fixes #478
./check.shpassed