Skip to content
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

Improve video recording management #655

Merged

Conversation

rafaellehmkuhl
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl commented Dec 22, 2023

Put the video store in charge of managing the video recordings, so recording information gets stored with the stream data. There are two main benefits on this:

  • The lifecycle of a widget (e.g.: a view being changed and a recording widget being unmounted) does not affect the state of a recording
  • Multiple recordings, from different streams, can be done simultaneously

Fix #624
Fix #633

To be merged after #654.

@rafaellehmkuhl
Copy link
Member Author

@joaoantoniocardoso @patrickelectric open for review.

@rafaellehmkuhl rafaellehmkuhl marked this pull request as ready for review December 22, 2023 15:46
Copy link
Member

@joaoantoniocardoso joaoantoniocardoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not critical, but confusing at least:

Because the widget shares a global state, and the source selection only happens when the user clicks the record/stop button, the user can't select a different source when there is a recording happening for the first source of the list of streams available, requiring the user to stop the recording before configuring the latter added widget:

cannot_change_recording_source_because_widgets_share_global_state.mp4

note that it is also impossible to record two videos from the same source (let's say, a quick clip and the entire dive)

@rafaellehmkuhl
Copy link
Member Author

Not critical, but confusing at least:

Because the widget shares a global state, and the source selection only happens when the user clicks the record/stop button, the user can't select a different source when there is a recording happening for the first source of the list of streams available, requiring the user to stop the recording before configuring the latter added widget:

cannot_change_recording_source_because_widgets_share_global_state.mp4
note that it is also impossible to record two videos from the same source (let's say, a quick clip and the entire dive)

Nice catch.
I think the multiple records for the same stream is something we can delay for now. Will ask our beta testers what they think about it. But the problem of not being able to record multiple sources I will fix already.

@rafaellehmkuhl
Copy link
Member Author

@joaoantoniocardoso fixed.

@rafaellehmkuhl rafaellehmkuhl merged commit a2d7886 into bluerobotics:master Dec 23, 2023
7 checks passed
@rafaellehmkuhl rafaellehmkuhl deleted the improve-video-recording branch December 23, 2023 20:04
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.

[Video Recorder] Fail to record multiple video streams Video recording stops when changing views
3 participants