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

Video: Fix issues with the window presentation #12000

Merged
merged 4 commits into from Aug 28, 2023

Conversation

Filoppi
Copy link
Contributor

@Filoppi Filoppi commented Jun 27, 2023

  • There was always a black line around one of the 4 edges (top/left/bottom/right) of the window because the final output size wasn't calculated right (unless the aspect ratio was set to stretch)
  • The Auto-Adjust Window Size setting was calculating the window size based on the resolution of the window in the previous frame if we used the "stretch" aspect ratio setting, so it's result would be self influence in a loop and behave unreliably (e.g. when changing resolution between Auto/Native/2x the automatic window scaling would behave randomly)
  • The Auto internal resolution scaling wasn't working correctly if the window weird aspect ratios (e.g. 32:9), beacuse it would account for the the portion of the image that will show black bars into the calcuations to find the best matching resolution
  • The % 4 that was done on the rendering resolution was only meant to be done when recording videos (due to encoding limitations) but one case was missed (this had no consequences really, as it was just in the code that automatically resizes the window). The hardcoded 4 has been replaced with VIDEO_ENCODER_LCM for clarity.

Proof that there was always one black line around the edges of the window (right and bottom):
Dolphin_LS909ueNW8
Dolphin_ExIjNE7sxu
because the resolution/aspect ratio correction code wasn't rounding correctly.

@Filoppi Filoppi marked this pull request as ready for review June 27, 2023 00:32
@Filoppi Filoppi force-pushed the window_res_fix branch 3 times, most recently from fe33981 to c945145 Compare June 27, 2023 09:45
@Filoppi Filoppi force-pushed the window_res_fix branch 2 times, most recently from 326bd05 to 3ba3db1 Compare August 4, 2023 13:03
@AdmiralCurtiss
Copy link
Contributor

Most of this makes sense to me but I don't fully understand what's happening here with the Auto-Window Size + Stretch. In my head this combination doesn't even make sense in the first place, because Stretch implies 'use whatever the current window size is as the output size' and Auto-Adjust Window Size does exactly the opposite, 'change the window size to whatever the output is'. So what exactly are you doing here, effectively calculating the window size as if it was set to Auto? Would it make more sense to just not change the window size at all in this combination?

@Filoppi
Copy link
Contributor Author

Filoppi commented Aug 17, 2023

@AdmiralCurtiss In the case of "Auto-Window Size" + "Stretch", which indeed doesn't make sense, I just basically disable "Stretch" and make it behave like "Auto", because previously they'd mutually influence each other and make the window ever growing or just act very weirdly.
Tbh, my intention was to fix the broken behavior. I think the behaviour I went for is the best solution. Disabling "Auto-Window Size" when "Stretch" is also engaged would be a weird for users I suppose. I think "Stretch" should be seen a setting for the aspect ratio, so it's not entirely related to the auto size, but whatever, as long as it's not broken, it barely matters as nobody should actively use the combination.

@AdmiralCurtiss
Copy link
Contributor

Yeah okay, that makes sense to me.

@Filoppi
Copy link
Contributor Author

Filoppi commented Aug 17, 2023

*Seems like this fails to build on linux due to some issue with the build machine, the PR is fine.

@iwubcode
Copy link
Contributor

Seems like this fails to build on linux due to some issue with the build machine, the PR is fine.

Doesn't look like you rebased yet?

…eant to be done when recording videos (due to encoding limitations) but one case was missed (this had no consequences really, as it was just in the code that automatically resizes the window). The hardcoded `4` has been replaced with `VIDEO_ENCODER_LMC` for clarity.
…eft/bottom/right) of the window because the final output size wasn't calculated right (unless the aspect ratio was set to stretch)
…ow size based on the resolution of the window in the previous frame if we used the "stretch" aspect ratio setting, so it's result would be self influence in a loop and behave unreliably (e.g. when changing resolution between Auto/Native/2x the automatic window scaling would behave randomly)
…y if the window weird aspect ratios (e.g. 32:9), beacuse it would account for the the portion of the image that will show black bars into the calcuations to find the best matching resolution
@dolphin-ci
Copy link

dolphin-ci bot commented Aug 18, 2023

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

  • lego-star-wars-crane-shadow on ogl-lin-mesa: diff
  • lego-star-wars-crane-shadow on sw-lin-mesa: diff
  • lego-star-wars-crane-shadow on mvk-osx-m1: diff
  • lego-star-wars-crane-shadow on mtl-osx-m1: diff

automated-fifoci-reporter

@AdmiralCurtiss
Copy link
Contributor

@Pokechu22 I assume we're fine with the fifoci difference here? It's a slight aspect ratio change which is not unsurprising to me. (In fact, is there a reason we don't use just internal resolution for fifoci output...?)

@Pokechu22
Copy link
Contributor

The fifoci difference seems a bit weird given that it does 640 by 448 EFB copies. I don't entirely understand why things would behave differently for that, since it's a common size (I'd expect more differences, at least).

I don't know why we don't have internal resolution frame dumps enabled for fifoci. My understanding is that "Dump at Internal Resolution" is generally only relevant at higher IRs where the window won't fit on the screen, and I don't know if that even applies to dolphin-nogui.

@JosJuice
Copy link
Member

I think it may be as simple as that setting not yet existing when FifoCI was created :)

@Filoppi
Copy link
Contributor Author

Filoppi commented Aug 19, 2023

Supposedly it was made like this to make the screenshots have the correct aspect ratio? Though to analize differences between pixels, altering the EFB aspect ratio isn't a great thing IMO, but that's a problem for another PR.
Here the window presentation is just using a more appropriate resolution now, and hence the fifoci differences.

@JosJuice
Copy link
Member

"Dump at Internal Resolution" still corrects the aspect ratio, so it's not using the "true" internal resolution. Rather, it tries to make one of the two dimensions match the internal resolution.

@AdmiralCurtiss AdmiralCurtiss merged commit df120b0 into dolphin-emu:master Aug 28, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants