win-capture: Add option to adjust hook rate for game capture #1479
Conversation
There was a problem hiding this comment.
Hi! Congrats on your first PR here! To help out, I've reviewed this PR, mostly for conformance with this project's style guidelines.
Other than my inline remarks, please also wrap your commit message body (the paragraph under the initial commit message line/subject) at 72 characters.
plugins/win-capture/game-capture.c
Outdated
| HOOK_RATE_SLOW, | ||
| HOOK_RATE_NORMAL, | ||
| HOOK_RATE_FAST, | ||
| HOOK_RATE_FASTEST, |
There was a problem hiding this comment.
There's an unneeded comma after the last enum item. Remove it.
plugins/win-capture/game-capture.c
Outdated
| static inline float hook_rate_to_float(enum hook_rate rate) | ||
| { | ||
| switch (rate) { | ||
| case HOOK_RATE_SLOW: return 2.0f; |
There was a problem hiding this comment.
Style: Generally, avoid putting multiple statements on a single line. In this case, split case and return statements onto separate lines.
plugins/win-capture/game-capture.c
Outdated
| case HOOK_RATE_FAST: return 0.5f; | ||
| case HOOK_RATE_FASTEST: return 0.1f; | ||
| case HOOK_RATE_NORMAL: | ||
| default: return 1.0f; |
There was a problem hiding this comment.
I believe a "fallthrough" indicator is needed above this to avoid a GCC-7 warning.
plugins/win-capture/game-capture.c
Outdated
| @@ -537,7 +565,8 @@ static void *game_capture_create(obs_data_t *settings, obs_source_t *source) | |||
|
|
|||
| gc->source = source; | |||
| gc->initial_config = true; | |||
| gc->retry_interval = DEFAULT_RETRY_INTERVAL; | |||
| gc->retry_interval = DEFAULT_RETRY_INTERVAL* | |||
There was a problem hiding this comment.
Style: Typically, you need to surround math operators by a single space on each side. Don't leave a trailing space at the end of the line either, so in this case:
gc->retry_interval = DEFAULT_RETRY_INTERVAL *
plugins/win-capture/game-capture.c
Outdated
| @@ -537,7 +565,8 @@ static void *game_capture_create(obs_data_t *settings, obs_source_t *source) | |||
|
|
|||
| gc->source = source; | |||
| gc->initial_config = true; | |||
| gc->retry_interval = DEFAULT_RETRY_INTERVAL; | |||
| gc->retry_interval = DEFAULT_RETRY_INTERVAL* | |||
| hook_rate_to_float(gc->config.hook_rate); | |||
There was a problem hiding this comment.
Style: Tab twice to indent a line continuation.
plugins/win-capture/game-capture.c
Outdated
| @@ -1614,7 +1644,7 @@ static void game_capture_tick(void *data, float seconds) | |||
| return; | |||
|
|
|||
| } else if (!gc->showing) { | |||
| gc->retry_time = 10.0f; | |||
| gc->retry_time = 10.0f*hook_rate_to_float(gc->config.hook_rate); | |||
There was a problem hiding this comment.
Style: You need to surround math operators by a single space on each side. Doing this will exceed the line length limit, so also line-break this line.
plugins/win-capture/game-capture.c
Outdated
| @@ -1640,7 +1670,8 @@ static void game_capture_tick(void *data, float seconds) | |||
| gc->error_acquiring = true; | |||
|
|
|||
| } else if (!gc->capturing) { | |||
| gc->retry_interval = ERROR_RETRY_INTERVAL; | |||
| gc->retry_interval = ERROR_RETRY_INTERVAL* | |||
There was a problem hiding this comment.
Style: Typically, you need to surround math operators by a single space on each side. Don't leave a trailing space at the end of the line.
plugins/win-capture/game-capture.c
Outdated
| @@ -1640,7 +1670,8 @@ static void game_capture_tick(void *data, float seconds) | |||
| gc->error_acquiring = true; | |||
|
|
|||
| } else if (!gc->capturing) { | |||
| gc->retry_interval = ERROR_RETRY_INTERVAL; | |||
| gc->retry_interval = ERROR_RETRY_INTERVAL* | |||
| hook_rate_to_float(gc->config.hook_rate); | |||
There was a problem hiding this comment.
Style: Tab twice to indent a line continuation.
plugins/win-capture/game-capture.c
Outdated
| @@ -1655,7 +1686,8 @@ static void game_capture_tick(void *data, float seconds) | |||
| debug("init_capture_data failed"); | |||
|
|
|||
| if (result != CAPTURE_RETRY && !gc->capturing) { | |||
| gc->retry_interval = ERROR_RETRY_INTERVAL; | |||
| gc->retry_interval = ERROR_RETRY_INTERVAL* | |||
There was a problem hiding this comment.
Style: Typically, you need to surround math operators by a single space on each side. Don't leave a trailing space at the end of the line.
plugins/win-capture/game-capture.c
Outdated
| @@ -1655,7 +1686,8 @@ static void game_capture_tick(void *data, float seconds) | |||
| debug("init_capture_data failed"); | |||
|
|
|||
| if (result != CAPTURE_RETRY && !gc->capturing) { | |||
| gc->retry_interval = ERROR_RETRY_INTERVAL; | |||
| gc->retry_interval = ERROR_RETRY_INTERVAL* | |||
| hook_rate_to_float(gc->config.hook_rate); | |||
There was a problem hiding this comment.
Style: Tab twice to indent a line continuation.
This option slows down or speeds up the rate at which the game capture plugin checks for a valid window.
|
Lots of style comments! Apologies, I've never worked with C that much so I mostly just tried to copy the style of the files. Should've just read the proper guide. I addressed all points you mentioned except for the fallthrough issue. I could not get it to compile with a [[fallthrough]] indicator, and I can't find any in the surrounding code so I presume that it is a feature from a later update than what OBS runs on. I added a comment instead to signify that it is intentional. Alternatively I can simply remove the default case and have it return the default value in case no switch condition applies, if that would be preferred? As for the commit message, is the current message a problem? I believe that the current, equal length message: reads nicer than the enforced wrap at 72 characters: Or did I misinterpret the style guidelines? To me it was purely an aesthetical choice, since it is shorter than 72 characters regardless. |
|
Regarding the "fallthrough" issue, see Pull Request #935. I do not believe "[[fallthrough]]", specifically, is supported in all compilers we currently use, which is why we use the comment mechanism instead. I build primarily on Windows, so I don't use GCC that often. You should be able to verify if this is needed by compiling with GCC 7.
I'm not sure what compiler optimizations occur with a default case compared to assigning a default outside of the switch. It's probably better to have the default case. As for the commit message, the specific style rule in question is:
I suppose one could interpret that as "wrap the commit body at 72 characters or less". I don't know Jim's preference on this one. I usually just recommend wrapping at 72, but you can keep it this way until Jim has time to review it. |
|
I don't really like adding too many advanced options like this, but I do acknowledge that the default hook retry interval is quite large. In the future, I think some of these more advanced options need to be segregated from the simple options. A redo of the settings here would be ideal in the future. |
|
Not sure if this will still emit a warning in GCC7 for the fallthrough syntax used. |
|
After reviewing the GCC documentation on warning options, it looks like this will be fine for -Wimplicit-fallthrough=n where n is anything from 0 to 4. It's not consistent with the other comments introduced in PR #935, but that's a nitpick. |
I run an automatic stream in which the game is frequently restarted, and OBS's retry interval on finding the game window was so long that you'd often miss the first few seconds.
This pull request adds an option to adjust the speed at which the game capture plugin tries to find a valid window.
It may be a niche option, but it improved my stream, so I figured I'd see if there was interest to include it.
I tried to make the option easy to understand by making it a simple slow/normal/fast/fastest setting.
The code mentions that trying too fast can cause crashes, e.g. by Steam, which is not a concern in my case. I have not encountered any problems thus far with using the fastest setting. Regardless, the default is unchanged from the existing retry interval and the option recommends to leave it at that.