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

Optimize the selection of pixel format in WGL to early exit if found #916

Merged
merged 2 commits into from
Oct 23, 2023
Merged

Optimize the selection of pixel format in WGL to early exit if found #916

merged 2 commits into from
Oct 23, 2023

Conversation

dtrebilco
Copy link
Contributor

Another startup saving on Windows - around ~20ms unless you select a crazy multisampling number that cannot be found.
Also removes a temp buffer allocation.
Same change could be applied to GLX looking at code. (not familiar enough to test myself)

@floooh
Copy link
Owner

floooh commented Oct 17, 2023

Apologies for not responding yet. I'll need to focus on getting the WebGPU backend finally merged, but after that I'll give the PR a whirl.

@dtrebilco
Copy link
Contributor Author

Just an additional note: I see a minor issue with the GLX startup in existing code. If ARB_multisample is not supported, this code is not run
if (_sapp.glx.ARB_multisample) { u->samples = _sapp_glx_attrib(n, GLX_SAMPLES); }
As the init of "u" goes through _sapp_gl_init_fbconfig () - the samples value will be left as -1 when passed to _sapp_gl_choose_fbconfig. (or _sapp_gl_select_fbconfig if you use my new code). I don't think this method is built to handle the values being negative).

This does not affect the Windows code as my recent change assigns it to 0 even if the extension does not exist.

@floooh
Copy link
Owner

floooh commented Oct 23, 2023

Starting to test this PR now...

@floooh
Copy link
Owner

floooh commented Oct 23, 2023

PS: I'm not bothering with GLX fixes too much at the moment, since it will probably be dropped with the alternative EGL initialization in place (first step would be to make EGL the default, and after a while drop GLX completely).

@floooh
Copy link
Owner

floooh commented Oct 23, 2023

Btw... I would suggest creating a dedicated branch for each PR on your side in the future :)

(makes it a bit easier for me to test those changes on my side, e.g. I already have branch called dtrebilco-master here)

floooh added a commit that referenced this pull request Oct 23, 2023
@floooh floooh merged commit 0ad28e3 into floooh:master Oct 23, 2023
23 checks passed
@floooh
Copy link
Owner

floooh commented Oct 23, 2023

Ok merged! I did a couple of small coding style fixes (also one indentation I missed in the last PR):

1db0431

@floooh
Copy link
Owner

floooh commented Oct 23, 2023

PS: many thanks for the PR!

@dtrebilco
Copy link
Contributor Author

OK, I'll try - I am not really up on how I should use github - I just create a fork for each new pull request I do to projects here.
I don't have any other pending changes.

FYI: This was all done/found while doing a line by line conversion to Rust of Windows startup code - I don't plan on taking it further, was just a leaning exercise to see how easy it was or what issues I would have.

https://github.com/dtrebilco/TestLoad/blob/main/src/sapp.rs

@floooh
Copy link
Owner

floooh commented Oct 23, 2023

Basically, if you are on the master branch of your sokol fork, just create a new branch with a somewhat descriptive name:

git checkout -b wgl-optimize-select-pixelformat

...or I think the new-fangled method is:

git branch wgl-optimize-select-pixelformat

...then implement the change in there and create a pull request from that branch.

Then when the pull request gets merged you can delete that branch and sync your fork from the upstream sokol repo:

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

PS:

I just create a fork for each new pull request I do to projects here.

That works too to avoid problems on your side, but in this case I had an old local branch from your previous PR around and had a name-collision (that's how I usually merge PRs, instead of just pressing the Merge button on the GH side - I only do this for very trivial PRs).

It's not a big deal though, I just deleted the old intermediate branch.

@floooh
Copy link
Owner

floooh commented Oct 26, 2023

FYI: I'm seeing a pretty serious problem on my old Asus laptop with integrated Intel GPU with this fix (GL programs are stuck at startup with a white screen). I'll try to investigate, but maybe I need to revert this PR.

@floooh
Copy link
Owner

floooh commented Oct 26, 2023

The difference between your code and the old code seems to be that the old code returns 28 from _sapp_wgl_find_pixel_format(), and the new code returns 27.

Some sort of off-by-one error?

@floooh
Copy link
Owner

floooh commented Oct 26, 2023

Ok, the difference between pixel format 27 (which is broken) and format 28 (which works) is the doublebuffer flag.

Format 27 has doublebuffer=false (which doesn't work), and format 28 has doublebuffer=true.

@floooh
Copy link
Owner

floooh commented Oct 26, 2023

Ah found the issue, the u.doublebuffer here will only ever be set to true, but never reset to false:

sokol/sokol_app.h

Lines 6825 to 6827 in 4eb208b

if (query_results[result_double_buffer_index]) {
u.doublebuffer = true;
}

...in the old code u wasn't reused, so there it worked correctly.

I'll implement a fix straight in master.

@floooh
Copy link
Owner

floooh commented Oct 26, 2023

Ok, fixed in 85d6541

@dtrebilco
Copy link
Contributor Author

Opps - sorry - initially I had that struct initialized in the loop like the old code when testing.
Pulling it out of the loop I thought was fine.

@floooh
Copy link
Owner

floooh commented Oct 26, 2023

tbf the original code with the if (...) was a bit wonky too ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants