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

UI: Add frontend event and output signal for replay buffer saving #8955

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Penwy
Copy link
Contributor

@Penwy Penwy commented May 22, 2023

Description

This adds a frontend event and a signal on the replay buffer's signal
handler that trigger when the replay buffer is saving (i.e. when a save
has been requested, but before it saves).
The frontend event triggers the moment the frontend requests a save
(either through hotkey or the button), whereas the signal triggers when
the replay buffer actually starts saving (separated by the encoding delay).

N.B.: Allowing the save hotkey to trigger a frontend event required
moving its registration from obs-ffmpeg (in the definition of the replay
buffer output) to the UI (with all the frontend hotkeys for outputs).
This actually constitutes the biggest change in this PR.

Motivation and Context

Motivation is mainly to bring replay buffer save to feature parity with
other output events, which almost all have both "before" and "after" events.
A specific usecase envisionned was to allow the dynamic changing of
replay buffer saving path/filename rather than having one set-in-stone
save formatting for the whole session, but many other usecases could be
possible.

Moving over the hotkey registration also has the added benefit of moving
its location next to the other replay buffer hotkeys in the settings UI,
which creates a more consistent UX.

How Has This Been Tested?

This has been tested on Ubuntu 22.04.
Changes to hotkey registration seem to not have affected its behaviour.
Event and signal trigger correctly from hotkey and button.

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Documentation (a change to documentation pages)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Is the hotkey member of struct ffmpeg_muxer used anywhere at all now? Should it be removed?

UI/obs-frontend-api/obs-frontend-api.h Outdated Show resolved Hide resolved
UI/window-basic-main.cpp Outdated Show resolved Hide resolved
UI/window-basic-main.cpp Outdated Show resolved Hide resolved
@RytoEX RytoEX changed the title UI: add frontend event and output signal for replay buffer saving UI: Add frontend event and output signal for replay buffer saving May 22, 2023
@Penwy
Copy link
Contributor Author

Penwy commented May 22, 2023

the hotkey member isn't used anywhere in obs code as far as I can see, but the commit that put it there, 87a64e6 , states it does so to "Allows the ability to use it for more output types". So I preferred to not remove it in case it was used by some third party's output.
If you think that's mistaken, I'll gladly remove it.

Copy link
Member

@gxalpha gxalpha left a comment

Choose a reason for hiding this comment

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

Are the changes here all dependent on each other? If not, it might make sense to split them into multiple PRs.
It might also make sense to combine "UI: Register the replay buffer save hotkey in the frontend" and "obs-ffmpeg: Clean up now-redundant hotkey registering code" into one commit, given that otherwise you’d have two replay buffer hotkeys at the same time if you only have one of the commits.

Would also like a @Warchamp7 review on the fact that the save replay buffer hotkey is now always available, even if replay buffer is disabled.

UI/window-basic-main.cpp Outdated Show resolved Hide resolved
UI/window-basic-main.cpp Show resolved Hide resolved
@gxalpha gxalpha requested a review from Warchamp7 May 23, 2023 11:45
@Penwy
Copy link
Contributor Author

Penwy commented May 23, 2023

The two parts ("add signal and event" and "move hotkey registration") are not technically dependent on each other.

However, if the hotkey isn't moved, saving by pressing the hotkey will not trigger the firing of the frontend event (unless you can access the frontend api from within ffmpeg-mux). Event will only be sent when pressing the UI button, which does not feel to me like wishable behaviour.

@Warchamp7
Copy link
Member

Would also like a @Warchamp7 review on the fact that the save replay buffer hotkey is now always available, even if replay buffer is disabled.

I am okay with this change

@Penwy Penwy force-pushed the replay-buffer-saving branch 2 times, most recently from ee12061 to c9192f5 Compare May 25, 2023 10:23
@WizardCM WizardCM added the Enhancement Improvement to existing functionality label May 27, 2023
@Penwy
Copy link
Contributor Author

Penwy commented Jun 17, 2023

(from what I can see the failed checks are just a result of temporary changes to the CI when this was last pushed, not of the changes introduced by this PR)

@NullFragment
Copy link

I just wanted to selfishly poke over here and see what's up. It'd be nice to have this functionality for using plugins with the replay buffer in the main branch instead of having to build with these changes pulled or use custom build artifacts.

@gxalpha is the only concern with this change right now (other than merge conflicts) that the two commits for moving the hotkey should be squashed into one?

Existing registering of the save hotkey in obs-ffmpeg makes it
impossible for it to trigger a frontend event, and is inconsistent with
other existing output hotkeys.

This registers the save hotkey in the frontend and removes the
registration in obs-ffmpeg, and adds a "due to hotkey" log message,
like other output hotkeys.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improvement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants