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
Frame dump at raw internal resolution #12436
Frame dump at raw internal resolution #12436
Conversation
d0a2740
to
0f4eef1
Compare
This is okay for FifoCI, but for regular users I think this is unacceptable unless it's put behind a toggle. You say that "it's very easy to size the window to match the target aspect ratio and then set
This would completely break TAS dumping workflows. |
@JosJuice I was indeed wondering what happens if the game changes resolution while recording. Theoretically this was already an issue before, as if Regarding the rest, we could simply make an enum with 3 choices (window resolution, XFB raw resolution, XFB aspect ratio corrected resolution). |
We split the video on resolution changes.
Yes, that sounds fine. |
0f4eef1
to
25bdb56
Compare
25bdb56
to
d92b03c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't just add a dropdown without an accompanying text label, not only it look out of place but it's also inconsistent with the other dropdowns from the Graphics config window (and from other Dolphin dialogs). Here's a suggestion for the text label:
Resolution: [dropdown with the options this PR adds]
@mbc07 it was an oversight, fixed |
d92b03c
to
740abfc
Compare
6432dd5
to
a5cba58
Compare
The fifoci differences are due to the following reasons:
|
e9cfdd4
to
6e8006f
Compare
Might I also make the suggestion to tag frame dump output with the intended aspect ratio? It looks like it can be set in FFmpeg with |
@impiaaa good point. I won't do anything more surrounding these features, given I don't use them, but anybody else could do in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly good to go, but a few comments.
…ut resolution, avoiding any scaling if possible. This should make comparisons much more reliable as pixels wouldn't be smushed together or stretched.
which also avoids the output window from being resized randomly to be a multiple of 4
b7fafea
to
14c3b2c
Compare
That's better. But I have some more notes. Having Dolphin do aspect ratio correction doesn't prevent scaling or post processing in external editing software, as is implied by this description. Raw doesn't "allow" it. Also, grammatically, it should be "at higher quality" not "with a higher quality". I still take issue with quality being mentioned here. This option basically has Dolphin go hands off so video editors have completely control over the pixels of the footage. This could allow them to optimize the pixel clarity of their video, or they just won't apply pixel aspect ratio correction at all and get a worse result than if they had used one of our correction options (which IMO is pretty likely). The whole point of this is that we're giving the user a clean slate for them to manipulate however they so please, which can mean basically anything. So I'm not super happy with saying that this option will improve quality. Cause it may very well not. Furthermore, I don't like the uneven presentation of these options. Window Resolution and Aspect Ratio Corrected Internal Resolution do not describe how to use them or their advantages or anything, they simply state what they are and move on. But for Raw, you continue to talk about its advantages and what users can gain from using it. That, plus positive language, are what gives is the impression that Dolphin wants you to use this and not the others. IMO, you should treat all three options the same way. If you want to matter of fact say what they are, do it for all of them. If you want to describe use cases, do it for all of them. |
@MayImilae please directly write the tooltip exactly as you want :), I already gave my version and proposed some alternatives. I don't think it matters at all, but if you are looking for something very specific, please directly suggest the full edit. Thanks. |
Ok, I'll give it a shot!
How's this? I added to the other options to even it out, and went with the one I showed earlier for Raw Internal Resolution but with some modifications. It's still not completely even between the options but by adding some details to the other ones, the details for Raw Internal Resolution now fit in better, imo. |
14c3b2c
to
d8d948d
Compare
@MayImilae re-uploaded with your text. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it known why the FifoCI outputs have changed? We were previously using internal resolution frame dumps on FifoCI, and that is the new default now as far as I can tell, so I don't know why the image width has changed.
// The aspect ratio corrected XFB resolution (XFB pixels might not have been square) | ||
XFB_ASPECT_RATIO_CORRECTED_RESOLUTION, | ||
// The raw unscaled XFB resolution (based on "internal resolution" scale) | ||
XFB_RAW_RESOLUTION, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CamelCase is recommended for the entries of enum classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have doing all my enums like that recently. There's plenty around :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change it.
also polish aspect ratio related code for clarity
…ping screenshots and not videos (only videos encoders have this limit). NOTE: this will likely trigger FIFOCI differences.
d8d948d
to
66592f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description and UI LGTM.
This has been sitting here for ages, works fine, and is a useful improvement to the options. |
db619ed
to
2f13be5
Compare
Allows the frame dumper to use the raw emulation output (XFB) resolution, avoiding any scaling if possible.
This should make comparisons much more reliable as pixels wouldn't be smushed together or stretched.
Currently even if
bInternalResolutionFrameDumps
is enabled, dumps are done after scaling to the target aspect ratio (which is almost always different from the raw emulation render), so they aren't really raw, they are scaled, but they scaling isn't done with gamma correction as the post process effects pipeline doesn't run on them.This PR adds an enum with 3 dumping resolution choices:
Some possible future improvements related to this:
VIDEO_ENCODER_LCM
(4) limit, or move it to the frame dumper and just make it add black pixel lines of padding on the edges instead of forcing images to stretch.The FifoCI differences are expected.
The 2nd commit moves the adjustments to the dumping resolution into the actual dumping function, so the window output resolution isn't affected by recording anymore (previously it would snap to the closest multiple of 4 when recording, which was unnecessary).
The 4th commit allows screenshots to be taken at any resolution, without the unnecessary
VIDEO_ENCODER_LCM
(4) limit.