Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[video_recorder] Add timer to record low fps video at the correct speed #600

Open
wants to merge 3 commits into
base: melodic
Choose a base branch
from

Conversation

708yamaguchi
Copy link

@708yamaguchi 708yamaguchi commented Sep 18, 2020

Without this pull request, when we record video with lower fps than fps rosparam, the output video will be fast and shortened.

For example, if we set fps rosparam as 15 but input sensor_msgs::Image topic's hz is 5 and we record video for 30 seconds, the output video is only 10 seconds and 3x speed.

This pull request solves the above problem by using timer callback and outputting video at regular intervals.

Copy link
Collaborator

@JWhitleyWork JWhitleyWork left a comment

Choose a reason for hiding this comment

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

In general, I'm OK with this feature. Just needs some tweaks.

outputVideo << image;
ROS_INFO_STREAM("Recording frame " << g_count << "\x1b[1F");
g_count++;
g_last_wrote_time = stamp;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is no longer true. The timestamp of the last-received image can no longer be considered close enough to the actual time that the frame was written. Use ros::Time::now() instead.

@@ -70,7 +72,8 @@ void callback(const sensor_msgs::ImageConstPtr& image_msg)

}

if ((image_msg->header.stamp - g_last_wrote_time) < ros::Duration(1.0 / fps))
stamp = image_msg->header.stamp;
if ((stamp - g_last_wrote_time) < ros::Duration(1.0 / fps))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is unnecessary given my other comment.

@708yamaguchi
Copy link
Author

Thank you for your review.

I followed your advice and use ros::Time::now().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants