alex-ong / NESTrisOCR

OCR for statistics in NESTris
24 stars 7 forks source link

Capture once then extract areas with PIL #14

Closed timotheeg closed 4 years ago

timotheeg commented 4 years ago

Possible implementation of the "Capture Once Then Slice with PIL" approach.

Advantage of single capture is to not have race condition inconsistencies with multiple concurrent captures. The slicing and OCR can still be done in threads (although I haven't tested that on OSX)

Some notes:

Things seem to run at acceptable speed on OSX, but because multi-threading isn't working, I can't test that. The code should be ready for it though. Pls try it out in windows and let me know.

Any feedback welcome :)

Bonus: Only run scoreFixer when hex support is on (in separate commit)

alex-ong commented 4 years ago

The commit looks good; i like your use of func_tools partial in particular.

Lets have a discussion about a few things: 1) windows really sucks at capturing a large section of screenspace. This is why i've chosen the (not great) race condition method instead of capturing the largest section followed by sub-imaging. even running 6 mini screen captures in single thread is faster than capturing the large section. the larger the resolution the worse this performs. To be fair though most systems can probably run at 30fps on a single thread; the multi-threading optimization is mainly to hit that sweet 60fps.

2) i was thinking of instead moving single capture + crop into the capture method instead. note that main.py has capture_method.nextframe(). This would be the ideal place to capture the minimal area.

3) I have to do testing on multi-threading; i've recently added a replay feature (opens a file, and parses it). During my ventures in replay files, i noted that since python uses processes, the framecounters for every process would be different, and you'd open the video (num_threads) times. This is why i added nextframe() at the bottom. I haven't tested multithreading since then, maybe it actually works now.

4) Not sure how multiprocessing handles partial funcs. From memory it will just plain not work: https://stackoverflow.com/questions/45293989/how-to-use-the-partial-function-with-the-python-multiprocessing-module

5) Even if we had single big image -> small images, transferring the small images across process lines would be expensive due to pickling. Currently in multiprocessing land, each process calls windows capture api, and hence doesnt need transfer an image from python process to python process. In python 3.8, there are multiprocessing shared buffers. I can't wait to use those, except none of the libraries (pillow, numba, numpy etc) have wheels on 3.8 yet, so thats a future thing to think about.

The short answer is we still need to support both modes: single capture/frame with subregions and multiple captures with sadface race condition or remove multi-threading support entirely...

I definitely think moving all the rectangle calculation code to CaptureMethods folder as a lib.py or something could be good, and then put singlecapture as a setting somehow.

timotheeg commented 4 years ago

Thanks for the quick reply Alex! I figured, we'd still need to support 2 modes, but made this PR with a single approach first (capture-then-slice) only to make clearer the way it works to get your feedback :)

So Quick replies on your notes:

  1. windows really sucks at capturing a large section of screenspace. This is why i've chosen the (not great) race condition method instead of capturing the largest section followed by sub-imaging. even running 6 mini screen captures in single thread is faster than capturing the large section. the larger the resolution the worse this performs. To be fair though most systems can probably run at 30fps on a single thread; the multi-threading optimization is mainly to hit that sweet 60fps.

Noted. I imagine in full screen in OSX, it would be similarly slow. I was focusing on my setup right now, which isn't running full screen, and I can just about hit 60fps (some inconsistency in timing atm, but I get about 11ms to 15ms per loop)

  1. i was thinking of instead moving single capture + crop into the capture method instead. note that main.py has capture_method.nextframe(). This would be the ideal place to capture the minimal area.

I had not seen nextframe() (oops!), so I'll look into that.

  1. Not sure how multiprocessing handles partial funcs. From memory it will just plain not work: https://stackoverflow.com/questions/45293989/how-to-use-the-partial-function-with-the-python-multiprocessing-module

I didn't know about the limitations or partials with multiprocessing. I can easily get rid of partials and still be compatible with runTasks(). I'll add a commit asap just to at least make the branch runnable for testing.

  1. Even if we had single big image -> small images, transferring the small images across process lines would be expensive due to pickling.

Understood. I figured this might be a problem, but I didn't spend much time on it to get feedback first.

The short answer is we still need to support both modes: single capture/frame with subregions and multiple captures with sadface race condition or remove multi-threading support entirely...

Noted. With the feedback you already provided (thanks again!), I'll make a few changes when I can and get to a state where both methods are offered.

alex-ong commented 4 years ago

Sounds great! sorry for the news about func_tools. I personally love func_tools, it just sucks for multiprocessing.

timotheeg commented 4 years ago

I've dded a small commit to remove functools, so hopefully it's testable with multithreading under windows now. I'll look into enabling both capture methods properly as soon as I can.

alex-ong commented 4 years ago

Works! I'll write some further discussion in another thread.

alex-ong commented 4 years ago

Just referencing #13 since you completed this!

timotheeg commented 4 years ago

Awesome! Thanks Alex 😄

I saw your additional commit https://github.com/alex-ong/NESTrisOCR/commit/90c9d881d25132c32100aa9e312f77d691f4df8b

And I feel very dumb about it because I have an almost identical commit in my das trainer branch, but I completely forgot to push it to this branch 😅 https://github.com/timotheeg/NESTrisOCR/commit/36c32dafc69a5491ea40ce2e76b39989eebf7ec5

In any case, I'll go ahead and rebase my branch to your master, and resolve that minor conflict my side.

Cheers!