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

libobs: Add new signals for video reset #11686

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

walker-WSH
Copy link
Contributor

@walker-WSH walker-WSH commented Jan 1, 2025

Description

If obs_reset_video is called in OBS, for 3rd plugins, they should re-create the obs_output or video_output hold in their code. Otherwise, 3rd plugins will easily crash since their video_output and obs_output::video_t have been deleted in obs_reset_video.

With this commit, 3rd should no longer access their video output when they receive "core_video_released";
They should re-create video output hold in code or reset video_t in their obs_output when "core_video_ready" arrives.

Motivation and Context

if a 3rd plugin hold their obs_output or video_output, crash will easily happen after below operation:

install ndi plugin
open settings of OBS, to video page
modify resolution
click OK
Tool menu of OBS, select NDI, start its output, crash
the reason can be found in DistroAV/DistroAV#1096

Except NDI plugin, I also found this kind of crash on other plugin, such as vertical-canvas, source-record.
If OBS provide this new front event, 3rd plugin has the chance to reset their obs_output or video_output.

How Has This Been Tested?

settings -> video -> modify resolution -> click OK -> check if this event can be sent

Types of changes

New feature (non-breaking change which adds functionality)

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.

@exeldro
Copy link
Contributor

exeldro commented Jan 1, 2025

For obs_free_video I think a signal before the free instead of after the free would be better, so something like signal per obs_view_t* like void view_destroy(ptr view). This would make it the same as source signals.
I would expect a main view initialized signal with a pointer to the obs_view_t* of the main view, so this can be void view_add(ptr view) or void view_create(ptr view) signal to keep it the same as sources.

@walker-WSH
Copy link
Contributor Author

walker-WSH commented Jan 1, 2025

@exeldro

  1. I agree with you that release signal should be sent before free logic, I will modify it.

  2. in obs_reset_video, all video_t and obs_core_video_mix will be released.
    when 3rd receives release signal, they should clear their all video_t returned by obs_view_add
    So I am not sure if it is useful about view_destroy(ptr view). how do you think it?

  3. "core_video_ready" is just for notifying main view is added and 3rd can get the new main video_t by obs_get_video.
    Do you really need main obs_view? since currently no API to expose it, only video_t of main view is exposed.

@walker-WSH
Copy link
Contributor Author

walker-WSH commented Jan 1, 2025

I have modified the timing of sending release signal

@WizardCM WizardCM added the New Feature New feature or plugin label Jan 4, 2025
@WizardCM
Copy link
Member

WizardCM commented Jan 5, 2025

I am unable to reproduce a crash using the provided reproduction steps. Please attempt on a fresh OBS installation of OBS 31.0.0 and provide the exact version of DistroAV used.

Additionally, please provide the crash log (stack trace).

@walker-WSH
Copy link
Contributor Author

@WizardCM this crash still reproduce on obs-31.0 and ndi-6.0.0.

it has also been reproduced by others in DistroAV/DistroAV#1096

have you checked if your ndi output is working normally? you can add a ndi source to check its output view in OBS.

@WizardCM
Copy link
Member

WizardCM commented Jan 5, 2025

@WizardCM this crash still reproduce on obs-31.0 and ndi-6.0.0.

it has also been reproduced by others in DistroAV/DistroAV#1096

have you checked if your ndi output is working normally? you can add a ndi source to check its output view in OBS.

Yes, my NDI output is working correctly. Verified in an NDI source. No crash occurs for me when changing resolution and enabling the output.

@walker-WSH
Copy link
Contributor Author

@exeldro

  1. I agree with you that release signal should be sent before free logic, I will modify it.
  2. in obs_reset_video, all video_t and obs_core_video_mix will be released.
    when 3rd receives release signal, they should clear their all video_t returned by obs_view_add
    So I am not sure if it is useful about view_destroy(ptr view). how do you think it?
  3. "core_video_ready" is just for notifying main view is added and 3rd can get the new main video_t by obs_get_video.
    Do you really need main obs_view? since currently no API to expose it, only video_t of main view is exposed.

@Lain-B @WizardCM

According to my SRE statistics, these third-party output plugins are widely used:
ndi
mult-rtmp-output
source_record
vertical_canvas

@exeldro developed both of the last two.
maybe we should discuss his above requirement. I am not sure whether my idea is right

@walker-WSH
Copy link
Contributor Author

walker-WSH commented Jan 5, 2025

@WizardCM this is my operation record

crash.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Feature New feature or plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants