-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
obs-ffmpeg: Add end action UI property for local file source #6239
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
base: master
Are you sure you want to change the base?
Conversation
|
When changing properties like this, you need to account for existing setups in the wild. You should cleanly migrate such settings to the equivalent value in the new settings or version the source such that old versions still work unchanged while newly added sources use the new behavior. |
7bf8778 to
4aaba5b
Compare
|
I update the patch. And let the previous property "looping", but hide it. |
|
I'm not 100% sold on terminology. I think at the very least, "Freeze on final frame" (if that's what happens) would be better than "Freeze". If the "Hide" action doesn't actually toggle the eye to automatically hide the source, then I'd like to avoid the term "Hide" in favour of the previous "Show nothing". I'm unsure how better to communicate the term "End Action" without it being very long. "When Playback Ends.." maybe? I don't know. |
Yes we can use Show nothing, who avoid confusion with show/hide property. Another name for the "Freeze" action (who keep the last frame), can be "Frame hold" (is something close to what Premiere Pro used for this), or "Freeze last frame" who is shorter than "Freeze on final frame".
I think shorter text are cleaner for UI. But if you prefer another text, will update. |
|
Will this function finally auto disable Media Sources which finshed playing? Streamdeck is a pain to use with OBS Studio right now as Media Sources stay active, event they finished playing. IF changing scenes, all Medisa Sources will re-start which is a pain. |
|
This PR doesn't change the existing behaviour (it change UI in local file mode). |
yes I was referring to local video files indeed. So with this new "Hide" option, after the local Video has played, it would hide the local media source in OBS right? Is there a way I could test that allready witihout having to compile myself for win 64 bit? I guess it requires only 1 updated dll |
|
There is no new options in this PR, only a proposal for reorganize two checkbox to a property list with 3 parameters. Currently the behaviour is : Hide = Show nothing when playback ends (Checked) / Loop (Unchecked)
Sorry didn't know how to enable test build in a PR. |
|
@Username69992872 I added the required label and restarted CI, once it's finished you should be able to download test artifacts from the "Checks" section of this PR. |
|
Show nothing tooltip: |
|
Hello, Thanks for your comments. Don't see a function to add tooltip to a property list item. |
Warchamp7
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this as a UX improvement.
Only suggestions I have are to phrasing and keeping familiar wording where we can.
End Action -> End of Playback
Freeze -> Freeze on last frame
Hide -> Show Nothing
For local file, replace UI properties : "Loop" and "Show nothing when playback ends" by a property list call End of Playback with three possible values : Show Nothing, Freeze on last frame, Loop Keep previous looping property for compatibility with previous version
4aaba5b to
ae96c4a
Compare
|
Don't forget to explain to translators that "loop" is verb here. |
How to do that ? |
|
The way you can do this:
|
| end_playback = 1; | ||
| } | ||
|
|
||
| obs_data_set_int(settings, "end_playback", end_playback); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably also want to use obs_data_erase to remove the old setting when upgrading it to the new variant. Otherwise the old setting will persist in a user's configuration and you will always go and update it.
Probably worthwhile to check if the old setting exists first and only go through the effort to upgrade when it's necessary in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A "safer" method would be to only upgrade the setting if the old setting exists and the new setting does not. Erasing old setting values completely breaks configs for older versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that expected? If I were to create the source anew after the change, I could not go back to an older version of OBS and expect the setting to be "downgraded" either.
I'm aware of programs commonly upgrading settings but not that these upgraded settings are still expected to work with older versions (new settings would just be ignored).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not saying that the upgraded settings should work with old versions. I'm saying that if you open a newer OBS one time that deletes settings entirely, and then you open an older OBS because you only wanted to run the newer OBS once to check it out, your settings are gone. I'm aware that we don't want to encourage or support downgrading, but purposefully breaking stuff seems a bit risky for users who just want to test stuff (and don't know that we encourage using portable mode or backing up your settings beforehand).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with me, there's no issue with keeping it. The property should be removed nevertheless though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with me, there's no issue with keeping it. The property should be removed nevertheless though.
Agree with that.
| dstr_free(&path); | ||
|
|
||
| obs_properties_add_bool(props, "looping", obs_module_text("Looping")); | ||
| prop = obs_properties_add_bool(props, "looping", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the property is upgraded, it should not be retained and hidden. As I outlined above, the config value should be upgraded when it exists, but from then on this property will not have any use.
Properties can and will be queried by other code which will not take visibility into account, so for those consumers it will be confusing that there is a bespoke property for looping and another one that also enables looping.
For local file, replace UI properties :
"Loop" and "Show nothing when playback ends"
by a property list call End Action
with three possible values : Hide, Freeze, Loop
Description
Currently in Obs FFmpeg source, when using local file
the end action of a video file, can be set to three mode,
using two checkbox (loop and Show nothing when playback ends)
If loop is choose, the checkbox "Show nothing when playback ends", do nothing.
The UI proposal, is to use a property list, with the three mode when local file is checked
Doesn't change the UI when Local file is unchecked.
Internally, doesn't change the way is_looping and clear_media_end works.
Motivation and Context
Having something more easy to understand for user, to define end action of a local video file.
How Has This Been Tested?
Mac os 11.6.2
Mac intel, Xcode
Tested using local file, and checking the three mode.
Types of changes
Checklist:
I'm not sure it's the cleanest way to implement it.
Mainly for UI change for non local file, and using integer or enum for property list.