image_pipeline
image_pipeline copied to clipboard
ros-perception
sec_per_frame_ is unintuitive
The preprocessor variable VIDEO_ makes no sense in the extract_images node.
Due to that, the very un-intuitive parameter sec_per_frame_ is necessary in the callback.
So I vote for getting rid of VIDEO_ due to video_recorder node.
Then, sec_per_frame_ is no longer necessary by that definition.
Its default parameter can be then set to 0, or it should completely replaced by e.g. store_nth_frame to store ever n'th frame which respects the image stream more precisely.
I agree for VIDEO sec_per_frame is used to respect a framerate though, don't you agree. Your definition is more to only keep every other frame.
But sec_per_frame_ doesn't respect the real frame rate. The thing is, that one can only store the images with 1/1, 1/2, 1/3, 1/4, ..., 1/N of the real framerate anyway. Therefore it is unnecessary from my point of view, to do the calculation of sec_per_frame_:=1/real_fps*1/N if I want to store every n'th image. Every fracture of N (s.t. one chooses freely sec_per_frame_) won't make any sense because there are not any frames "in between" which can be stored.
In particular, there is also a faulty behavior in the application due to the usage of the time stamp in the code and not the one of the message. Let's assume you have image topic publishing with 1hz. Now one wants to downsample to exactly 0.5hz s.t. sec_per_frame_:=2. Due to the _time = ros::Time::now().toSec(); the delay between the actual image time and process is always a bit of and that delta accumulates. Thus, it will eventually wont get the second image, but the third. I think instead of using ros::Time::now().toSec() in 111 and 114 one should use msg->header.stamp.toSec() to respect the image time and not taking the transport and process time into account. But with this will produce a faulty behavior as well, while the camera or the publishing process might have a jitter. Therefore, some time range needs to be introduced to check, if the image time stamp lies within that range, and if so it should be stored to the hard drive.
VIDEO_ is gone in ROS 2 (looks like it's still in noetic, but I don't see anybody addressing that at this point)
Therefore, renaming this ticket since the real issue is the sec_per_frame_ sampling
