Skip to content

Add support for single channel 16 bit grayscale image format#229

Merged
deepanshubansal01 merged 4 commits intomainfrom
deepanshu/camera-l16-format
Jun 1, 2022
Merged

Add support for single channel 16 bit grayscale image format#229
deepanshubansal01 merged 4 commits intomainfrom
deepanshu/camera-l16-format

Conversation

@deepanshubansal01
Copy link
Contributor

@deepanshubansal01 deepanshubansal01 commented May 26, 2022

Summary

This PR adds support for 16 bit grayscale image format full description here #73

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review [another open pull request

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 deepanshubansal01 marked this pull request as draft May 26, 2022 21:32
@github-actions github-actions bot added the 🌱 garden Ignition Garden label May 26, 2022
@deepanshubansal01
Copy link
Contributor Author

deepanshubansal01 commented May 26, 2022

@iche033 I was wondering for 16 bit image format unit test should I just replicate like the one did for 8 bit, seems like it will be a lot of code duplication and if their is a way we can avoid the code duplication. cc @adityapande-1995

@codecov
Copy link

codecov bot commented May 26, 2022

Codecov Report

Merging #229 (4cb7fee) into main (b90996f) will not change coverage.
The diff coverage is n/a.

❗ Current head 4cb7fee differs from pull request most recent head a1cb297. Consider uploading reports for the commit a1cb297 to get more accurate results

@@           Coverage Diff           @@
##             main     #229   +/-   ##
=======================================
  Coverage   80.00%   80.00%           
=======================================
  Files           1        1           
  Lines          15       15           
=======================================
  Hits           12       12           
  Misses          3        3           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55c5666...a1cb297. Read the comment docs.

@deepanshubansal01
Copy link
Contributor Author

@iche033 I was wondering for 16 bit image format unit test should I just replicate like the one did for 8 bit, seems like it will be a lot of code duplication and if their is a way we can avoid the code duplication. cc @adityapande-1995

Can we say that if it works for 8 bit then it will work for 16 bit as well and we don't need to write additional unit tests for 16 bit, not sure.

@iche033
Copy link
Contributor

iche033 commented May 26, 2022

you can modify that test and the test sdf file to load 2 cameras: one 8 bit and one 16 bit, and just subscribe to 2 topics and verify the image format in 2 different callbacks

@deepanshubansal01
Copy link
Contributor Author

you can modify that test and the test sdf file to load 2 cameras: one 8 bit and one 16 bit, and just subscribe to 2 topics and verify the image format in 2 different callbacks

Thanks a lot!

@deepanshubansal01 deepanshubansal01 self-assigned this May 26, 2022
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 deepanshubansal01 marked this pull request as ready for review May 27, 2022 21:21
unsigned int g_imgCounter2 = 0;

void OnGrayscaleImage(const gz::msgs::Image &_msg)
void OnGrayscaleImageL8(const ignition::msgs::Image &_msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Because of the transition to gz namespace, we should keep this as gz::msgs::Image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Thanks

g_imgCounter1++;
}

void OnGrayscaleImageL16(const ignition::msgs::Image &_msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, gz

</noise>
</camera>
</sensor>
<sensor name="camera2" type="camera">
Copy link
Contributor

Choose a reason for hiding this comment

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

camera_16_bit sounds more descriptive. Similarly in the code for sensor pointers, handles and helpers, it would be nice to have descriptive names, like cameraId8Bit or sensorPtrCamera8Bit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the CI is also failing after I resolved merge conflict. I will take a look at that as well.

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@adityapande-1995
Copy link
Contributor

The tests for cameras are segfaulting on jammy.

@chapulina
Copy link
Contributor

The tests for cameras are segfaulting on jammy.

Those have been happening before this PR 🙃

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

looks good to me, thanks for updating the test.

@deepanshubansal01 deepanshubansal01 merged commit 3359a0f into main Jun 1, 2022
@deepanshubansal01 deepanshubansal01 deleted the deepanshu/camera-l16-format branch June 1, 2022 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🌱 garden Ignition Garden

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants