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 compressed Images messages in python for kinetic #132

Merged
merged 1 commit into from Jun 13, 2016

Conversation

Projects
None yet
2 participants
@talregev
Copy link
Contributor

commented May 19, 2016

  • Add cv2_to_comprssed_imgmsg: Convert from cv2 image to compressed image ros msg.
  • Add comprssed_imgmsg_to_cv2: Convert the compress message to a new image.
  • Add compressed image tests.
  • Add time to msgs (compressed and regular).

@talregev talregev changed the title Support compressed Images messges in python Support compressed Images messages in python for kinetic May 20, 2016

import numpy as np

str_msg = cmprs_img_msg.data
dtype, n_channels = self.cvtype2_to_dtype_with_channels(cv2.CV_8UC1)

This comment has been minimized.

Copy link
@vrabaud

vrabaud May 24, 2016

Contributor

why cv2.CV_8UC1 ? That is a bit confusing, you can use the proper numpy dtype in the line below.

This comment has been minimized.

Copy link
@talregev

talregev May 27, 2016

Author Contributor

I don't know what is the proper numpy dtype for cv2.CV_8UC1.
This code came from c++:
https://github.com/ros-perception/vision_opencv/blob/kinetic/cv_bridge/src/cv_bridge.cpp#L431
When you find example in the internet how to convert jpg data to image.
For me it more understandable like this.

This comment has been minimized.

Copy link
@vrabaud

vrabaud Jun 12, 2016

Contributor

Just:

buf = np.ndarray(shape=(1, len(str_msg)), dtype=np.uint8, buffer=cmprs_img_msg.data)
This function returns a sensor_msgs::Image message on success, or raises :exc:`cv_bridge.CvBridgeError` on failure.
"""
import cv2, rospy, time

This comment has been minimized.

Copy link
@vrabaud

vrabaud May 24, 2016

Contributor

the bad thing about that is that it adds a dependency on rospy. I would actually not fill the time member (like done in the non-compressed version) and let the user do it by hand.

This comment has been minimized.

Copy link
@talregev

talregev May 27, 2016

Author Contributor

Why is a bad thing to adds a dependency on rospy?
For me as a user, I expect from using this methods (the compressed and the regular),
that they fully add all the properties with correct data, including time.
If user want to change time, he can do it after using this method.
When I use the first time in the regular method: cv2_to_imgmsg
I surprised that it didn't fill in the time. and i need to fill it manually.
I add time also to regular method.
But if you still not agree with me, I will change it, without time.

This comment has been minimized.

Copy link
@vrabaud

vrabaud Jun 12, 2016

Contributor

rospy is a huge dependency, and just to convert a Python time.
Having now as a default does not make sense: now() is different from the time at which you acquired the image so it seems like a dangerous default.
The API could take a time as an argument (that would default to None and therefore (0,0)) but I would suggest this as a later PR as it would need to be added to the non-compressed API too.

import sensor_msgs.msg
from cv_bridge import CvBridge

class source:

This comment has been minimized.

Copy link
@vrabaud

vrabaud May 24, 2016

Contributor

should be Source

This comment has been minimized.

Copy link
@talregev

talregev May 27, 2016

Author Contributor

ok. we need to change also for all tests. I copy the tests from regular tests.

merge regular tests and compressed together. change to Source as well

rospy.core.signal_shutdown(outcome)

if __name__ == '__main__':
main(sys.argv)

This comment has been minimized.

Copy link
@vrabaud

vrabaud May 24, 2016

Contributor

a small description of what it does would be nice.

This comment has been minimized.

Copy link
@talregev

talregev May 27, 2016

Author Contributor

ok. need to add description to regular test as well:
opencv_tests/nodes/broadcast.py

merge regular tests and compressed together. add a small description as well.

@vrabaud

This comment has been minimized.

Copy link
Contributor

commented May 24, 2016

ok, so overall pretty good. For the nodes, how about making the old nodes work with both compressed and uncompressed ? That would remove some copy/paste code.
Also, it needs a test. I would suggest to add compression tests to: https://github.com/ros-perception/vision_opencv/blob/kinetic/cv_bridge/test/enumerants.py

@vrabaud vrabaud referenced this pull request May 24, 2016

Closed

add cv2_to_cmprsimgmsg #131

@talregev

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2016

For the nodes, how about making the old nodes work with both compressed and uncompressed ? That would remove some copy/paste code.

I think on this in the beginning, and i ask my self what if some one change bad thing in the regular or the compressed image implantation. It good we have separate tests to check which functionality it not working. But you right we need to avoid copy/past thing. so maybe we can share identical code for both tests (regular and compressed). you have idea how to do it?

merge regular tests and compressed together.

Also, it needs a test. I would suggest to add compression tests to: https://github.com/ros-perception/vision_opencv/blob/kinetic/cv_bridge/test/enumerants.py

ok

Add test in enumerants.py

@talregev

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2016

@vrabaud
All the changes i made in indigo branch, because in that branch there is automatic tests.
When you can, please respond at my comments, then we can proceed.

@talregev

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2016

@vrabaud I also make the changes here.
Can you check?

@vrabaud vrabaud closed this Jun 12, 2016

@vrabaud vrabaud reopened this Jun 12, 2016

@vrabaud vrabaud added the in progress label Jun 12, 2016

@@ -33,11 +34,13 @@

if __name__ == '__main__':

#TODO add this file in the repository and make it relative to this python script. (not all people will run this from linux)

This comment has been minimized.

Copy link
@vrabaud

vrabaud Jun 12, 2016

Contributor

indentation please use spaces and not tabs.

haarfile = '/usr/share/opencv/haarcascades/haarcascade_frontalface_alt.xml'

parser = OptionParser(usage = "usage: %prog [options]")
parser.add_option("-c", "--cascade", action="store", dest="cascade", type="str", help="Haar cascade file, default %default", default = haarfile)
parser.add_option("-t", "--topic", action="store", dest="topic", type="str", help="Topic to find a face on, default %default", default = '/camera/rgb/image_raw')
parser.add_option("-ct", "--ctopic", action="store", dest="ctopic", type="str", help="Compressed topic to find a face on, default %default", default = '/camera/rgb/image/compressed')

This comment has been minimized.

Copy link
@vrabaud

vrabaud Jun 12, 2016

Contributor

indentation

rospy.init_node('rosfacedetect')
rospy.Subscriber(options.topic, sensor_msgs.msg.Image, detect_and_draw)
rospy.Subscriber(options.ctopic, sensor_msgs.msg.CompressedImage, compressed_detect_and_draw)

This comment has been minimized.

Copy link
@vrabaud

vrabaud Jun 12, 2016

Contributor

indentation

@@ -71,12 +75,12 @@ def spin(self):
ball_yv = -ball_yv

self.pub.publish(cvb.cv2_to_imgmsg(cvim))

self.pub_compressed.publish(cvb.cv2_to_compressed_imgmsg(cvim))

This comment has been minimized.

Copy link
@vrabaud

vrabaud Jun 12, 2016

Contributor

indentation

@vrabaud

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2016

Sorry, I was out of town.
We now have a Travis job so please fix it. And please do not use tabs, cf http://wiki.ros.org/PyStyleGuide#Coding_Style
Thx.

@talregev

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2016

Hi @vrabaud,
Thanks for the feedback, I fix all the issues you tell me.
But still the test enumerants.py doesn't pass. it fell on line 61.
After compressed and de-compressed the image (from jpg) the shape it not the same.
What should we do?

@vrabaud

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2016

Well, that's why we have tests :) If they don't pass, there are errors in your code.

str_msg = cmprs_img_msg.data
buf = np.ndarray(shape=(1, len(str_msg)),
dtype=np.uint8, buffer=cmprs_img_msg.data)
im = cv2.imdecode(buf, cv2.IMREAD_COLOR)

This comment has been minimized.

Copy link
@vrabaud

vrabaud Jun 12, 2016

Contributor

you should not assume it is a color image. Use cv2.IMREAD_ANYCOLOR no ?

This comment has been minimized.

Copy link
@vrabaud

vrabaud Jun 12, 2016

Contributor

that is probably what's making the test fail.

This comment has been minimized.

Copy link
@talregev

talregev Jun 12, 2016

Author Contributor

I will try.

import cv2
import numpy as np

str_msg = cmprs_img_msg.data

This comment has been minimized.

Copy link
@vrabaud

vrabaud Jun 12, 2016

Contributor

trailing white spaces, please use a linter (e.g. pylint)

This comment has been minimized.

Copy link
@talregev

talregev Jun 12, 2016

Author Contributor

Do you have online tool for checking?
This tool is ok?
http://pep8online.com/
It give me many errors.

This comment has been minimized.

Copy link
@vrabaud

vrabaud Jun 12, 2016

Contributor

it's more for later contributions, what you have right now is fine, just be careful about trailing spaces.

cmprs_img_msg.data = np.array(cv2.imencode(ext_format, cvim)[1]).tostring()
except:
raise CvBridgeError("format specified as %s is not recognized" % dst_format)

This comment has been minimized.

Copy link
@vrabaud

vrabaud Jun 12, 2016

Contributor

trailing white spaces.

@vrabaud

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2016

Also, be careful with your test, you cannot encode to jpg with 2 channels.

try:
cmprs_img_msg.data = np.array(cv2.imencode(ext_format, cvim)[1]).tostring()
except:
raise CvBridgeError("format specified as %s is not recognized" % dst_format)

This comment has been minimized.

Copy link
@vrabaud

vrabaud Jun 12, 2016

Contributor

you should probably keep the OpenCV error message.

else:
self.assert_(original.shape == newimg.shape)
self.assert_(original.shape == newimg2.shape)

This comment has been minimized.

Copy link
@vrabaud

vrabaud Jun 12, 2016

Contributor

Just have another set of for loops, it will make things easier to track. In that new set, just have the number of channels in ([],1,3,4) (ignore two for jpg).
Also, you are erasing the first set of fmts so that's why you should have a second set (I will fix the tests to use fmts as they don't for now).

This comment has been minimized.

Copy link
@talregev

talregev Jun 12, 2016

Author Contributor

Change all basic linter errors (also for previous tests).

Just have another set of for loops, it will make things easier to track. In that new set, just have the number of channels in ([],1,3,4) (ignore two for jpg).

Done.

Also, you are erasing the first set of fmts so that's why you should have a second set (I will fix the tests to use fmts as they don't for now).

Done.

change from IMREAD_COLOR to IMREAD_ANYCOLOR.

Still enumerants.py are falling.
Maybe if you compressed an image and de-compressed it not identical?
Because I check the code. it send and receive compressed images. The pics are ok for
the eye, but maybe the shape it not the same?

@vrabaud Thanks for all feedbacks.

@talregev

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2016

@vrabaud now tests are pass.
I am not sure how to test 4 channels.
If there is no more comments, I will squash the commits.

@vrabaud

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2016

Not all formats support 4 channels. Jpg and BMP do not. PNG does. Maybe you can skip four channels. 1 and 3 are enough for now.

@vrabaud

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2016

Oh, and please squash your commits once you are satisfied with the result. Thx !

@talregev

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2016

I squash them!
@vrabaud

"""
Convert a sensor_msgs::CompressedImage message to an OpenCV :cpp:type:`cv::Mat`.
:param img_msg: A :cpp:type:`sensor_msgs::Image` message

This comment has been minimized.

Copy link
@vrabaud

vrabaud Jun 13, 2016

Contributor

please change to cmprs_img_msg and comment properly.

This comment has been minimized.

Copy link
@talregev

talregev Jun 13, 2016

Author Contributor

please change to cmprs_img_msg and comment properly.

ok

from cv_bridge.boost.cv_bridge_boost import cvtColor2

try:
res = cvtColor2(im, "bgr8", desired_encoding)

This comment has been minimized.

Copy link
@vrabaud

vrabaud Jun 13, 2016

Contributor

Actually, we do not know the original encoding of the image. Let's say it is a JPG with 3 channels. Maybe it is rgb8 or bgr8, or just 8UC3. I'd say you should just remove the desired_encoding and assume it is always passthrough. People can then have their logic outside of the function to convert or not.

This comment has been minimized.

@vrabaud

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2016

Almost done, I just spotted the logic problem when reading from compressed, let me know what you think.

Support compressed Images messages in python for indigo
- Add cv2_to_comprssed_imgmsg: Convert from cv2 image to compressed image ros msg.
- Add comprssed_imgmsg_to_cv2:   Convert the compress message to a new image.
- Add compressed image tests.
- Add time to msgs (compressed and regular).

add enumerants test for compressed image.
merge the compressed tests with the regular ones.
better comment explanation. I will squash this commit.

Fix indentation
fix typo mistage: from .imgmsg_to_compressed_cv2 to .compressed_imgmsg_to_cv2.

remove cv2.CV_8UC1
remove rospy and time depndency.

change from IMREAD_COLOR to IMREAD_ANYCOLOR.

- make indentaion of 4.
- remove space trailer.
- remove space from empty lines.
- another set of for loops, it will make things easier to track. In that new set,  just have the number of channels in ([],1,3,4) (ignore two for jpg). from: #132 (comment)
- keep the OpenCV error message. from: #132 (comment)

add debug print for test.

add case for 4 channels in test.

remove 4 channels case from compressed test.

add debug print for test.

change typo of format.

fix typo in format. change from dip to dib.
change to IMREAD_ANYCOLOR as python code. (as it should).

rename TIFF to tiff

Sperate the tests one for regular images and one for compressed.

update comment
@talregev

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2016

@vrabaud

Almost done, I just spotted the logic problem when reading from compressed, let me know what you think.

I say to keep it that way, because most of the case jpg is in BGR, and to keep the same api in c++.
Also, when you recive jpg picture (or other compressed image) in rviz, it assume it BGR (it has no way to know) and display it as BGR. that why before i send compressed image, i convert it to BGR first.

@vrabaud

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2016

Hmm, indeed, that would be against the C++ API.
Then again, it is wrong: what if you send an RGBA PNG ? The code breaks :)

@vrabaud vrabaud merged commit ca70f5a into ros-perception:kinetic Jun 13, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vrabaud vrabaud removed the in progress label Jun 13, 2016

vrabaud added a commit that referenced this pull request Jun 14, 2016

Support compressed Images messages in python for indigo
- Add cv2_to_comprssed_imgmsg: Convert from cv2 image to compressed image ros msg.
- Add comprssed_imgmsg_to_cv2:   Convert the compress message to a new image.
- Add compressed image tests.
- Add time to msgs (compressed and regular).

add enumerants test for compressed image.
merge the compressed tests with the regular ones.
better comment explanation. I will squash this commit.

Fix indentation
fix typo mistage: from .imgmsg_to_compressed_cv2 to .compressed_imgmsg_to_cv2.

remove cv2.CV_8UC1
remove rospy and time depndency.

change from IMREAD_COLOR to IMREAD_ANYCOLOR.

- make indentaion of 4.
- remove space trailer.
- remove space from empty lines.
- another set of for loops, it will make things easier to track. In that new set,  just have the number of channels in ([],1,3,4) (ignore two for jpg). from: #132 (comment)
- keep the OpenCV error message. from: #132 (comment)

add debug print for test.

add case for 4 channels in test.

remove 4 channels case from compressed test.

add debug print for test.

change typo of format.

fix typo in format. change from dip to dib.
change to IMREAD_ANYCOLOR as python code. (as it should).

rename TIFF to tiff

Sperate the tests one for regular images and one for compressed.

update comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.