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.

Sign up for GitHub

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

Jump to bottom

Some fixes to work #2

Merged
merged 2 commits into from
Aug 11, 2017
Merged

Some fixes to work #2

merged 2 commits into from
Aug 11, 2017

Conversation

furushchev
Copy link
Contributor

  • requires mono8 image as zbar::Image
  • fix memory leak

paulbovbel
paulbovbel requested changes Jul 14, 2017
View reviewed changes
Copy link
Member

@paulbovbel paulbovbel left a comment

Choose a reason for hiding this comment

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

The image encoding should probably be a rosparam rather than a fixed string. If you'd like the default to be something other than mono8, happy to change it given some rationale.

@furushchev
Copy link
Contributor Author

furushchev commented Jul 14, 2017 •
edited
Loading

@paulbovbel Thanks for reviewing!
Well, it's ok to support rosparam to change destination encoding, but zbar::Image with parameter Y800 only supports mono8 image from ROS.
If any images whose encoding is NOT mono8 are given to zbar::Image, ImageScanner returns nothing (even no error shows up..)

@paulbovbel
Copy link
Member

Ah, good call, just looked up what Y800 is. I've seen this configuration work, but it's possible suboptimal.

@furushchev
Copy link
Contributor Author

@paulbovbel Thanks for approve. Any other place to be fixed?

@furushchev
Copy link
Contributor Author

kindly ping to maintainer

@mikepurvis
Copy link
Member

Looks good, thanks!

@mikepurvis mikepurvis merged commit 80a0411 into ros-drivers:hydro-devel Aug 11, 2017
@fmessmer fmessmer mentioned this pull request Oct 20, 2017
Closed
@furushchev furushchev deleted the fix branch December 12, 2017 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulbovbel paulbovbel paulbovbel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@furushchev @paulbovbel @mikepurvis