-
Notifications
You must be signed in to change notification settings - Fork 737
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
Added frame-skipping tolerance parameter #605
Conversation
@@ -108,6 +114,7 @@ int main(int argc, char** argv) | |||
bool stamped_filename; | |||
local_nh.param("stamped_filename", stamped_filename, false); | |||
local_nh.param("fps", fps, 15); | |||
local_nh.param("tolerance", tolerance, 15.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind making this a value from 0-1 instead? Additionally, I'd prefer something more descriptive to indicate that it's a tolerance for the FPS rate (e.g. fps_tolerance_pct
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with switching it to 0-1. Then the "tolerance" can be interpreted as a fraction of the frame duration (1/fps). In that case, can I call the parameter something like fps_tolerance_fraction
or maybe fps_variation_allowance
?
{ | ||
// Skip to get video with correct fps | ||
ROS_INFO_STREAM("Skipping frame "<< g_count <<": " | ||
<< image_msg->header.stamp - g_last_wrote_time << " sec is outside of the " | ||
<< tolerance<< "\% tolerance"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is likely to happen often on some systems, I would prefer this be a _THROTTLE
at minimum and a _DEBUG
if you're OK with it instead of an _INFO
. Terminal spam is not recommended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the concern over terminal spam, so I'd be fine with setting this to _DEBUG
.
I'm not sure what you mean by "throttle at minimum"? Do you mean I should change the debug print statement to say "Throttle at maximum framerate" or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, no, I meant using one of the _THROTTLE
macros so the output of this message is throttled to once per time t (see http://wiki.ros.org/roscpp/Overview/Logging - the Throttle section).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice! That's super handy. I didn't know the _THROTTLE
type existed. Okay, so then what's a reasonable period? Once per second? Before, there was no notice of skipped frames at all. Do you think once every 2 seconds is fine?
Another thing: I'm not sure if it matters but I also changed the way frames are counted here (before, the skipped frames were not counted in g_count
. But I felt it made sense for g_count
to be "every frame that has ever arrived" rather than just "every frame that was saved". Thoughts on this?
Ping @cbteeple for changes based on feedback. |
There's been no response to the review on this PR for over a year, so I'm going to close this out. If you'd still like to get this in, feel free to reopen and respond to feedback. |
I’ve implemented this tolerance and even tried setting a tire like 50% and 30% but there is still a lot of frame skipping. At times a five minute video turns out to be 2 minutes because of skipping. My launch file:
|
Package/Feature:
image_view, video_recorder
Description:
Added a tolerance parameter for frame-skipping that enables the user to choose how much variation in framerate is acceptable.
What this fixes:
This is a fix for Issue #604. Adding a frame-skipping tolerance allows us to change the condition for frame skipping to allow for some natural variation (which often occurs when streaming video from a webcam using usb_cam for example).
Details:
The tolerance parameter is a percent, and is passed to the video_recorder node via the parameter server:
The condition in video_recorder for frame skipping changes from:
to
This allows some amount of variation in the framerate to be acceptable.
Testing
Platform: Ubuntu 20.04, ROS Noetic
Hardware: Logitech C920 Webcam
I think this confirms that the addition of the frame-skip tolerance has fixed the problem.