Snapshot should write to a different file every time it is triggered

[bijoua29]

Description

When you trigger multiple snapshots through the service call, each snapshot data is written to the same file. This is not very useful for multiple snapshots.

The typical use-case for the snapshot feature is to save a snapshot when the user see something "interesting". This might happen multiple times. Usually triggering a snapshot is not always a one-time event.

Related Issues

N/A

Completion Criteria

• Save to a different file when a snapshot is triggered
• Add ability to specify filename for each snapshot or allow option for timestamped file

Implementation Notes / Suggestions

N/A

Testing Notes / Suggestions

N/A

[swiz23]

I second this feature request. This seems helpful.

[wvergouwenavular]

+1

[emersonknapp]

Note - the recorder does provide the service split_bagfile - you could call that immediately after the snapshot to split. Though, I think it's not unreasonable to provide a split option in Snapshot.srv so that it can happen atomically. PRs welcome!

[Timple]

Shall we extend the Snapshot.srv from this:

---
bool success

To this?

string filename   # Name of bag file to save snapshot to.
---
bool success

In line with this: https://github.com/ros/rosbag_snapshot/blob/main/rosbag_snapshot_msgs/srv/TriggerSnapshot.srv

We probably need to come up with a way to append the timestamp as well. Perhaps if we enter something like my_file_{stamp} that this would result in my_file_2024_01_22-14_18_28/? Suggestions welcome

[MichaelOrlov]

@Timple Extend Snapshot.srv with a few new parameters is not a big deal or a problem.
However, it turns out that it is not trivial to implement this feature on the lower rosbag2 layers.
There is also another request to extend snapshot feature to be able to write a buffer after the event as well. These two extensions are sort of overlaps in implementation and even alone are not trivial to implement.

[bijoua29]

I guess I'm surprised that just adding the ability to name the file seems not trivial to implement. But I have no knowledge of the structure of the code.

[Timple]

Would it make sense to align on the service call syntax already?

ROS2 Jade is coming up, and changing a service after release won't be possible I guess. Implementation can then be worked on after the interface is agreed.

[bijoua29]

Yes, it would make sense on the service call syntax. Was there a proposal for an updated syntax?

[Timple]

https://github.com/ros2/rosbag2/issues/1099#issuecomment-1903990853 ?

[reinzor]

Hi all,

Thanks for all the information so far. I've read this issue and studied this PR with all its comments. Based on this info and fiddling a bit in the rosbag2_cpp package, I can come up with the following plan:

• Extend Snapshot.srv to include a name, as suggested here
• This name can then be passed in the take_snapshot method as an input argument.
• The SequentialWriter will then receive the name and implement the following logic after the notify_data_ready
  • Store the current storage_ path
  • Call the switch_to_next_storage method
  • Rename the last written storage file and alter the metadata (rename based on the name and the current stamp)
  • Write the metadata (as is being done in the close() method)

I rapidly implemented this and this seemed to work.

This will give the following output:

.
└── rosbag2_2024_04_09-14_59_55
    ├── metadata.yaml
    ├── rosbag2_2024_04_09-14_59_55_0_snapshot_A.mcap
    ├── rosbag2_2024_04_09-14_59_55_1_snapshot_B.mcap
    ├── rosbag2_2024_04_09-14_59_55_2_snapshot_C.mcap

@bijoua29 @dkorenkovnl @Timple @MichaelOrlov could you give your feedback on this approach before I send a PR? Thanks!

[MichaelOrlov]

@reinzor That is one more very hacky way to implement this feature.

1. I don't like the idea of renaming a file right after writing it because we should have a callback bag_events::BagEvent::WRITE_SPLIT right after creating a new file and closing the old one. Essentially we need functionality from the SequentialWriter::split_bagfile()
2. The SequentialWriter::switch_to_next_storage() will call cache_consumer_->stop(); which will try to swap circular buffers again and flush data to disk one more time. And there will be a race condition between the current call to data dumping from the buffer. if we are lucky and we have a lot of messages in buffer and slow hdd we probably in a good luck. If not we will dump extra data randomly one more time which we were not asked for. This is undefined behavior!

[reinzor]

@reinzor That is one more very hacky way to implement this feature.

1. I don't like the idea of renaming a file right after writing it because we should have a callback bag_events::BagEvent::WRITE_SPLIT right after creating a new file and closing the old one. Essentially we need functionality from the SequentialWriter::split_bagfile()
2. The SequentialWriter::switch_to_next_storage() will call cache_consumer_->stop(); which will try to swap circular buffers again and flush data to disk one more time. And there will be a race condition between the current call to data dumping from the buffer. if we are lucky and we have a lot of messages in buffer and slow hdd we probably in a good luck. If not we will dump extra data randomly one more time which we were not asked for. This is undefined behavior!

Thanks for your feedback @MichaelOrlov ; could you perhaps point me in the right direction for this implementation? Regarding point 1, what would be your suggestion instead of renaming? As far as I know, the name of the file is already defined when starting the record so if we would like the snapshot to have a specific filename, this would not be possible. We could maybe add some additional data to the custom_data structure in the metadata? As for point 2, it is not very clear for me how the internal consumer work so how can I implement this in a proper way without the undefined behavior? Thanks again!

[Timple]

I believe https://github.com/ros2/rosbag2/pull/1584 was a step in that direction. However some workarounds where proposed there and now it's closed. It does contain a hint on where to start it seems: https://github.com/ros2/rosbag2/pull/1584#issuecomment-2048794012

@MichaelOrlov you seem to be most familiar with the codebase in this thread. Would it be possible to write up a small set of steps to take to tackle this issue?

[MichaelOrlov]

I am sorry folks, I am currently kinda busy with Jazzy release preparation. A lot of PRs and other things need to wrap up. I can get back to this task after a couple of weeks.
Need to think thoroughly about how we can implement this feature in the best way. In the case of the filename not sure if it would be possible to enforce a new file name over the snapshot service call without file renaming I was thinking about maybe in this case we can return the saved filename in the snapshot service response message instead of enforcing a new name?

[Timple]

I'm not quite sure why the bagfile is already open actually. Because that is where this problem is originating from.

Why not open it upon receiving a snapshot request?

[fujitatomoya]

Why not open it upon receiving a snapshot request?

sounds reasonable, file should be named and created upon snapshot capture request by user. but i am not sure how we can deal with metadata.yaml? can it include the multiple bag data information? if not, it should create rosbag2_2024_04_15-15_19_02?snapshot folder on the request by user?