functionality for compression quality

[emrekuru97]

Implemented paramtric compression quality only if strem from camera is raw, it can be done when the strem from camera is compressed too but i tried to avoid double compression.

[christianrauch]

See the comments regarding the parameter, format conversion and error handling. For reference, this is the implementation of toCompressedImageMsg that you want to replace: https://github.com/ros-perception/vision_opencv/blob/066793a23e5d06d76c78ca3d69824a501c3554fd/cv_bridge/src/cv_bridge.cpp#L512-L535

It may be easier to just copy it and add the compression parameter to cv::imencode. Or better, send a PR to the vision_opencv repo with that additional parameter :-)

[christianrauch]

From your comments, I can now see that you are essentially reimplementing CompressedPublisher::publish instead of CvImage::toCompressedImageMsg. The implementation of CompressedPublisher::publish is much more complex than CvImage::toCompressedImageMsg.

Do you think re-implemening toCompressedImageMsg with the compression quality option would be sufficient for you?

[emrekuru97]

Yes it would be sufficient, if it's ok i will copy the function and make the necessary changes and update the pull request. But we still need to call cv_bridge::toCvShare to get a CvImage. Since the function toCompressedImageMsg works on CvImage. But i'll left the format blank so cv_bridge::toCvShare wont do any unnecessary converting.

[emrekuru97]

I tryed to fix all requested changes, if necessary i can go to a new branch with only one commit(if no more issues are present) or let this as it is.

[christianrauch]

Apart from my comments, you also have to apply the code style to let the CI pass. You can use the .clang-format with clang-format, or an IDE, to do this automatically.

[christianrauch]

I am mostly fine with this now. The major thing is now to move this into a function and document its source and license.

[christianrauch]

Thanks. The code looks much cleaner now. Missing is not only the source and license hint for the function that was copied (compressImageMsg) and a minor style change.

[christianrauch]

I am still a bit worried about mixing the code with the copied implementation of CvImage::toCompressedImageMsg. Could you move this function either into its own library (cleaner) or into a stand-alone function outside of the CameraNode class (easier)?

[emrekuru97]

I'll also try to get a PR in original_repo with additional default parameter to the function. Or with a new signature. May be if they merge we can thane change this too.

[christianrauch]

I'll also try to get a PR in original_repo with additional default parameter to the function. Or with a new signature. May be if they merge we can thane change this too.

That would be awesome and remove the need to keep this function here.

[christianrauch]

This looks good now. Final request: Can you squash all your commits into one and rebase/force-push to the PR? There are many back and forth changes that are better suited for a single commit.

[emrekuru97]

This looks good now. Final request: Can you squash all your commits into one and rebase/force-push to the PR? There are many back and forth changes that are better suited for a single commit.

Squashed the final code into the first commit, and changed the commit to "functionality for compression quality".

[emrekuru97]

Thanks :). Yesterday i also tryed to get a PR here in the original cv_bridge repo to the humble branch, but i'm not sure if that is the correct way since there are many request that are awaiting. Anyway if they will merge i'll remove the function and pass directly the parameter to the new version. I'll add the tag closes now.