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: Remove duplicate missing files code #5148

Merged
merged 1 commit into from
Jul 18, 2022

Conversation

cg2121
Copy link
Contributor

@cg2121 cg2121 commented Aug 18, 2021

Description

The same code was in different places, so call single function to
cleanup code.

Motivation and Context

Code cleanup.

How Has This Been Tested?

Tested with missing files to make sure dialog still worked properly.

Types of changes

  • Code cleanup (non-breaking change which makes code smaller or more readable)

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.

@WizardCM WizardCM added the Code Cleanup Non-breaking change which makes code smaller or more readable label Aug 18, 2021
UI/window-basic-main.cpp Outdated Show resolved Hide resolved
UI/window-basic-main.cpp Outdated Show resolved Hide resolved
@norihiro
Copy link
Contributor

There was a bug that the missing file dialog cannot catch missing files on transitions. However, this PR fixed the issue.

The step to reproduce the bug is as below.

  1. Add a stinger transition. Set a video file.
  2. Exit OBS
  3. Remove the video file.
  4. Start OBS
    • Without this change, Missing Files dialog does not appear.
    • With this change, Missing Files dialog report a file is missing from the stinger transition.
  5. With this change, update the file path on the dialog.
  6. Run the stinger transition and confirmed the updated video file is played.

I've made another PR #6753 but the PR #6753 is unnecessary if this PR is merged.

UI/window-basic-main.cpp Outdated Show resolved Hide resolved
The same code was in different places, so call single function to
clean up code.
@gxalpha gxalpha added the Bug Fix Non-breaking change which fixes an issue label Jul 18, 2022
@gxalpha gxalpha added this to the OBS Studio 28.0 milestone Jul 18, 2022
@gxalpha gxalpha merged commit 6eb5a92 into obsproject:master Jul 18, 2022
@norihiro
Copy link
Contributor

I'm afraid this change is dangerous in terms of thread safety. Prior to this change, the sources have not been revealed to the graphics thread when checking the missing files. Now, the missing files are checked after the graphics thread started to access the sources. The graphics thread also touch the data when defer_update_count is set.

Especially, some sources such as clut_filter calls obs_source_update during the create callback, which triggers first tick in the graphics thread to call update callback and modify pointers for example char *file in clut_filter. Assuming #5401 will be merged, UI thread will read the same pointer so that this is a race condition.

I'd like to suggest reverting this change and also do not call obs_source_update from the create callback but directly call each callback function for update.

cg2121 added a commit to cg2121/obs-studio that referenced this pull request Jul 20, 2022
With obsproject#5148, it was
brought up that the loading of the missing files was not thread
safe. This PR fixes that problem, while still simplifying the
original missing files code.
cg2121 added a commit to cg2121/obs-studio that referenced this pull request Jul 20, 2022
With obsproject#5148, it was
brought up that the loading of the missing files was not thread
safe, as the missing files were being loaded late in the loading
process. This PR tries to fix that problem, while still simplifying
the original missing files code.
@cg2121 cg2121 deleted the missing-files-cleanup branch July 26, 2022 15:17
cg2121 added a commit to cg2121/obs-studio that referenced this pull request Jul 26, 2022
With obsproject#5148, it was
brought up that the loading of the missing files was not thread
safe, as the missing files were being loaded late in the loading
process. This PR tries to fix that problem, while still simplifying
the original missing files code.
WizardCM pushed a commit that referenced this pull request Jul 27, 2022
With #5148, it was
brought up that the loading of the missing files was not thread
safe, as the missing files were being loaded late in the loading
process. This PR tries to fix that problem, while still simplifying
the original missing files code.
@obsproject obsproject deleted a comment Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Non-breaking change which fixes an issue Code Cleanup Non-breaking change which makes code smaller or more readable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants