fix(windows): Lazy-init desktop and memory DCs#516
fix(windows): Lazy-init desktop and memory DCs#516mxschmitt wants to merge 2 commits intoBoboTiG:mainfrom
Conversation
|
Hm one the test failure (https://github.com/BoboTiG/python-mss/actions/runs/25008585510/job/73238170128?pr=516#step:6:268) seems not OK, but it also seems to be unrelated to your changes. Or at least, your changes show an unexpected usage of MSS that could occur in other projects. @halldorfannar how does it sound to you? |
|
I don't have enough background in this area but I feel like the per-thread DC via threading.local sounds reasonable? Investigation report: https://gist.github.com/mxschmitt/cfa1b5f7ae60265450f5f45c5edeaa37#failure-23-test_same_object_multiple_threads--releasedc-returns-0 |
Let me take a look! |
|
Instead of delaying the acquisition of DCs we should try to acquire them as before (inside |
|
To iterate on your proposal @halldorfannar: let's use a try/except block in |
|
@BoboTiG yes, this will work. A try/except block around the acquisition of both DCs would be required and inside the except we would set both to None. Then raising a runtime error from |
|
(Unrelated to that PR) And about that kind of error: It seems to take the path https://github.com/BoboTiG/python-mss/blob/v10.2.0/src/mss/windows/gdi.py#L123-L127 if I read it correctly. Should we improve our logic there? |
Isn't this error due to the potential thread mismatch I was talking about? Do you want the error reporting improved? Maybe I'm just not understanding what you are trying to tell me 😄 |
|
Maybe it's not clear enough in my head too, haha! We check for I'm trying to get more information if we should enhance our error reporting, or if we just ignore it and move forward. |
|
I'll readily admit that I don't really understand the proposed change. Is there consensus, or are the guys who actually know Windows (i.e., everybody but me) still working it out? The thread mismatch does concern me somewhat. The MSS docs currently state that you can pass around MSS objects all you want between threads, and there's an implication that you can close them in a different thread than you opened them. I do see that the GetDC and ReleaseDC need to be paired on the same thread, per the docs. I'm not seeing anything about CreateCompatibleDC and DeleteDC, though. Do those need to be paired? If not, we could keep memdc for the lifetime of the MSS object, and do the GetWindowDC and ReleaseDC on srcdc in grab. I'm guessing those are cheap operations? Of course, we'd want to measure it, but this might be a reasonable solution. |
|
Additionally: I see that we delete self._dib while it's still selected into self._memdc, in A fresh memory DC, I'm led to understand, has a default 1x1 bitmap selected into it. We'll get that back when we select self._dib into self._memdc. I suggest that we save that (as self._placeholder_dib or self._old_dib or whatever) when we call SelectObject. Then, whenever we're about to delete self._dib, we select that default dib into memdc first. But again, I may be completely wrong here. I was alerted to this issue by an AI, and so before acting on that, it should be considered by somebody who knows Windows GDI better than I. |
OK, I think I understand our misunderstanding now. The call did fail, it was just that Windows didn't set last error. I will improve the messaging. It's confusing, I see that now. I can just do that as a separate PR. The error we are seeing here has to do with thread requirements when it comes to creating and releasing DCs. This is why the current PR as written is not safe. |
|
For the overall PR I want to perform some benchmarks to compare a few different strategies for allocating/freeing the necessary resources. That will inform us on how we can allow monitor enumeration (as this PR does) while still safely acquiring/releasing the Windows objects. |
|
The error handling improvements are in #519 - turns out we had three problems 😅 Once merged you will have a much cleaner error log from this PR. |
|
And #519 is merged. Can you rebase your PR @mxschmitt? |
|
Reporting back on my benchmarks. The overhead of acquiring and releasing the device contexts is minimal. I've measured on 60hz, 75hz, and 120hz configurations. We should therefore acquire and release memdcs/srcdc within the |
596a3f4 to
f2758ae
Compare
Move GetWindowDC(0) + CreateCompatibleDC out of __init__ and into each grab() call, releasing them in a try/finally before returning. Benefits: - MSS() construction no longer calls GetWindowDC(0), so sct.monitors works even when the desktop DC is unavailable (locked screen, UAC, RDP disconnect, GDI handle pressure) - DCs are acquired and released on the same thread within the same call, eliminating the cross-thread ReleaseDC failures seen in CI Fixes BoboTiG#509.
|
Could you add a line or two in |
- Rewrite regression test to simulate GetWindowDC failure and verify monitors still work while grab() properly raises ScreenShotError - Add changelog entry to v11.0.0.md
f2758ae to
7fc867e
Compare
Fixes #509.
Problem
MSSImplGdi.__init__unconditionally callsGetWindowDC(0)andCreateCompatibleDCatsrc/mss/windows/gdi.py:208-209. Both can fail withWinError 5: Access is deniedunder:As a result, even read-only use like
with mss.MSS() as sct: sct.monitorscrashes in these environments — despitemonitors()only needingEnumDisplayMonitors/GetMonitorInfoW, neither of which requires a desktop DC.Fix
Defer
GetWindowDC(0)andCreateCompatibleDCto the firstgrab()call via a new_ensure_dcs()helper.close()already guards onNone, so no cleanup changes are needed. The change is backwards compatible —grab()still works identically; the DCs are just created on demand.Test plan
test_monitors_does_not_call_getwindowdcinsrc/tests/test_windows.pythat wrapsGetWindowDCwith an assertion so any call duringsct.monitorsfails the test.ruff checkpassesruff format --checkpasses