Skip to content

Fix first image loss when using the RPC to save the data#1012

Merged
GiulioRomualdi merged 10 commits intomasterfrom
fix_first_image_loss
Nov 27, 2025
Merged

Fix first image loss when using the RPC to save the data#1012
GiulioRomualdi merged 10 commits intomasterfrom
fix_first_image_loss

Conversation

@S-Dafarra
Copy link
Copy Markdown
Collaborator

@S-Dafarra S-Dafarra commented Oct 24, 2025

This is on top of #1011

This PR fixes a nasty bug introduced in #1002. I was using a single mutex to block all the videos. Beside the fact that this was also making the video threads blocking each other, the logic to reset the frame index was done before locking the mutex. This means that the threads blocked while saving the data would update the frame index only starting from the second frame. The first frame would either be overwritten when reaching again the previous index number, or it would appear as last.

In this PR, I exploited a separate variable to request the video thread to pause, and I wait until everyone is paused.

Moreover, I switched from saving the frame timestamp, to the saving of the index. The timestamp was already saved anyway.

cc @giotherobot @carloscp3009

@S-Dafarra
Copy link
Copy Markdown
Collaborator Author

S-Dafarra commented Oct 24, 2025

While looking at the code, I realized the reason why we were not able to log the first data point on the action interface (that is using VectorsCollection). In particular, when the first message arrived, it was only used to setup the channels, without storing the content. I fixed it with 532da24

@S-Dafarra S-Dafarra force-pushed the fix_first_image_loss branch from f6cba5a to 04586f6 Compare October 25, 2025 09:46
@S-Dafarra S-Dafarra force-pushed the fix_first_image_loss branch 2 times, most recently from 77efcea to 52f9709 Compare November 17, 2025 10:00
@S-Dafarra S-Dafarra marked this pull request as ready for review November 17, 2025 10:06
@S-Dafarra S-Dafarra force-pushed the fix_first_image_loss branch from 52f9709 to 4c9cc48 Compare November 25, 2025 17:23
@GiulioRomualdi
Copy link
Copy Markdown
Collaborator

Hi @S-Dafarra can I proceed with this?

@S-Dafarra
Copy link
Copy Markdown
Collaborator Author

Hi @S-Dafarra can I proceed with this?

Yes!

@S-Dafarra
Copy link
Copy Markdown
Collaborator Author

I had rebased on top of #1019 yesterday, let me know if you want me to rebase on top of master

@GiulioRomualdi GiulioRomualdi enabled auto-merge (squash) November 27, 2025 13:09
@GiulioRomualdi GiulioRomualdi merged commit 592d1cd into master Nov 27, 2025
9 checks passed
@S-Dafarra S-Dafarra deleted the fix_first_image_loss branch November 27, 2025 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants