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

Override configuration based on video mode #567

Merged
merged 2 commits into from
Mar 17, 2022
Merged

Override configuration based on video mode #567

merged 2 commits into from
Mar 17, 2022

Conversation

tsowell
Copy link
Contributor

@tsowell tsowell commented Mar 11, 2022

This adds support for overriding configuration variables for specific video modes. The intended purpose is to support automatically switching to an output mode that is best-suited to the core's video mode. It's especially useful for letting CRTs do their own scaling for "rectangular pixel" modes like 320x200.

Overrides are specified at the end of MiSTer.ini in sections with names in the format video=WIDTHxHEIGHT[@VREFRESH].

For example:

[video=320x200]
video_mode=640,16,96,48,400,12,2,35,25175
vscale_mode=2

[video=720x400@70.1]
video_mode=720,15,108,51,400,11,2,32,28322

When a core changes video mode, MiSTer checks for a configuration section matching the new mode and swaps in the new configuration if there is a match. Sections that match width, height, and vertical refresh rate take precedence. Then if there is no match, it looks for a section matching width and height but with no vertical refresh rate specified.

By the way, I tried to take a light touch with the changes to cfg.cpp, and one drawback is that it stores a copy of the entire cfg struct for each video mode override section... which is a bit wasteful. Let me know if you'd like me to take a different approach.

@sorgelig
Copy link
Member

When you add a feature, don't rewrite the whole code. Remove all stl code, please.

@tsowell
Copy link
Contributor Author

tsowell commented Mar 11, 2022

Thanks for looking at this and responding. What do you think about doing this by reloading the configuration file, like how the core sections work? I think that would meet your requirements. Do you see a problem with doing that every time the video mode changes?

@sorgelig
Copy link
Member

sorgelig commented Mar 12, 2022

if there is no resolution sections used then code should avoid to reload of INI, so it should work exactly as before changes, so users not using this feature won't be affected by any possible side effect. Reloading ini gives some latency in processing of other FPGA requests and input devices. If core will change resolution together with some other events (like reading CD data) then it may impact the work. I cannot tell how it will affect the work in reality...

Configuration options can now be overridden based on the core's video
mode.

The config parser now supports sections with names in the format
"video=WIDTHxHEIGHT[@VREFRESH]".  When a core changes video mode, MiSTer
reloads the config file and checks for a section matching the new mode.
If one is found, the options in the section override options in the
MiSTer/core sections.

MiSTer will look for a section matching the width, height, and vertical
refresh rate first.  If none is found, it will fall back to a section
matching the width and height but not specifying a refresh rate.  If
there is still no match, no overrides will be used.

Also, VREFRESH must match exactly the output from video_info or the
logs.  To match 720x400 31.48KHz 70.1Hz, the section title would need to
be [video=720x400@70.1].
@tsowell
Copy link
Contributor Author

tsowell commented Mar 16, 2022

Thanks - I was worried about impacting users who are not using the feature, so that's a great idea.

I just pushed a version that works by reloading INI and only reloads if there were video sections in the initial load. Where possible, I tried to make it consistent with the way the core sections work. Please let me know if you have any other thoughts.

@sorgelig
Copy link
Member

why HAS_VIDEO_SECTIONS and USING_VIDEO_SECTION added as possible options in ini? According to code they are calculated variables.

@tsowell
Copy link
Contributor Author

tsowell commented Mar 17, 2022

Just to make use of the existing cfg interface, but I understand why that wouldn't be the right place for them. Here's a commit that moves them into separate variables.

@sorgelig sorgelig merged commit f5fd16c into MiSTer-devel:master Mar 17, 2022
@sorgelig
Copy link
Member

i didn't mean to move them. Just don't expose them into INI file would be fine. Anyway..

theypsilon pushed a commit to MiSTer-unstable-nightlies/Main_MiSTer that referenced this pull request Mar 17, 2022
* Override configuration based on video mode

Configuration options can now be overridden based on the core's video
mode.

The config parser now supports sections with names in the format
"video=WIDTHxHEIGHT[@VREFRESH]".  When a core changes video mode, MiSTer
reloads the config file and checks for a section matching the new mode.
If one is found, the options in the section override options in the
MiSTer/core sections.

MiSTer will look for a section matching the width, height, and vertical
refresh rate first.  If none is found, it will fall back to a section
matching the width and height but not specifying a refresh rate.  If
there is still no match, no overrides will be used.

Also, VREFRESH must match exactly the output from video_info or the
logs.  To match 720x400 31.48KHz 70.1Hz, the section title would need to
be [video=720x400@70.1].

* Move video section variables out of cfg_t

(changes from upstream commit f5fd16c)
dentnz pushed a commit to dentnz/Main_MiSTer that referenced this pull request May 12, 2022
* Override configuration based on video mode

Configuration options can now be overridden based on the core's video
mode.

The config parser now supports sections with names in the format
"video=WIDTHxHEIGHT[@VREFRESH]".  When a core changes video mode, MiSTer
reloads the config file and checks for a section matching the new mode.
If one is found, the options in the section override options in the
MiSTer/core sections.

MiSTer will look for a section matching the width, height, and vertical
refresh rate first.  If none is found, it will fall back to a section
matching the width and height but not specifying a refresh rate.  If
there is still no match, no overrides will be used.

Also, VREFRESH must match exactly the output from video_info or the
logs.  To match 720x400 31.48KHz 70.1Hz, the section title would need to
be [video=720x400@70.1].

* Move video section variables out of cfg_t
dentnz pushed a commit to dentnz/Main_MiSTer that referenced this pull request May 12, 2022
* Override configuration based on video mode

Configuration options can now be overridden based on the core's video
mode.

The config parser now supports sections with names in the format
"video=WIDTHxHEIGHT[@VREFRESH]".  When a core changes video mode, MiSTer
reloads the config file and checks for a section matching the new mode.
If one is found, the options in the section override options in the
MiSTer/core sections.

MiSTer will look for a section matching the width, height, and vertical
refresh rate first.  If none is found, it will fall back to a section
matching the width and height but not specifying a refresh rate.  If
there is still no match, no overrides will be used.

Also, VREFRESH must match exactly the output from video_info or the
logs.  To match 720x400 31.48KHz 70.1Hz, the section title would need to
be [video=720x400@70.1].

* Move video section variables out of cfg_t
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants