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

Support CompressedImage and Stamped types #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

awesomebytes
Copy link
Contributor

Following up #3 but only adding CompressedImage and the XXXXStamped types.

@@ -80,6 +80,9 @@ def image_to_numpy(msg):
data = data[...,0]
return data

@converts_to_numpy(CompressedImage)
def compressed_image_to_numpy(msg):
return np.fromstring(msg.data, np.uint8)
Copy link
Owner

Choose a reason for hiding this comment

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

This should be np.frombuffer - np.fromstring is deprecated in this use (I know this because I deprecated it it!)

Copy link
Owner

@eric-wieser eric-wieser Apr 30, 2018

Choose a reason for hiding this comment

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

Actually, this just feels like the wrong thing to do in general. If conversion from CompressedImage is going to exist at all, it should do the full png/jpeg/etc decoding using the python imageio module.

That might be a nasty dependency to pull in, so I'd rather not open that can of worms in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About frombuffer, done, commited.

About the CompressedImage, the reality of compressed images is not very nice. That transformation works for all jpg. For png one can do exactly the same but must skip the first 12 bits of a header. For CompressedImages that contain a depth image (there is no way to know) there is one step more even... So that would be a bit of a mess to figure out all the cases.
I think this implementation helps, better than none.

Copy link
Owner

Choose a reason for hiding this comment

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

For png one can do exactly the same but must skip the first 12 bits of a header

Huh? What do you mean by this?

So that would be a bit of a mess to figure out all the cases.

Then throw an error in the cases that aren't supported.

I think this implementation helps, better than none.

I disagree - the point of this library was to do the "one obvious thing". The obviously thing for a comrpessed image is to return a 3D RGB array. Returning a 1d sequence of bytes is not what I would expect at all.

Copy link
Owner

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

One small complaint

@eric-wieser
Copy link
Owner

eric-wieser commented Apr 30, 2018

I don't like CompressedImage at all here - it's doing something dangerously different from expectation, and it's caused you to try and roll your own PNG parser just to test a rosbag!

At the very least, I would consider CompressedImage a can of worms - so I would like to merge the rosbag (#6) and Stamped (this PR) changes without mixing that in - and discuss it further in a separate PR.

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