Deal better with looping on a video file and the start and stop frame selection #67
Conversation
| gen.add("start_frame", int_t, LEVEL.NORMAL, "Start frame of the video ", 0, 0) | ||
| gen.add("stop_frame", int_t, LEVEL.NORMAL, "Stop frame of the video", -1, -1) | ||
| gen.add("start_frame", int_t, LEVEL.RUNNING, "Start frame of the video ", 0, 0) | ||
| gen.add("stop_frame", int_t, LEVEL.RUNNING, "Stop frame of the video", -1, -1) |
There was a problem hiding this comment.
Incrased the level to force the dynamic_reconfigure callback to do unsubscribe and then subscribe so frame correctness checks are done.
| if (video_stream_provider_type == "videofile" && | ||
| frame_counter == latest_config.stop_frame - latest_config.start_frame) | ||
| frame_counter >= latest_config.stop_frame - latest_config.start_frame) |
There was a problem hiding this comment.
There are cases where the frame_counter variable can go over this check and then we are stuck forever on the Could not capture frame error. By doing >= we make sure to reset.
| @@ -241,16 +243,22 @@ virtual void subscribe() { | |||
| cap->open(video_stream_provider); | |||
| if(video_stream_provider_type == "videofile" ) | |||
| { | |||
| if(latest_config.stop_frame == -1) latest_config.stop_frame = cap->get(cv::CAP_PROP_FRAME_COUNT); | |||
| // We can only check the number of frames when we actually open the video file | |||
| NODELET_INFO_STREAM("Video number of frames: " << cap->get(cv::CAP_PROP_FRAME_COUNT)); | |||
There was a problem hiding this comment.
Try to avoid doing things obscurely and warn the user. The user may want to use this information to update their launch file to the last frame.
| latest_config.stop_frame = cap->get(cv::CAP_PROP_FRAME_COUNT); | ||
| } | ||
|
|
||
| if(latest_config.start_frame >= latest_config.stop_frame) | ||
| { | ||
| NODELET_WARN_STREAM("Invalid 'start frame' " << latest_config.start_frame << ", which excceds 'stop frame' " << latest_config.stop_frame << ". Set 'start frame' to 0."); | ||
| NODELET_ERROR_STREAM("Invalid 'start_frame' " << latest_config.start_frame << ", which exceeds 'stop_frame' " << latest_config.stop_frame << ". Setting internally (won't be shown in dynamic_reconfigure) 'start_frame' to 0."); |
There was a problem hiding this comment.
Upgrade these to errors, as they are actually configuration errors.
|
P.S.: sorry about the second commit, please squash them together when merging (if you find this approach makes sense to you). |
There was a problem hiding this comment.
Very sorry for delay.
Looks good to me!
Now #72 should be fixed with this patch.
Based on this issue: #61
And this PR: #65
I've used the approach from @martiege. I tried to have a flag to mark if the dynamic_reconfigure server needed to be updated, and the call updateConfig with the new configuration, but this didn't work (the updateConfig never actually calls the dynamic_reconfigure server to update itself).
I've also added some extra logging to try to make a bit clearer what is happening when a user plays with the
start_frameandstop_frameparameters.I've increased the dynamic_reconfigure levels of changing
start_frameandstop_frameto force re-subscribing as in the subscription point in the code we have the checks for the frame count. I couldn't move these checks to the dynamic_reconfigure call because we don't have a way to know the amount of frames up until we've opened the video file to call that API. Changing this would imply more changes than I'm comfortable making right now without adding a lot of unit tests (that we should have).