Skip to content

Comments

add container for Opentera webrtc ros #91

Open
wendwosenbb wants to merge 9 commits intointrolab:ros2from
wendwosenbb:opentera-webrtc-ros-add-container
Open

add container for Opentera webrtc ros #91
wendwosenbb wants to merge 9 commits intointrolab:ros2from
wendwosenbb:opentera-webrtc-ros-add-container

Conversation

@wendwosenbb
Copy link

adds container for ros2 humble branch of opentera-webrtc-ros.

instruction is provided in the readme file.

Copy link
Collaborator

@mamaheux mamaheux left a comment

Choose a reason for hiding this comment

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

Thank you for contributing to the repository. I have left some comments for improving your contribution.

update-ca-certificates

# run bash file
CMD ["/bin/bash", "-c", "chmod +x ./opentera_teleop_startup.sh && ./opentera_teleop_startup.sh"] No newline at end of file
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing blank line

update-ca-certificates

# run bash file
CMD ["/bin/bash", "-c", "chmod +x ./opentera_teleop_startup.sh && ./opentera_teleop_startup.sh"] No newline at end of file
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe put "chmod +x ./opentera_teleop_startup.sh" in a run statement.

@@ -0,0 +1,19 @@
-----BEGIN CERTIFICATE-----
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remove all certificate files from the repository? You can replace them by a bash script for creating them. Make sure you add a .gitignore file containing the certificates. You can add a step in the readme file for generating them.

I would suggest you generate new certificates for your application since these have been exposed on the Internet.

<static>1</static>
<allow_auto_disable>1</allow_auto_disable>
</model>
</sdf>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing blank line.


echo $ROS_DISTRO
ros2 launch startup_opentera_demos.launch.py
#ros2 run rviz2 rviz2 No newline at end of file
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add an option to run rviz or remove the line.
Add the missing blank line.

Comment on lines 54 to 72
<!-- Front camera -->
<!--group><include file="/opt/ros/OPENTERA_WS/ros_stream_client.launch.xml">
<arg name="name" value="topic_streamer1"/>
<arg name="is_stand_alone" value="$(var is_stand_alone)"/>
<arg name="can_send_audio_stream" value="true"/>
<arg name="can_receive_audio_stream" value="false"/>
<arg name="can_send_video_stream" value="true"/>
<arg name="can_receive_video_stream" value="false"/>
<arg name="is_screen_cast" value="false"/>
<arg name="needs_denoising" value="false"/>
<arg name="server_url" value="http://$(var signaling_server_hostname):$(var signaling_server_port)"/>
<arg name="client_name" value="Robot Camera 1"/>
<arg name="room_name" value="VideoConf"/>
<arg name="room_password" value="$(var signaling_server_password)"/>
<arg name="input_camera_topic" value="$(var input_camera_topic_based_on_face_cropping)"/>
<arg name="force_gstreamer_video_hardware_acceleration" value="$(var force_gstreamer_video_hardware_acceleration)"/>
</include></group-->

<!-- Bottom camera -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove

Suggested change
<!-- Front camera -->
<!--group><include file="/opt/ros/OPENTERA_WS/ros_stream_client.launch.xml">
<arg name="name" value="topic_streamer1"/>
<arg name="is_stand_alone" value="$(var is_stand_alone)"/>
<arg name="can_send_audio_stream" value="true"/>
<arg name="can_receive_audio_stream" value="false"/>
<arg name="can_send_video_stream" value="true"/>
<arg name="can_receive_video_stream" value="false"/>
<arg name="is_screen_cast" value="false"/>
<arg name="needs_denoising" value="false"/>
<arg name="server_url" value="http://$(var signaling_server_hostname):$(var signaling_server_port)"/>
<arg name="client_name" value="Robot Camera 1"/>
<arg name="room_name" value="VideoConf"/>
<arg name="room_password" value="$(var signaling_server_password)"/>
<arg name="input_camera_topic" value="$(var input_camera_topic_based_on_face_cropping)"/>
<arg name="force_gstreamer_video_hardware_acceleration" value="$(var force_gstreamer_video_hardware_acceleration)"/>
</include></group-->
<!-- Bottom camera -->
<!-- Camera -->

Comment on lines 1 to 93
<launch>

<!-- Arguments -->
<arg name="name" default="topic_streamer"/>
<arg name="is_stand_alone" default="true"/>
<arg name="video_queue_size" default="1"/>
<arg name="audio_queue_size" default="1"/>

<!-- Video -->
<arg name="can_send_video_stream" default="false"/>
<arg name="can_receive_video_stream" default="false"/>
<arg name="is_screen_cast" default="false"/>
<arg name="needs_denoising" default="false"/>

<!-- Codec -->
<arg name="forced_video_codecs" default="['']"/> <!-- "[]" is bugged (https://github.com/ros2/rclcpp/issues/1955), using "['']" and filtering empty strings in the node -->
<arg name="force_gstreamer_video_hardware_acceleration" default="false"/>
<arg name="use_gstreamer_video_software_encoder_decoder" default="false"/>

<!-- Audio -->
<arg name="can_send_audio_stream" default="false"/>
<arg name="can_receive_audio_stream" default="false"/>
<arg name="sound_card_total_delay_ms" default="40"/>
<arg name="echo_cancellation" default="true"/>
<arg name="auto_gain_control" default="true"/>
<arg name="noise_suppression" default="true"/>
<arg name="high_pass_filter" default="false"/>
<arg name="stereo_swapping" default="false"/>
<arg name="transient_suppression" default="true"/>

<!-- Signaling -->
<arg name="server_url" default="http://localhost:8080"/>
<arg name="client_name" default="streamer"/>
<arg name="room_name" default="chat"/>
<arg name="room_password" default="abc"/>
<arg name="verify_ssl" default="false"/>

<!-- Topics -->
<arg name="input_camera_topic" default="ros_image"/>
<arg name="output_webrtc_image_topic" default="webrtc_image"/>
<arg name="input_audio_topic" default="audio_in"/>
<arg name="output_audio_mixed_topic" default="audio_mixed"/>
<arg name="output_webrtc_audio_topic" default="webrtc_audio"/>

<node name="$(var name)" pkg="opentera_webrtc_ros" exec="topic_streamer" respawn="true"> <!--launch-prefix="xterm -e gdb -ex run args" -->
<!-- Enable debug log level -->
<param name="log_level" value="debug"/>
<param name="is_stand_alone" value="$(var is_stand_alone)"/>
<param name="video_queue_size" value="$(var video_queue_size)"/>
<param name="audio_queue_size" value="$(var audio_queue_size)"/>

<param name="video_stream">
<param name="can_send_video_stream" value="$(var can_send_video_stream)"/> <!-- Can send video stream to the signaling server -->
<param name="can_receive_video_stream" value="$(var can_receive_video_stream)"/> <!-- Can receive video stream from the signaling server -->
<param name="is_screen_cast" value="$(var is_screen_cast)"/> <!-- Is the image source a screen capture? -->
<param name="needs_denoising" value="$(var needs_denoising)"/> <!-- Does the image source needs denoising? -->
</param>

<param name="video_codecs">
<param name="forced_codecs" value="$(var forced_video_codecs)"/>
<param name="force_gstreamer_hardware_acceleration" value="$(var force_gstreamer_video_hardware_acceleration)"/>
<param name="use_gstreamer_software_encoder_decoder" value="$(var use_gstreamer_video_software_encoder_decoder)"/>
</param>

<param name="audio_stream">
<param name="can_send_audio_stream" value="$(var can_send_audio_stream)"/> <!-- Can send audio stream to the signaling server -->
<param name="can_receive_audio_stream" value="$(var can_receive_audio_stream)"/> <!-- Can receive audio stream from the signaling server -->
<param name="sound_card_total_delay_ms" value="$(var sound_card_total_delay_ms)"/>
<param name="echo_cancellation" value="$(var echo_cancellation)"/>
<param name="auto_gain_control" value="$(var auto_gain_control)"/>
<param name="noise_suppression" value="$(var noise_suppression)"/>
<param name="high_pass_filter" value="$(var high_pass_filter)"/>
<param name="stereo_swapping" value="$(var stereo_swapping)"/>
<param name="transient_suppression" value="$(var transient_suppression)"/>
</param>

<param name="signaling">
<param name="server_url" value="$(var server_url)"/> <!-- Signaling server URL used in stand_alone mode only -->
<param name="client_name" value="$(var client_name)"/> <!-- Peer name as which to join the room -->
<param name="room_name" value="$(var room_name)"/> <!-- Room name to join -->
<param name="room_password" value="$(var room_password)"/> <!-- Room password used in stand_alone_mode only -->
<param name="verify_ssl" value="$(var verify_ssl)"/> <!-- SSL peer verification -->
</param>
<!-- Add GStreamer debug environment variable -->
<env name="GST_DEBUG" value="4"/>
<remap from="ros_image" to="$(var input_camera_topic)"/>
<remap from="webrtc_image" to="$(var output_webrtc_image_topic)"/>
<remap from="audio_in" to="$(var input_audio_topic)"/>
<remap from="audio_mixed" to="$(var output_audio_mixed_topic)"/>
<remap from="webrtc_audio" to="$(var output_webrtc_audio_topic)"/>
</node>

</launch>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why have you duplicated the file?

@philippewarren
Copy link
Collaborator

Also, don't wory about the CI failing for now: we are still waiting on an update to the tf2 ROS package in the ROS repositories for Ubuntu 22.04, the bug is on their side.

wendwosenbb and others added 2 commits January 29, 2025 12:21
…sary lines and files. added missing blank spaces.
Co-authored-by: Marc-Antoine Maheux <[email protected]>
@wendwosenbb
Copy link
Author

I have added the changes you requested. @mamaheux

let me know if there is more changes required.

RUN git clone https://github.com/elFarto/nvidia-vaapi-driver.git

# Build and install nvidia-vaapi-driver
WORKDIR /opt/ros/OPENTERA_WS/nvidia-vaapi-driver
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could colcon later try to build it again since it's in the workspace?

export LIBVA_DRIVER_NAME=nvidia

# Add the signaling server to PATH
export PATH=/opt/ros/OPENTERA_WS/opentera-webrtc-ros/install/opentera_webrtc_ros/local/bin:$PATH
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be in PATH already after line 3, where we source the workspace setup file.

Comment on lines 20 to 21
export GAZEBO_MODEL_PATH=/opt/ros/OPENTERA_WS/opentera-webrtc-ros/install/opentera_webrtc_demos/share/opentera_webrtc_demos/models:$GAZEBO_MODEL_PATH
export GAZEBO_MODEL_PATH=/opt/ros/OPENTERA_WS/models:$GAZEBO_MODEL_PATH
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are they not in the MODEL_PATH after sourcing the workspace? We export it here: https://github.com/introlab/opentera-webrtc-ros/blob/ros2/opentera_webrtc_demos/package.xml#L30
If it does not work, this is a bug with the package and should be fixed directly (using ament_environment_hooks in cmake?)

Copy link
Collaborator

@philippewarren philippewarren left a comment

Choose a reason for hiding this comment

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

Good changes, still missing a few points to address from the previous review.

Comment on lines 78 to 80
<arg name="can_receive_audio_stream" value="false"/>
<arg name="can_receive_audio_stream" value="true"/>
<arg name="can_send_video_stream" value="true"/>
<arg name="can_receive_video_stream" value="false"/>
<arg name="can_receive_video_stream" value="true"/>
Copy link
Collaborator

@philippewarren philippewarren Feb 19, 2025

Choose a reason for hiding this comment

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

Only the first node needs these to True, put these specific ones back to false

Comment on lines 97 to 99
<arg name="can_receive_audio_stream" value="false"/>
<arg name="can_receive_audio_stream" value="true"/>
<arg name="can_send_video_stream" value="true"/>
<arg name="can_receive_video_stream" value="false"/>
<arg name="can_receive_video_stream" value="true"/>
Copy link
Collaborator

@philippewarren philippewarren Feb 19, 2025

Choose a reason for hiding this comment

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

Only the first node needs these to True, put these specific ones back to false


# Set the Video Acceleration API (VA-API) driver to NVIDIA
export LIBVA_DRIVER_NAME=nvidia
# export LIBVA_DRIVER_NAME=nvidia
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove completely (you can keep it in another branch in your repo if you want to keep working on it afterwards)

Comment on lines 117 to 121
RUN git clone https://github.com/elFarto/nvidia-vaapi-driver.git
# RUN git clone https://github.com/elFarto/nvidia-vaapi-driver.git

# Build and install nvidia-vaapi-driver
WORKDIR /opt/ros/OPENTERA_WS/nvidia-vaapi-driver
RUN meson setup build \
&& meson install -C build
# # Build and install nvidia-vaapi-driver
# WORKDIR /opt/ros/OPENTERA_WS/nvidia-vaapi-driver
# RUN meson setup build \
# && meson install -C build
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove completely


COPY opentera_teleop_startup.sh .

COPY launch .
Copy link
Collaborator

Choose a reason for hiding this comment

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

The files in the launch directory will get in the container because we then git clone the repo inside it, therefore I don't think the copy is necessary.

Comment on lines +5 to +9
# Add /usr/lib/x86_64-linux-gnu to the library path
export LD_LIBRARY_PATH=/usr/lib/x86_64-linux-gnu:$LD_LIBRARY_PATH

# Add /usr/local/lib to the library path
export LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:/usr/local/lib
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure these are needed? I would think they are there by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants