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

Allow HWND to be passed to ImageGrab.grab() on Windows #8516

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

radarhere
Copy link
Member

@radarhere radarhere commented Oct 31, 2024

Resolves #4415

Adds a handle argument to ImageGrab.grab() that accepts a HDC, allowing for a screenshot of a specific window, rather than the entire screen.

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
Copy link
Contributor

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).

Copy link
Member Author

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.

@radarhere radarhere changed the title Allow HDC to be passed to ImageGrab.grab() on Windows Allow HWND to be passed to ImageGrab.grab() on Windows Nov 4, 2024
Copy link
Contributor

@nulano nulano left a 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.


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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
handle: int | ImageWin.HWND | None = None,
window: int | ImageWin.HWND | None = None,

or

Suggested change
handle: int | ImageWin.HWND | None = None,
window_handle: int | ImageWin.HWND | None = None,

seems clearer.

Copy link
Member Author

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);
Copy link
Contributor

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.

Copy link
Member Author

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);
Copy link
Contributor

@nulano nulano Nov 4, 2024

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++).

Copy link
Member Author

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.

Comment on lines 62 to 63
with pytest.raises(OSError):
ImageGrab.grab(handle=-1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Member Author

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);
Copy link
Contributor

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.

src/display.c Outdated Show resolved Hide resolved
nulano and others added 3 commits November 7, 2024 22:22
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
Fix GetWindowDpiAwarenessContext NULL check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows application screenshot
2 participants