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

Add RFC: Window Capture Rewrite (Windows) #1

Closed
wants to merge 1 commit into from

Conversation

WizardCM
Copy link
Member

@WizardCM WizardCM commented Nov 6, 2019

Description

Rewrite Window Capture on Windows to be 1) more consistent 2) more user friendly 3) don’t capture a random window as a fallback 4) better support for strange windows

Motivation and Context

Window Capture is one of the core capabilities of OBS, and yet its behaviour can be wildly unpredictable (and sometimes even downright dangerous). It has a number of flaws. Check the RFC for full details.

View the RFC

@palana
Copy link

palana commented Nov 7, 2019

@WizardCM: you may want to include a link to the rendered rfc in your description: rendered

@skeletonbow
Copy link

For me, the desired behaviour of window capture would be to have options to specify the window desired as clearly and unambiguously as possible, and have the defaults OBS uses if you don't specify something be to not show anything unless it is 100% sure that it is what you want. ie: never fall back to some other window that is not what you asked for, as that can leak all kinds of personal information, etc.

I watched a stream once where an OBS window capture failed and switched to a copy of Notepad.exe with a list of 50 or so game keys used for giveaways for example, even though the window capture was set to some video game EXE. The streamer was mortified.

@UserNaem
Copy link

UserNaem commented Nov 10, 2019

I agree with everything except the drawbacks:
It's true that changing the default behaviour will likely require some users to modify their scene collections, but you can always just not remove the "old" window capture, this will solve any and all edge-cases that require the old behaviour. Obs-studio already exposes two options for text rendering and two (or three) for video rendering, and no one seems to complain about this, or care about the increased startup times and memory footprint this incurs.

As for user confusion, that's easily solved by explicitly telling them to select a window. Some applications tell the user "I need your inputs here" by highlighting the button that needs pressing, I think this should solve any confusion.

@alinsavix
Copy link

Windows should be sorted alphabetically (by window title, not exe).

Is this actually the best sort? For example, if I have a bunch of Chrome windows open, alphabetizing by window name means those chrome windows are scattered all over the list (since each website has its own title)... sorting by exe name at least makes all my chrome windows group together...

Or perhaps an alternate view where you select a running exe name and it lists all the windows of that particular exe?

The user should be able to drag an icon from the Properties window onto the window they want to capture, instead of finding it in the list. “It’s right there, I just want to capture it”

Similar to this, there should also be a "capture foreground window on keypress" option, similar to what game capture does. "I'm using this window, I just want to capture it"

Either allow the user to define the behaviour when a window is lost, or choose a sane default

I think part of the "sane default" should be never, ever pick a window running from a different exe than the window that just lost, since I can't imagine any use case where that would be sane. (Perhaps overridable (explicitly) when using a title/regex match or something. Not sure what makes the most sense)

@VodBox VodBox mentioned this pull request Jan 1, 2020
lsamayoa pushed a commit to lsamayoa/rfcs that referenced this pull request Apr 1, 2020
@jp9000
Copy link
Member

jp9000 commented Apr 11, 2020

In the future, combine the "problem" and "solution" of each item if possible to keep things more readable. Now on to the issues:

  1. When a Window Capture source is initially created, it defaults to the first window in the list. If the user is currently streaming, this is bad - it can expose personal data because it’s unpredictable.
  1. By default, Window Capture should default to “No window selected”. While this’ll be different than Display Capture defaulting to the first monitor (user can minimise sensitive windows beforehand), and Game Capture defaulting to “Any fullscreen game” (games are less likely to contain sensitive data), it makes sense for the capture type. This ensures that the user always manually selects which window is exposed.

This was fixed. I think. If not, it's a good suggestion, and I say someone should just make a pull request to fix it. It should probably mimic what game capture does for window selection.

  1. When a Window Capture source’s window is lost, OBS by default tries to find a “similar” window. However, “similar” can vary wildly depending on the application. Say you’re capturing a specific Chrome window. If OBS loses that window even for a second, it’ll try and find another Chrome window - one that may expose unwanted data.
  1. Either allow the user to define the behaviour when a window is lost, or choose a sane default: freeze on the last good frame, or go invisible (the second is likely better feedback to the user).

What do you really do when you have something that has the window name change constantly? It'd be annoying. Anyway all this would require is changing the default for "Window Match Priority" to "Window title must match". It would require a v2 if it was changed because it changes default behavior. Are you really sure we should do that when so many windows change their title? I made it auto-select windows of the same type way back when in OBS classic because people complained about their window titles changing. Should probably ask other people to comment on this one for consensus on this.

  1. When the user clicks “Cancel” without changing any settings, the Window hook is “refreshed”, causing a brief flash to viewers. The assumption is that if nothing has changed, and the user decided to “discard” the changes, nothing should change.
    • On a related note, the same behaviour doesn’t happen if the user clicks “OK” without changing any settings.
  1. Clicking Cancel should not flash the window. This could be that Cancel tries to “undo” all changes, and because nothing has changed, everything gets unset then set again, which causes the flash. Ideally, it should compare before/after and if everything is the same, do nothing.

This should have been fixed already. Probably could have created a PR for this without even mentioning it in an RFC, but almost totally positive I fixed this.

  1. Minimised windows are not included in the Windows dropdown. While this may be a good way to exclude “hidden” windows, it can cause user confusion because the window is “open”.
  1. Minimised windows should be included in the dropdown, always. This is a pretty easy change on its face, but edge cases like hidden minimised windows should be tested.

The reason I did this originally is because people sort of expect it to start capturing something, and when it's minimized it can't capture. And games that are fullscreen won't capture especially, which I think was the biggest reason. We could do it, it's fine with me, but just get ready for support messages about it. Should probably ask for consensus on this.

  1. The list of windows is not automatically refreshed, and provides no way for the user to manually refresh them.
  1. The list should refresh when the user opens the dropdown, and there should be a simple “Refresh” button near the dropdown.

This is a good suggestion, but refreshing the properties is kind of a convoluted process because the obs_properties system sucks. I agree it does need a way to refresh though. I think we just need a way to create properties with Qt directly (i.e. not using the automatically generated properties) because trying to do this with the properties system might not worth the pain.

  1. The list of windows is not sorted in any meaningful way, making it hard to find a window, especially if the user attention is supposed to be elsewhere.
  1. Windows should be sorted alphabetically (by window title, not exe).
    • The dropdown should have “search/filter” capabilities
    • The user should be able to manually type in the name of the window, in the rare cases where they know the window title in advance and would rather Window Capture always capture that window
    • The user should be able to drag an icon from the Properties window onto the window they want to capture, instead of finding it in the list. “It’s right there, I just want to capture it”

I don't mind sorting, it's a good suggestion. But it'd have to have a multi-tiered sorting, like executable first, then by title. But anyway, do people usually have that many windows open that it becomes a problem otherwise? Currently I'm pretty sure it's just sorted by Z-order. Anyway, we can't do this without custom Qt properties (i.e. not using the automatically generated properties). Unless we want a load of pain and more exceptions to that thing.

  1. Windows that change their title often (eg. Dolphin emulator) can cause performance issues because OBS tries matching by title, which (sometimes) means unhooking and re-hooking, or repeatedly changing details about the hook unnecessarily.
  1. Users have suggested a “Regex” or “Partial match” mode, where they include the non-changing part of the window title, and if that window isn’t lost, don’t try and rehook. Alternative solutions are welcome.

Feels more like a bug report. It's not supposed to uncapture once it's found a window handle to start capturing. You'd have to provide reproduction steps. Once it has the window it shouldn't be searching for new windows. It just keeps capturing the current window handle until the window has been closed/destroyed, and then and only then should it start searching again.


In general, most of these are fine. Two require custom Qt properties. Two you should probably have consensus on (indicated above) and be very sure you want to be ready for support messages about them. One was already fixed, first one should have a PR created to fix, and the others are probably fine.

@WizardCM
Copy link
Member Author

Closing this as most issues mentioned have since been fixed, and the rest can be smaller PRs and therefore a rewrite isn't necessary.

@WizardCM WizardCM closed this Feb 20, 2021
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.

6 participants