alex-ong / NESTrisOCR

OCR for statistics in NESTris
24 stars 7 forks source link

Minor optimizations #5

Closed timotheeg closed 5 years ago

timotheeg commented 5 years ago

Hey there Alex,

I changed my mind on starting with big change in configs. Instead I thought I'd start by opening this PR, for stuff I know for sure are optimizations (at least in OSX), and I'll work on introducing further optimisations, and refactoring the config system later.

So, 2 things in this PR, in 2 neatly independent commits:

commit 1: Remove the need to iterate over all windows when fetching the window info at each frame. There are now 2 nested loops: one to find the window, one to refresh its data. I haven't searched for the win API equivalent yet, so on windows, the behaviour is (should be!) unchanged. In OSX, the quartz API really are quite slow, and so commit 1 alone is enough to remove some Warning, not scanning field fast enough I could see.

commit 2: Introduce the non-numpy optimization I had made in my branch for scoreDigit() (renamed here getDigit()). The improved version is 2x as fast as the original (achieved by dropping repeat reads of the image pixels, and dropping sorting). Sadly, this is just for academic purpose you might say, because the numpy version is still over 4x as fast as the improved version!! 😮. I don't expect anyone would not use the numpy version at this stage.

That said, assuming you do keep the NP_SUPPORTED:False code paths, the second commit also corrects an issue with the img.getdata() vs img.load(). getdata() was incorrectly applied for both paths, and that broke the tuple access into the image data.

This is tested (a bit) on OSX, no testing done in windows. That said, my wife just got herself a windows box, so I'll hopefully have a win environment to test my stuff in the future!

Looking forward to your comments and feedback! :)

timotheeg commented 5 years ago

Alrighty! I tested on windows and things still to still be working fine!

I also force-pushed a small change to modify the first commit. I implemented a very simple getWindow(hwnd) for Win32WindowMgr too. It means that windows too benefits from not iterating over all windows at each frame.

Under windows, what the inner loop does is verify that hwnd is still a window, if it is, just use again it as-is. If it is not, break and go to the outer loop till the window of the desired name shows up again. The method names are a bit "funny", but I suppose we can correct that later.

There is a concern that reusing hwnd might give you a different window if the original window is closed and the window ID is reused. That said, I think it's fine for NESTrisOCR to use because it's mitigated by the use case:

The last comment in that stackoverflow post is interesting btw. It suggested tracking the pid associated with the window ID for greater safety. That said, I think for NEStrisOCR, that's be unnecessary.

Cheers!

alex-ong commented 5 years ago

I'm happy to merge as is and make the little modifications myself (unless you want to do it to claim those delicious contribution stats); let me know!

timotheeg commented 5 years ago

K, should be all corrected now, and I've also made a quick fix to the Readme :)

alex-ong commented 5 years ago

Merged.