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

Not really an issue, just some tweaks, from a new-ish scripter #4

Open
BlakeTech opened this issue Apr 18, 2021 · 9 comments
Open

Not really an issue, just some tweaks, from a new-ish scripter #4

BlakeTech opened this issue Apr 18, 2021 · 9 comments

Comments

@BlakeTech
Copy link

BlakeTech commented Apr 18, 2021

Forked your repo, and had a look, also had a look about the suggestions made by the other user, but honestly, cv2 and the way you're currently doing the ascii conversion, is over my head slightly. If I properly sat down and read it, I might get somewhere, but anyways.
I've tweaked your script to also use threading on the frames, so it should be faster. Also, completely removed the need to save to a file, twice, by just directly passing the cv2 result to PIL, and then outputting the frames, though I do like the idea for keeping the frames, which is why I'm also looking into perhaps RLE compressing the frames into a single file, instead of having a whole folder of 7kb ASCIIs.
I'm not really sure if you wanted the tweaks I made in your version, so I made it as a separate file on my repo and put this as a issue, instead of a PR (though if comments were a thing, I'd put it there), if you just want to take a look. Just as someone who vaguely knows Python, this was a nice project to at least be able to work on.

Oh, and progress bar still works!

@CalvinLoke
Copy link
Owner

Thanks for taking a look at my code! Wasn't expecting people to even care much, so it was definitely a surprise.

Your implementation looks robust. I am still quite fuzzy regarding threading, however, I guess I could learn a thing or two from you.

Yeah definitely the main issue would most likely be attributed to file I/O, as what
karoush1
said. Compressing them into a single .txt file might fix the issue. I am currently trying to that right now.

As for forking the repo, no worries you could create a branch and I guess have it as an alternative to my original.

Once again, really appreciate your time and effort looking at my code.

@BlakeTech
Copy link
Author

BlakeTech commented Apr 21, 2021

Side question, how long does it take for you to run v2? Tested it, took about 17+ hours, and 5% cpu.

Working on compression, nearly done, just need to get decoding. Seems to net nearly 50% saving.

@CalvinLoke
Copy link
Owner

Wait 17 hours just for frame extraction and ASCII generation? Wow that is a first, most of my tests are within 2 minutes top.

May I know what environment you are running v2 on?

@BlakeTech
Copy link
Author

Thought as much. Windows though IDLE does not seem to like running the bar either.
Doesn't matter, I don't usually run my scripts on windows anyhow. Chalking it up to IDLE bug.

@CalvinLoke
Copy link
Owner

Hmm, yeah still very fuzzy on concurrency and parallelism in general, so I thought I'd give it a shot to get some bearing first.

For the most part, asset generation is indeed faster, though as you suggested, boils down more to the execution. Your code skipped the frame extraction entirely, which halves the process already. Perhaps I could implement something similar, though I usually avoid directly copying others.

@BlakeTech
Copy link
Author

I removed my comment cause it felt slightly wrong to compare timings between the two scripts since one wrote to storage vs the other to memory.

My script doesn't skip frame extraction, it skips saving the frames extracted, instead passing it directly to the asciification, which I have now added back in to one compressed file. I am interested in benchmarking how well the implementation actually will do if I also had to write to disk, so I'll test that later. But that does indeed help save time, which I decided to do since it was the resulting frames that we need anyways, so I ignored the individual frames.

Otherwise, copying is fine, there are many ways to do something, and eventually the ideas will get reused. Also, found it mildly amusing that you imported queue, without actually using it.

I will say, the midi idea is actually a good one, makes for an overall smaller package, though rather than having the user opt for the file, rather, just check for both, if one doesn't exist, check the other, if both don't, then send an error.

Also, it's not a problem, but there's also the idea of "truthiness" that you can use to reduce code size.

Finally, found a funny little module called fpstimer that seems to help with the frame output timing, though, on my Windows machine, it's slower than on my Linux one, possibly due to how Python itself is being compiled for different OSes, based on some light searching online. Still, it's stable enough to have the sound and frames mostly match up.

@BlakeTech
Copy link
Author

Just a heads up, since we happen to both be doing "any video" support, keep in mind that videos have different fps. Might want to handle that too... ;)

@BlakeTech
Copy link
Author

Also, potential issue for v4. I could be wrong, since I didn't actually modify the script, but if you plan to expand v4 to suport ascii-fying any video, you'll need to purge asciilist in main, otherwise, it will just check and find that the list is full, but it's of the first video you played in the session, even if you selected a different video.

@CalvinLoke
Copy link
Owner

Hi there, thanks for bringing up the fpstimer library, it really helped a ton!

Yeah, I noticed that you still need to "extract" the frames, just that you didn't write them to local storage (which I assume incurs quite a bit of file IOPS). I took a similar approach and adapted yours to store the frame temporarily in memory, which did speed up ASCII generation by quite a bit.

As for v4.5, yeah I will look into it, will most likely need to adjust the frame rate in accordance to the source video, thanks for pointing that out.

I guess you are right, I do need to purge the ASCII list, or at least add some logic to keeping the ASCII characters in memory for v4.5.

Really appreciate the time and effort invested, I learnt quite a great deal along the way :)

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

No branches or pull requests

2 participants