-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Allow HWND to be passed to ImageGrab.grab() on Windows #8516
base: main
Are you sure you want to change the base?
Conversation
3c59997
to
72e3f68
Compare
src/display.c
Outdated
@@ -331,14 +331,18 @@ PyImaging_GrabScreenWin32(PyObject *self, PyObject *args) { | |||
HMODULE user32; | |||
Func_SetThreadDpiAwarenessContext SetThreadDpiAwarenessContext_function; | |||
|
|||
if (!PyArg_ParseTuple(args, "|ii", &includeLayeredWindows, &all_screens)) { | |||
if (!PyArg_ParseTuple( | |||
args, "|ii" F_HANDLE, &includeLayeredWindows, &screens, &screen |
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.
This seems to be stealing a reference to the HDC and then deleting it, which could violate the DeleteDC
documentation, https://learn.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-deletedc#remarks:
An application must not delete a DC whose handle was obtained by calling the GetDC function. Instead, it must call the ReleaseDC function to free the DC.
It would also be more intuitive to accept a HWND argument and get the HDC with the GetDC
function or the GetWindowDC
function (making sure to release it with ReleaseDC
as stated in the documentation).
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.
Ok, I've updated the commit to receive a HWND and use ReleaseDC
.
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.
FWIW not all windows can be captured like this. Notably, Vivaldi (my browser) only shows as a black box of the correct size, and the Windows file explorer is missing its toolbar. Other applications, e.g. PyCharm or Microsoft Spy++, are captured correctly. But I think that is just a limitation of modern Windows applications and nothing we can easily fix.
src/PIL/ImageGrab.py
Outdated
|
||
def grab( | ||
bbox: tuple[int, int, int, int] | None = None, | ||
include_layered_windows: bool = False, | ||
all_screens: bool = False, | ||
xdisplay: str | None = None, | ||
handle: int | ImageWin.HWND | None = None, |
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.
handle: int | ImageWin.HWND | None = None, | |
window: int | ImageWin.HWND | None = None, |
or
handle: int | ImageWin.HWND | None = None, | |
window_handle: int | ImageWin.HWND | None = None, |
seems clearer.
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.
Ok, I've chosen window
.
@@ -420,7 +441,11 @@ PyImaging_GrabScreenWin32(PyObject *self, PyObject *args) { | |||
PyErr_SetString(PyExc_OSError, "screen grab failed"); | |||
|
|||
DeleteDC(screen_copy); | |||
DeleteDC(screen); | |||
if (screens == -1) { | |||
ReleaseDC(wnd, screen); |
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.
You added this for the error:
branch but not for the success branch.
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.
Ok, I've updated the commit to add it or success as well.
src/display.c
Outdated
@@ -351,11 +362,17 @@ PyImaging_GrabScreenWin32(PyObject *self, PyObject *args) { | |||
dpiAwareness = SetThreadDpiAwarenessContext_function((HANDLE)-3); |
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.
We need to match the capturing thread DPI awareness to the target window DPI awareness which can be queried with GetWindowDpiAwarenessContext (also added in Windows 10 version 1607): https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getwindowdpiawarenesscontext
Without it I get a black border around applications without native DPI scaling support (e.g. Microsoft Spy++).
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.
Ok, I've pushed a commit.
Tests/test_imagegrab.py
Outdated
with pytest.raises(OSError): | ||
ImageGrab.grab(handle=-1) |
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.
with pytest.raises(OSError): | |
ImageGrab.grab(handle=-1) | |
with pytest.raises(OSError): | |
ImageGrab.grab(handle=-1) | |
with pytest.raises(OSError): | |
ImageGrab.grab(handle=0) |
The value 0 is a special way to refer to the desktop. However, it cannot be captured in the same way as regular windows so it fails, but we can add a test for 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.
Thanks. I've added a commit.
src/display.c
Outdated
GetWindowDpiAwarenessContext_function(wnd); | ||
if (dpiAwarenessContext != NULL) { | ||
dpiAwareness = | ||
SetThreadDpiAwarenessContext_function(dpiAwarenessContext); |
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.
It should probably never happen that SetThreadDpiAwarenessContext
is found but GetWindowDpiAwarenessContext
is not.
However, if it somehow does happen, we would then use dpiAwareness
before its initialized after measuring the window size. So we should ideally either fallback to PER_MONITOR_AWARE context if GetWindowDpiAwarenessContext
is not found, or skip the reset a few lines later.
…available Co-authored-by: Ondrej Baranovič <[email protected]>
for more information, see https://pre-commit.ci
Co-authored-by: Andrew Murray <[email protected]>
Fix GetWindowDpiAwarenessContext NULL check
Resolves #4415
Adds a
handle
argument toImageGrab.grab()
that accepts a HDC, allowing for a screenshot of a specific window, rather than the entire screen.