Clean code for Image display #1271

penww · 2024-08-26T09:45:16Z

Some cleanup:

Considering duplicated code in multiple places, using a common function to handle YUV to RGB pixel conversion.
Using std::clamp() instead of clamp expressions.

ahcorde

I'm worried here about the performance, we should call this function twice for everypixel. Did you compare the performance ? maybe we should use inline?

penww · 2024-08-30T02:48:16Z

@ahcorde Thanks for your suggestion. I ignored the performance impact of function calls, will change it to inline.

I will continuelly work on ImageDisplay to support more YUV format, such as nv12, nv24 etc.

BTW, eventhough it is inline, convert color for every pixel is not efficient, especially when image is large.
Can we consider to convert with OpenGL / OpenMP? I can have a try on it.

Signed-off-by: Peng Wang <[email protected]>

penww · 2024-08-30T09:29:13Z

Has pushed new patch to change this function to inline.

I also compared std::clamp with ((i & 0xFFFFFF00) == 0) ? i : (i < 0) ? 0 : 0xFF, it shows std::clamp faster.
And checked std::tie, it shows no more extra performance cost.

In O2 compile option, std::clamp and std::tie are faster than old code. I think compiler making optimization for std functions.

zycczy · 2024-10-12T02:53:27Z

Hi @ahcorde
Is there any update for this PR

ahcorde · 2024-10-14T10:11:16Z

Pulls: #1271
Gist: https://gist.githubusercontent.com/ahcorde/3e98ba46a6470006da49fd65022f8c58/raw/02f480667308172745d5283f9da22dc37754cf03/ros2.repos
BUILD args: --packages-above-and-dependencies rviz_default_plugins --packages-above-and-dependencies rviz_default_plugins
TEST args: --packages-above rviz_default_plugins --packages-above rviz_default_plugins
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14700