improve image rendering performance by less waited locking

[christianrauch]

The current implementation of image_view::ImageViewNode has bad performance. You roughly only see half of the images on screen that are published on the topic.

This PR improves the thread locking and shows the full framerate of images on the topic.

[ahcorde]

Not really sure about the solution here. I prefer the condition_variable approach.

Did you benchmark ?

Now, we can use cpp20 at least on lyrical and rolling. what's about std::binary_semaphore ?

[christianrauch]

Did you benchmark ?

Not "professionally". When you see an image stream and compare it with rqt, it is obvious that the framerate is lower. Printing on the screen shows that the sync callback (imageCb) is called twice before an image is shown (cv::imshow).

[christianrauch]

Now, we can use cpp20 at least on lyrical and rolling. what's about std::binary_semaphore ?

What is the advantage of using the std::binary_semaphore over std::mutex in this case? From https://en.cppreference.com/cpp/thread/counting_semaphore it seems they would behave the same.

I replaced the mutex with the binary_semaphore now anyway. While technically, this seems to work the same, semantically using semaphore is easier to understand the purpose.

[christianrauch]

The CI is complaining about the new include #include <semaphore>:

    /__w/image_pipeline/image_pipeline/src/image_view/include/image_view/image_view_node.hpp:20:  Found C system header after C++ system header. Should be: image_view_node.h, c system, c++ system, other.  [build/include_order] [4]

There are no C headers in image_view_node.hpp, and there is no image_view_node.h in the repo.

@ahcorde Do you have any idea how to resolve this?

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[christianrauch]

@ahcorde Can you take a look at this?

The CI report:

- image_view.cpplint build/include_order [4] (/__w/image_pipeline/image_pipeline/src/image_view/include/image_view/image_view_node.hpp:20)
  <<< failure message
    Found C system header after C++ system header. Should be: image_view_node.h, c system, c++ system, other.
  >>>

does not make sense to me. Line 20 includes a standard C++ header (#include <semaphore>) and there is no such header image_view_node.h.

[mikeferguson]

The image_view_node.h part is a red herring I think, that's just the format of the error message (it lets you know that you can have a header file for the specific cpp file).

I think the issue is that cpplint is miscategorizing something as a "C system header" (I've seen that happen before - but I got no idea what is going on in this case).

Perhaps try reverting the change on line 25 and make cv_bridge still use quotes instead of brackets? (although I thought the rule was that a .h file would be considered a C system header if it was in brackets, not a .hpp file).

[christianrauch]

@ahcorde Please help me out here :-) I am not including any C headers. Can you please fix the linters to remove these false positives?

[christianrauch]

@ahcorde Can you take care of the linting in the CI? I reverted some of my header changes but still get the same warning. If the linting provides false positives, can you simply remove it?

[christianrauch]

@ahcorde I managed to avoid the problem by removing the <semaphore> header. I still don't understand why this could potentially be treated as a C header, but the package builds and the tests pass. Please check this again.