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

Dev/image encoding param #10

Closed
wants to merge 10 commits into from

Conversation

kfieldho
Copy link

@kfieldho kfieldho commented Dec 1, 2016

This branch provides the ability to specify the OpenCV encoding for the processing of of Image topics (ours are in bgr8 format). Also includes a node configuration example in the marv.conf.in template.

chaoflow and others added 10 commits October 7, 2016 23:17
Authors:
- Marko Durkovic <marko@ternaris.com>
- Florian Friesdorf <florian@ternaris.com>
Allows other OpenCV image encodings
This provides at least a bit of a clue regarding the configuration
of nodes, complementing the already existing examples for the
various "detail" views
Copy link

@NikolausDemmel NikolausDemmel left a comment

Choose a reason for hiding this comment

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

I'm not quite Sure why this would Matter. I'm assuming that the passed encoding is for the resulting opencv image encoding. But that temporary object is then written to a file with jpeg encoding, so the intermediate encoding does bot really matter, does it?

@@ -59,7 +60,7 @@ def camera_frames(bagmeta, messages, image_width=320, max_frames=50):
continue

try:
img = imgmsg_to_cv2(msg, "rgb8")
img = imgmsg_to_cv2(msg, encoding)
Copy link

Choose a reason for hiding this comment

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

If I'm not mistaken, then the argument you specify here is the "target" encoding, why do you want to change that?
The encoding that the ROS image has is already part of the Image msg and should be already considered by the conversion function....

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, now that you mention it, that makes sense. That said, the existing code, when it deals with our bag files, generates incorrect images (blueish grass etc.) Adding this code and switching to "bgr8" fixed the problem. Though now I'm uncertain why.

Choose a reason for hiding this comment

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

I would double check that your data is indeed encoded bgr8 and that the encoding string is also set correctly. Otherwise, can you provide a sample bag file?

Choose a reason for hiding this comment

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

All video images in my Marv instance show a R/B and B/R confusion as well. Using bgr8 as target encoding makes sense to me. OpenCV uses BGR encoding internally and methods like cv2.imwrite want BGR encoded images.

Copy link

@NikolausDemmel NikolausDemmel Jan 2, 2018

Choose a reason for hiding this comment

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

@Earthwings, do you use the latest release? If so, I would check which code path is taken here https://github.com/ternaris/marv-robotics/blob/master/marv_robotics/cam.py#L124-L130 and open a new issue in case the bags look fine e.g. in image_view or rviz.

Choose a reason for hiding this comment

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

@NikolausDemmel Works indeed fine in 17.11.
Unfortunately I'm running into multiple other bugs with that release, but that's off topic here.

@chaoflow
Copy link
Member

Thank you everybody, especially @kfieldho for bringing this up! With eb8d466 we switched to bgr8 by default now. Probably, a change in opencv caused this to be an issue.

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.

5 participants