google / filament

Filament is a real-time physically based rendering engine for Android, iOS, Windows, Linux, macOS, and WebGL2
https://google.github.io/filament/
Apache License 2.0
17.69k stars 1.87k forks source link

SetPixelFormat is never called on the nativeWindow HDC #163

Closed robbiesri closed 6 years ago

robbiesri commented 6 years ago

Describe the bug The HDC that is drawn to and presented never has a HGLRC matching pixel format set on it. So when wglMakeCurrent is called from the HDC sourced from the ExternalContext::SwapChain/nativeWindow and the HGLRC generated in ContextManagerWGL::createDriver, wglMakeCurrent rebels and throws an error. On my most recent test machine, the native window HDC has a pixel format of 0 (sure), and the HGLRC was created with an HDC that had a pixel format of 11 set on it (indices could change across implementations).

In order to fix this issue, I just made sure the same pixel format that was used in ContextManagerWGL::createDriver is used when ContextManagerWGL::createSwapChain is called. I just bind the same pixel format to the HDC passed in. This is easy enough to do because the pixel format is static, and contained to ContextManagerWGL.cpp/ContextManagerWGL. In the future, if you ever decide to try other pixel formats, it should probably be stored somewhere in ContextManagerWGL.

I'm planning on submitting a PR, if for nothing else, as a code reference on what the issue is and how to fix it.

To Reproduce Steps to reproduce the behavior:

  1. Sync to master@a779e9
  2. Build material_sandbox (and dependencies) on Windows 10 x64 machine
  3. Run material_sandbox with the monkey.obj (material_sandbox ....\assets\models\monkey\monkey.obj)
  4. Crash in ContextManagerWGL::makeCurrent because wglMakeCurrent fails with ERROR_INVALID_PIXEL_FORMAT (0x7D0). I used GetLastError to grab the error after failure (maybe the code should do this in failing cases as additional context during failures? I guess I could file a FR for this...).

Expected behavior Application shouldn't crash? 😉 If I set a matching pixel format, sample app runs happily.

Desktop (please complete the following information):

Additional context I'm assuming this was tested on NV hardware. I suspect this happens enough where NV might just conveniently work around this case if they realize the user forgot to set the pixel format explicitly on the HDC. They would probably qualify the HDC by checking if any pixel format was ever set on it, and if not, it should be ok to just override the HDC pixel format with the pixel format associated with the HGLRC. Just speculation though.

I wrote some notes down here on this Google Doc, but it mostly just thought scribbles. I just started looking at this code today, so I was keeping notes as I navigated. Not necessary viewing: https://docs.google.com/document/d/1gWPSG4G-xS35oOVRr6cRF5y8jSCdxYu2_htWm6oFUNw/edit?usp=sharing

romainguy commented 6 years ago

Thank you, this completely explains #42. Unfortunately we only have NV GPUs right now but the issue you're describing makes complete sense.

Is the fix to simply call SetPixelFormat on the HDC passed to createSwapChain? I haven't done any Windows programming in almost 15 years, I'm rusty :))

robbiesri commented 6 years ago

Is the fix to simply call SetPixelFormat on the HDC passed to createSwapChain?

That's what I think. It fits in with the setup process for GL/WGL. The real questions are how you manage the PFD and/or pixel format index (static in the file, stashed in the ContextManagerWGL instance, probs more). I'll have the PR up in a bit, but player's choice on how to actually fix it 😄

Thanks for linking it back to #42. I just skimmed it, totally missed the issue with wglMakeCurrent in there, my b.

romainguy commented 6 years ago

Closed by PR #165