-
Notifications
You must be signed in to change notification settings - Fork 36
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
fix 'No enumerators available' - on Windows 10 #32
base: master
Are you sure you want to change the base?
Conversation
Fixes rr-#30 dc_full is a pointer, if interpretted as a signed number may be negative. Null -> 0 indicates failure. Set return types for foriegn function so return value can be compared to None. ReleaseDC was not being called with enough arguments, it should also take the same Window Handle (HWND) as GetDC.
screeninfo/enumerators/windows.py
Outdated
# HDC -> HANDLE -> PVOID -> void * | ||
# See https://docs.microsoft.com/en-us/windows/win32/winprog/windows-data-types | ||
GetDC = ctypes.windll.user32.GetDC | ||
GetDC.restype = ctypes.c_void_p |
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.
Is it necessary to set restype
by hand?
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.
Necessary, probably not. Without this, the type returned with my environment is c_long. It could be checked against 0 instead, but it's a loss of type safety.
From my understanding it's recommended by ctypes documentation on foreign functions
I don't think there is any way that ctypes
could determine the types for these methods. You can check what information is embedded in the dlly by checking the dumpbin.exe /EXPORTS
output for user32.dll
screeninfo/enumerators/windows.py
Outdated
# See https://docs.microsoft.com/en-us/windows/win32/winprog/windows-data-types | ||
GetDC = ctypes.windll.user32.GetDC | ||
GetDC.restype = ctypes.c_void_p | ||
dc_full = GetDC(hwnd) |
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.
Is it necessary to convert None
to c_void_p
by hand?
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.
Not necessary. I did this for two reasons:
- Using the same variable to when getting the DC and when releasing makes helps to clarify that it supposed to be the same value.
- Specifying the types make it clear what is expected by the function, and that the type has been checked to be correct and intentionally set. Otherwise it's just relying on ctypes to guess and get it right.
Pinging @hhannine for a review, as it contains his code. |
I probably won't put anymore effort into this PR. Please make modifications as you see fit, or close it. |
Thanks for honest answer |
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.
Works for me.
Maybe even resolved the GetDC failure we saw before since it doesn't seem to use any retries. Though it did fail detecting physical sizes once (on the first try(?)) in a bunch of get_monitors calls.
I've now gotten a couple more physical size detection failures, I wonder what causes them.. It goes through consistently on the first try though. Edit: Resolution detection has worked correctly every single time however. |
@hhannine could we rework the loop to cater for the new findings? |
I need to catch some intermediate step data to see if I can find if anything is different when the detection fails. Couldn't get anything yet. |
When the size detection fails I'm getting dc_full values
and when it succeeds they're more like
Is this size discrepancy a sign issue and this is again the failure with negative dc_full values? For whatever reason I'm getting failures at a higher rate now than I got yesterday. |
Those values suspiciously all start with the same digits, are also larger than a 32 bit void pointer (max 4 bytes). To me this indicates memory corruption. The first thing I would do is confirm the types for all the dll functions, and specify them according to ctypes documentation on foreign functions When you say "size detection fails" do you mean the calls to https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getmonitorinfow |
Thanks for taking a look. I'm having trouble debugging this further: no exceptions are raised so the only output I get of the failure is that the physical sizes and display names are all None. And as I try to add some prints to tell me what the data are none of the prints come through when this failure occurs, only when the enumeration is successful. Even a print on the first line of the callback is not printed if the failure occurs and yet it returns the resolutions.. Any ideas what is going on? Would it not return anything I'd believe that the call to EnumDisplayMonitors fails based on the data it's getting but since there's some output I don't know what to make of this. I also noticed that 2^64 = 18446744073709551616, notably close to the strange values I listed above. Is this a typing issue for dc_full? |
I added typing to all the dll function calls. While doing this I found that the types specified for |
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.
Thanks for the changes, they look very good.
Just two questions:
- is it OK to name
hwnd = HDC
? Aren't HWNDs and HDCs semantically different things (even if under the hood they are of the same primitive type and size)? - Is it OK to pass None to
EnumDisplayMonitors
without explicit conversion like we do withHDC(None)
?
Good catch, I'll fix that.
I hoped that ctypes would complain if you gave it the wrong type (now that |
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.
👍 @hhannine could you run some tests on your machine to see if we still need that loop?
Thanks for taking the time to make things rigorous! Unfortunately however, I'm not getting any physical sizes or display names at all. First line print from the callback is never printed. |
However now that I changed Edit:
|
And with that fix, how many loop iterations do you need to get the good values? |
I ran into a new exception that I added above, and it now fails the enumeration of that specific display that it occurs on and I'm getting an incomplete list on monitors as an output. |
Now after some testing I got again a |
It seems that now with the new changes it is possible to get bad
|
Can you print out the type of |
With
Does this help or did you want me to do something ctypes specific? |
Adds more typing.
It didn't give me any leads. I can't reproduce the issue that you're seeing in either python 3.7.3 or 3.8.2. I have fixed the issue with One thing that is worth trying is moving the creation of the |
How unfortunate, I'm still getting the issue with the new changes. I also updated to python 3.8.2 but that did not help either. Is this then a ctypes regression or something if you don't have the issue? Hardware specific? Should we reimplement the checking of dc_full for being of moderate size, whatever the spec says it should be instead of a 64-bit integer? As a workaround like we have in the current release? I was messing around and I think I might have worked around the issue by testing if a dc is larger than 2^32 and if it is, subtracting 2^64 from it, yielding actual output...
|
The above workaround does seem to work reliably for |
I can't reproduce the issue with bad I added the following to the print(f"MonLen {len(monitors)} - got dc {dc}")
if dc is not None and dc > pow(2, 32):
# Got a valid DC, break.
print(f"callback error: {dc}") and the following to right after print(f"Retry {retry} - got dc_full {dc_full}")
if dc_full > pow(2, 32):
# Got a valid DC, break.
print(f"GetDC error: {dc_full}") I called for i in range(100):
for m in enumerate_monitors():
print(f"{i} - {str(m)}") I only have one monitor. All values of A few questions that might shed light on this:
It might be a bug in the ctypes library, but without being ale to reproduce the problem it's very difficult to be sure or debug. |
|
With Python3.7
I got out
No prints from callback there. When it succeeds:
|
Also on
I got after a while of testing
No prints from callback or exceptions. |
I'm also getting bad |
By the way, by writing a loop of calls of get_monitors I'm not getting refreshed dc_full values and a large number of calls can succeed in succession with the same good dc_full. I need to restart python and do calls with some time delay to sometimes get new dc_full values. I've tried also deleting the pycache and it might help getting new dc_full values, maybe not I don't know. Getting a bad |
This fix worked on my windows 10 system. |
To everyone involved, is this PR good to merge? |
Can you please squash the changes? |
Fixes #30
dc_full
is a pointer, if interpretted as a signed number may it be negative despite the call toGetDC
being successful.Failure is indicated by
NULL
->0
. This PR sets the return type for the foriegn functionGetDC
so the value can be compared to None.GetDC
docs: https://docs.microsoft.com/en-gb/windows/win32/api/winuser/nf-winuser-getdcReleaseDC was not being called with enough arguments, it should also
take the same Window Handle (HWND) as GetDC.
ReleaseDC
docs: https://docs.microsoft.com/en-gb/windows/win32/api/winuser/nf-winuser-releasedc