feat: Enhance take_screenshot for multi-monitor support #792

onyedikachi-david · 2024-06-22T06:36:30Z

Fixes #766
/claim #766

What kind of change does this PR introduce?

Feature

Summary

This PR introduces support for multi-monitor setups in the take_screenshot function. It includes the following changes:

Implemented get_current_monitor to determine the monitor where the cursor is currently located.
Modified take_screenshot to use get_current_monitor for capturing the correct monitor.
Added comprehensive tests for take_screenshot with multiple monitors.
Mocked get_current_monitor and mss.mss in tests to simulate multiple monitor configurations.
Ensured take_screenshot correctly handles multiple monitors and returns the expected screenshot.

Checklist

My code follows the style guidelines of OpenAdapt
I have performed a self-review of my code
If applicable, I have added tests to prove my fix is functional/effective
I have linted my code locally prior to submission
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation (e.g. README.md, requirements.txt)
New and existing unit tests pass locally with my changes

How can your code be run and tested?

To run and test the code:

Ensure you have the necessary dependencies installed.
Run the test suite using pytest:

pytest tests/openadapt/test_monitors.py

Verify that all tests pass, including the new tests for multi-monitor support.

Other information

No additional context needed.

…onitor support

abrichr · 2024-06-22T13:48:37Z

Thank you @onyedikachi-david ! What do you think about saving taking a screenshot of all of the available monitors, rather than just the active one? This can provide useful additional context to the model if something changes on one of the screens.

onyedikachi-david · 2024-06-22T22:25:10Z

Thank you @onyedikachi-david ! What do you think about saving taking a screenshot of all of the available monitors, rather than just the active one? This can provide useful additional context to the model if something changes on one of the screens.

Okay, I'll implement it. just that I was being mindful of how it is being used across the codebase. If there are more than one monitors, then a tuple or list will be returned, which will affect the screenshot function usage across the codebase.

abrichr · 2024-06-23T00:24:06Z

Thank you please do. Maybe keep the current functionality behind a config param.

onyedikachi-david · 2024-06-23T01:03:38Z

Thank you please do. Maybe keep the current functionality behind a config param.

New take_screenshot function using config:

def take_screenshot() -> list:
    """Take screenshots of the current monitor or all available monitors.

    Returns:
        list of PIL.Image.Image: A list of screenshot images.
    """
    with mss.mss() as sct:
        monitors = sct.monitors
        screenshots = []
        
        if config.CAPTURE_ALL_MONITORS:
            for monitor in monitors[1:]:  # Skip the first entry which is a union of all monitors
                sct_img = sct.grab(monitor)
                image = Image.frombytes("RGB", sct_img.size, sct_img.bgra, "raw", "BGRX")
                screenshots.append(image)
        else:
            current_monitor = get_current_monitor(monitors)
            sct_img = sct.grab(current_monitor)
            image = Image.frombytes("RGB", sct_img.size, sct_img.bgra, "raw", "BGRX")
            screenshots.append(image)
        
        return screenshots

If I got you right, I should go ahead and change it to have a list return type, if that is the case, then its current implementation in models.py

 @classmethod
    def take_screenshot(cls: "Screenshot") -> "Screenshot":
        """Capture a screenshot."""
        image = utils.take_screenshot()
        screenshot = Screenshot(image=image)
        return screenshot

will change to this:

@classmethod
    def take_screenshot(cls: "Screenshot") -> list:
        """Capture a screenshot."""
        images = take_screenshot(all_monitors=all_monitors)
        screenshots = [cls(image=image) for image in images]
        return screenshots

And its usage will also change:

screenshots = Screenshot.take_screenshot(all_monitors=True)
for idx, screenshot in enumerate(screenshots):
    screenshot.image.show(title=f"Monitor {idx + 1}")

abrichr · 2024-06-23T13:45:57Z

I think we want to return a single PIL.Image containing all screenshots. Any way to determine relative positioning of the screens?

onyedikachi-david · 2024-06-24T08:13:32Z

I think we want to return a single PIL.Image containing all screenshots. Any way to determine relative positioning of the screens?

Yes, it is best to return PIL.Image. Let me think of a way to go about it.

onyedikachi-david · 2024-06-25T01:44:14Z

@abrichr I just implemented the requested changes.

onyedikachi-david · 2024-07-01T11:41:35Z

@abrichr I don't know if you've found time to review the PR.

abrichr

Thank you for putting this together @onyedikachi-david ! And thank you for your patience.

I left a few comments concerning performance. Because we call take_screenshot in a tight loop during recording, it's important that it is as performant as possible.

Can you please run python -m openadapt.record with CAPTURE_ALL_MONITORS = True and = False (while recording similar behavior, e.g. opening the calculator and clicking 2 x 3), and paste the performance plots here? (The path to the performance plot is logged to stdout at the end of the recording.)

abrichr · 2024-07-04T16:47:24Z

openadapt/utils.py

-    return image
+        PIL.Image.Image: The screenshot image.
+    """
+    with mss.mss() as sct:


Is it possible to re-use the global SCT here? Creating a new instance impacts performance; see https://python-mss.readthedocs.io/usage.html#intensive-use

abrichr · 2024-07-04T16:48:16Z

openadapt/utils.py

+            combined_image = Image.new("RGB", (total_width, total_height))
+
+            for monitor in monitors[1:]:  # Skip the first entry which is a union of all monitors
+                sct_img = sct.grab(monitor)


Is it possible to grab once and then recombine? Again the issue is performance.

openadapt/utils.py

onyedikachi-david · 2024-07-04T17:41:31Z

Thank you for putting this together @onyedikachi-david ! And thank you for your patience.

I left a few comments concerning performance. Because we call take_screenshot in a tight loop during recording, it's important that it is as performant as possible.

Can you please run python -m openadapt.record with CAPTURE_ALL_MONITORS = True and = False (while recording similar behavior, e.g. opening the calculator and clicking 2 x 3), and paste the performance plots here? (The path to the performance plot is logged to stdout at the end of the recording.)

You're welcome. Thanks for the feedback and all. Two issues though:

I don't have an external monitor to test.
This one isn't much of an issue though, I'll see how to run it on my Linux machine (although it didn't work last time I tried); my Mac OS crashed yesterday.

Just give me awhile to implement the requested changes.

…stly

onyedikachi-david · 2024-07-05T08:24:03Z

Please review @abrichr, just implemented the requested changes, no screenshot yet though. Yet to fix out my Mac OS crash

abrichr · 2024-07-05T19:58:45Z

openadapt/utils.py

+    if config.CAPTURE_ALL_MONITORS:
+        # Grab all monitors at once
+        sct_img = SCT.grab(SCT.monitors[0])  # Grab the union of all monitors
+        full_img = Image.frombytes("RGB", sct_img.size, sct_img.bgra, "raw", "BGRX")


@onyedikachi-david can you please clarify why the rest of this block is necessary? Why not just return full_img directly?

abrichr · 2024-07-05T20:01:13Z

openadapt/utils.py


+config = Config()


This should not be necessary. Please replace with from openadapt.config import config like it's used elsewhere.

onyedikachi-david · 2024-07-16T21:44:52Z

@abrichr I'm sorry for keeping a stale PR. I hope it isn't blocking anything. I'm still trying to get my macOS setup together. I was using a Hackintosh (dual-booted with Linux), but it got corrupted while I was reinstalling Docker. I'm still figuring out how to resolve this without making things worse. I don't want to lose my Linux files/partition, which is still functional.

abrichr · 2024-07-17T15:24:26Z

@onyedikachi-david thank you for the update. For now it is not blocking. 🙏

onyedikachi-david · 2024-07-17T15:54:14Z

@onyedikachi-david thank you for the update. For now it is not blocking. 🙏

Okay.

onyedikachi-david added 2 commits June 22, 2024 07:27

feat: Add get_current_monitor and enhance take_screenshot for multi-m…

316aef6

…onitor support

remove unnessary comments

3fff425

algora-pbc bot mentioned this pull request Jun 22, 2024

Support multiple monitors #766

Open

algora-pbc bot added the 🙋 Bounty claim label Jun 22, 2024

feat(config): Add support for capturing screenshots of all monitors.

56faa5d

abrichr requested changes Jul 4, 2024

View reviewed changes

onyedikachi-david and others added 2 commits July 4, 2024 22:26

Merge branch 'OpenAdaptAI:main' into feature/multi-monitor-screenshot

8468c7c

feat(screenshot): Enhance capture to handle multi-monitor setups robu…

aa5567a

…stly

abrichr reviewed Jul 5, 2024

View reviewed changes

Merge branch 'main' into feature/multi-monitor-screenshot

e412651

Merge branch 'main' into feature/multi-monitor-screenshot

a0bca65

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Enhance take_screenshot for multi-monitor support #792

feat: Enhance take_screenshot for multi-monitor support #792

onyedikachi-david commented Jun 22, 2024 •

edited

Loading

abrichr commented Jun 22, 2024 •

edited

Loading

onyedikachi-david commented Jun 22, 2024 •

edited

Loading

abrichr commented Jun 23, 2024

onyedikachi-david commented Jun 23, 2024

abrichr commented Jun 23, 2024

onyedikachi-david commented Jun 24, 2024

onyedikachi-david commented Jun 25, 2024

onyedikachi-david commented Jul 1, 2024

abrichr left a comment

abrichr Jul 4, 2024

abrichr Jul 4, 2024

onyedikachi-david commented Jul 4, 2024

onyedikachi-david commented Jul 5, 2024

abrichr Jul 5, 2024

abrichr Jul 5, 2024

onyedikachi-david commented Jul 16, 2024 •

edited

Loading

abrichr commented Jul 17, 2024

onyedikachi-david commented Jul 17, 2024

feat: Enhance take_screenshot for multi-monitor support #792

Are you sure you want to change the base?

feat: Enhance take_screenshot for multi-monitor support #792

Conversation

onyedikachi-david commented Jun 22, 2024 • edited Loading

abrichr commented Jun 22, 2024 • edited Loading

onyedikachi-david commented Jun 22, 2024 • edited Loading

abrichr commented Jun 23, 2024

onyedikachi-david commented Jun 23, 2024

abrichr commented Jun 23, 2024

onyedikachi-david commented Jun 24, 2024

onyedikachi-david commented Jun 25, 2024

onyedikachi-david commented Jul 1, 2024

abrichr left a comment

Choose a reason for hiding this comment

abrichr Jul 4, 2024

Choose a reason for hiding this comment

abrichr Jul 4, 2024

Choose a reason for hiding this comment

onyedikachi-david commented Jul 4, 2024

onyedikachi-david commented Jul 5, 2024

abrichr Jul 5, 2024

Choose a reason for hiding this comment

abrichr Jul 5, 2024

Choose a reason for hiding this comment

onyedikachi-david commented Jul 16, 2024 • edited Loading

abrichr commented Jul 17, 2024

onyedikachi-david commented Jul 17, 2024

onyedikachi-david commented Jun 22, 2024 •

edited

Loading

abrichr commented Jun 22, 2024 •

edited

Loading

onyedikachi-david commented Jun 22, 2024 •

edited

Loading

onyedikachi-david commented Jul 16, 2024 •

edited

Loading