-
Notifications
You must be signed in to change notification settings - Fork 49
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
Common: Rework Movies and Game color rendering #575
Common: Rework Movies and Game color rendering #575
Conversation
Thanks for the PR, I'll try to look at it more in detail this weekend, in the meantime, please drop this commit as it's unrelated to this PR, thank you: |
OK, I reverted the revert. If you end up reverting the revert of the revert, please take care that the two calls to setOverallColorGamut() relating to the logo don't get lost along the way. Git gave me merge conflicts in both directions with them. |
Please drop the commit, do not "revert the revert". Consider using interactive rebase and drop the commit. Then add the calls you need to make sure the PR is fully functional. Thank you. |
The first revert incorporates resolution of a merge conflict. If I delete it from the git history, the history will no longer include a record of the fully working state the PR started in. You might want to be able to recover that if the problems with 3437968 prove to be unfixable. Are you sure you want me to delete that? The current state of the PR is identical to 2 commits back, so a git reset would suffice. It should be in a working state except for the black screen bug with the logo. |
Yes please, I don't want to mix things here. This PR is very valuable but I'd like to keep it focused on the color resolution. Put the logo part aside for now. Thank you |
a190f27
to
c70b366
Compare
I finally found some time to look at this PR more carefully and first of all thank you for the massive work you've done. Is really appreciated. On top of it, I have only these three main concerns:
Atm I would consider focusing on 1 and 3 on your own side, and let's wait for Cosmos to come back for 2. Thank you very much in advance in the meantime, looking forward to it :) |
I've said before that this PR is not severable. The movie stuff depends on the general-purpose gamut stuff and general shader clean-up. This is ALL movie stuff, some of it just happens to have broader applicability. I know this isn't the answer you want. But I can't divide the indivisible.
I think I was unclear earlier. This PR makes zero changes to the lighting code and zero changes that should impact the lighting code. There is an additional change that I would like to make if it could be done without breaking lighting. But I don't believe that's possible, so that additional change is not in this PR. I do want @CosmosXIII's input on whether that potential change would be possible, but it's not necessary for merging this PR.
I'm not clear on whether you're unhappy with the LUT approach in general, or with the LUTs being independent files. As to the former, I don't see any other alternative. The math is just too heavy. Gamutthingy takes several minutes to convert a single HD image on CPU, with the use of a 400MB memo table for repeated colors. I don't believe it's possible to implement that for GPU with real-time frame rates. Also, it would require binding ~50MB of gamut boundary descriptor data as a uniform, which is pretty much the same thing as binding a LUT. I'm not aware of any other solution to "the math is too heavy for real-time" besides pre-computation via a LUT. As to the latter, I don't see why it matters. I don't see any downside to having them as independent files. (And I don't understand what "too meta" means.) The only downside I see to embedding them into the binary is that then the users couldn't customize them. So, I don't really care. I wouldn't object to you embedding them in the binary, if you want to, but I'm not inclined to do it. |
Thanks for the reply. Regarding the first part I'm still trying to analyze how much complexity I'm bringing into the code, also considering I might have to maintain it if you might happen to not be around even though I'd appreciate having you onboard like other core members are contributing to the project, but I'm calculating the worst case scenario here. I hope you do understand why I'm asking so many questions and why I'm trying to simplify every contribution. Regarding the second part, I see you're touching the lighting shader nevertheless so for me it's enough to wait for Cosmos to give me his opinion, as it's his code being touched there. About the third part:
How much do you expect users replacing them for something else? Isn't this a one-stop solution? Like merge, enable and enjoy? What would be the benefit of allowing users replacing these files? It's just for me to understand. Once I understand this part I may think about which would be the best way. There are ways we could allow users replace them, and we looking for the file, but if not found on the disk we could use the embedded resource. I know for you it doesn't matter, but it does for me as I'd like to keep the distribution as simple as possible and having those files inside the shaders folder feels like an abuse to me ( even though it's kinda related, I know ).
This although is not the attitude I'm looking forward for when working with someone. I appreciate your idea and I'm open on working on it, but the work has to be mutual. If you saw first Cosmos PRs I asked him to rewrite the entire code three times until it made sense for the current FFNx structure. I hope you understand I'm not the only one working on this code, and I have to accomodate many needs. I hope this will make your reconsider being collaborative. There are still minor topics to be fixed which I spotted that are about indentation for eg. so I hope if I rise them you'd be open to work on them. |
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.
Left some comments on various parts. I'll try to test the code later today and I'll let you know. I've seen you've touched the renderer part too and that one is a little bit delicate as it impacts also FF8. I'll keep you posted.
I tested this PR on both FF7 and FF8 and these are the results: FF7:
FF8:
I tested both movies and gameplay and I didn't notice anything strange, including animated textures :) Great job so far. LMK if you want to address the indentation issues or if I shall. Additionally, I'd like to know if there's margin to improve the glut texture loading part. Once all of that is done and adressed, we can proceed merging this PR. The only part I missed to test is the Lighting part, so any feedback from @CosmosXIII is really appreciated. |
@julianxhokaxhiu Tested with lighting and looks fine to me. IMO the shader movie code is now so complex that it would be better to make it it's own shader for both performance and maintenance reasons. But maybe that is better left out for another refactor PR. About the color improvement, I can see the difference in the sense that it is more saturated but for me the biggest improvement is that now the movies have no longer crushed blacks whether you use the NTSC-J option or not. |
I did it this way because it was simple and I figured the RAM cost was negligible. It should be possible to do better. In general, we want to have the required LUT loaded before we need it to be confident in avoiding stuttering. However, unless you're using movies with color gamuts that are unusual for our use case, you're likely to only use a maximum of 2 LUTs per session. So, I could rework things to pre-load the LUTs you're likely to need based on HDR and gamut mode settings, then lazy load the others in the unlikely event you need them. Please give me a few days.
Please address the indentation issues. My editor "solves" the "tabs vs spaces" indent problem by displaying them exactly the same, but for a barely visible mark on the tabs. (With my eyesight, basically invisible.) In one sense, this is good because I don't even notice when viewing a file with mixed indents. However, it makes it really hard to fix a file with mixed indents, and easy to generate one accidentally.
Most from-scratch mods inhabit a no-man's land where neither gamut mode will be quite right. To the extent the modder stuck closely to the original palette, NTSC-J mode should be correct. To the extent the modder livened up the colors, sRGB mode should be correct. Unfortunately, most from-scratch mods mostly stuck the original palette, but with some departures that are brighter/more saturated. The ninostyle mods stick surprisingly close to the original palette (see here), so I think NTSC-J mode comes closest to "what the game was originally supposed to look like, but with better models." Though, ultimately, that's a really subjective judgment.
I'm probably not the right person for that job, since I know so little about BGFX that I'd probably break things trying to add a new shader pass. Anyway, I agree that's best left for a refactor.
There should be some colors that the HDR monitor can produce and the SDR monitor can't. Try comparing a really saturated green and it should look "greener" on the HDR monitor. (Though I must say that I'm really proud that the LUT is doing a good enough job in SDR that you couldn't tell the difference.) Somewhat related to that: Is the autodetection for "this HDR monitor's nits for SDR white" working correctly? That's not something I was able to fully test, since I have neither Win10/11 nor a HDR monitor. The log file should say what happened. (This code starts around line 800 in renderer.cpp) |
Sure thing, thank you :)
Agree as well.
The intro movie with stars was rendered correctly here when using HDR so I'd say yes. Well done :) |
bc7a1df
to
585119e
Compare
I just pushed a commit for lazy loading LUTs. I gave it a very brief test and it seems to be working correctly. |
* Add SMPTE170M linearization function FMVs are likely bt601 or bt709, so we need this to properly convert them to linear RGB. * Use SMPTE170M linearization function if YUV texture is a movie * Use SMPTE170M linearization function if YUV texture is a movie
…with precise ones, clean up indentation.
… saturate() calls to make sure matrix operations stay in bounds
… 10-bit correct by lying about range, but only if file has no range metadata. Instead telling the truth about range gets 8-bit files correct. Needs more work to find workaround for swscale doing unwanted extra range conversions.
(1) Prevent unwanted, incorrect color range conversions by swscale (2) Check colorspace and do conversion if needed (as yet untested) (3) Use YUV444 for swscale output (4) Use better scaling for chroma planes
…d use SMPTE170M when appropriate. (Works great with 10-bit formats. Some posterization near black with 8-bit formats. Not sure if this is an encoder issue or a swscale issue.)
…IFF input is TV-range and > 8-bit depth.
…on this HDR monitor" rather than "maximum brightness of this HDR monitor." Revised documentation in FFNx.toml accordingly. Replaced autodectection code with new implementation using DISPLAYCONFIG_DEVICE_INFO_GET_SDR_WHITE_LEVEL DisplayConfigGetDeviceInfo() query. Untested, since I don't have a HDR monitor or Win10.
…white point on Japanese television sets was actually 9300K+27mpcd, and (b) to use higher precision D65 white point for sRGB. Also added NTSCJ-to-rec2020 gamut conversion matrix
… parameter to to dithering in order avoid using the same dither pattern twice in the case in the case of a TV-range video played on HDR.
2. Move HDR gamut conversions up as early as possible. 3. Implement plumbing for gamut conversions for textures controlled by metadata, pending support for said metadata. 4. Apply NTSC-J to sRGB (or rec2020) gamut conversion for rendering polygons of solid color/gradients.
…ce()" This reverts commit 26aaa65.
This reverts commit 72c329e.
…n to enable "NTSC-J Gamut Mode." This isn't ideal because it necessitates double gamut conversions for videos in some cases, but it seems to be the only way to do gamut conversions on gameplay without breaking animated textures. (Also some cleanup and reorganizing.)
…e original games do.)
…ils among colors that are out-of-bounds for sRGB. Also, fix dithering to protect values near 0 and 1.
585119e
to
d7ef1d9
Compare
We did it! Thanks a lot for the amazing work and energy you put on this one, really appreciated! |
Summary
Motivation
Fix FMV playback and colorimetry. Ended up with a general purpose NTSC-J mode along the way.
ACKs