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

Fix checking against frame_count in Screencopy.match #119

Merged
merged 1 commit into from
May 21, 2024

Conversation

hbatagelo
Copy link
Contributor

self.frame_count accessed in match() could contain a frame number that is out of sync with the grabbed screenshot. This fix makes sure that the frame count checked in match() is the actual frame count when grab_screenshot() is called.

Copy link
Contributor

@RAOF RAOF left a comment

Choose a reason for hiding this comment

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

Hm. I must be misunderstanding Python asynico, because I was under the impression that the async code is only run while in the await statement, so returning self.framecount from the async method should give you exactly the same result as when checking self.framecount immediately after the async method returns?

@AlanGriffiths
Copy link
Contributor

I was under the impression that the async code is only run while in the await statement

My understanding is the same - in Python it is more like co-routines, than threading.

@hbatagelo
Copy link
Contributor Author

I think this issue may be related to how the event loop schedules the callbacks. What I've observed is that ScreencopyTracker._frame_ready may be called just after grab_screenshot returns. If that happens, _frame_ready increases frame_count, but the returned image still refers to the previous frame.

When I log the values of frame_count and last_checked_frame_count in the while loop of Screencopy.match, sometimes I encounter the following scenario that motivates this PR:

# 1st iteration
screenshot = await asyncio.wait_for(self.grab_screenshot(), timeout) # In grab_screenshot, self.frame_count is some number, e.g., 10
# screenshot refers to the frame with self.frame_count==10
# But self_frame_count is now 11 because _frame_ready was scheduled to run after grab_screenshot was being waited for
if last_checked_frame_count != self.frame_count: # True because this is the first iteration and last_checked_frame_count==0
    last_checked_frame_count = self.frame_count # OK, last_checked_frame_count is now 11
    # find_template_in_image is called. Let's assume the template doesn't match with this screenshot
    
# 2nd iteration
screenshot = await asyncio.wait_for(self.grab_screenshot(), timeout) # In grab_screenshot, self.frame_count is 11
# screenshot refers to the frame with self.frame_count==11
# This time, self_frame_count is still 11 because there were no new frames
if last_checked_frame_count != self.frame_count: # False
    last_checked_frame_count = self.frame_count # Won't be reached, but screenshot may contain the matching template
    # ...
    
# If no additional frames are created, the loop will time out, even if screenshot contains the matching template

@hbatagelo hbatagelo requested a review from RAOF May 9, 2024 16:27
Copy link
Contributor

@RAOF RAOF left a comment

Choose a reason for hiding this comment

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

Hm. I'm insufficiently pythonic to see precisely what's happening here, but returning (frame_count, screenshot) is definitely not incorrect, so let's do that.

@RAOF RAOF added this pull request to the merge queue May 21, 2024
Merged via the queue into main with commit 962fafc May 21, 2024
9 of 15 checks passed
@RAOF RAOF deleted the fix-screencopy-use-of-frame-count branch May 21, 2024 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants