Closed VedanshGoenka closed 3 years ago
I spent about 4 hours this afternoon working on this. Still squashing a few bugs before I merge the code. It works but isn't necessarily amazing. Definitely needs some optimizations. If you're on a 4k monitor, memory usage can go from 200MB with no song playing to 1GB when a song with cdArt begins playing. That's... not ideal 😄
Anyway, it's being worked on, and I'll need some people to test it before it makes an official release so I'm just going to assume you're volunteering haha
I just wanted to thank you for adding this feature. I asked you about it a few months ago, but at that time you didn't think it was feasible. I'm glad you worked it out. It works great and is fairly smooth with just a little jerkiness using an Intel i5 cpu . The rotation speed seems just about right. It does add about 10 - 20 percent cpu and 800 MB of memory, but it's worth it.
I've been playing around with this and found that reducing maxRotatedCdImages in georgia-main.js can drastically reduce memory usage. Changing it from the current 72 to 50 reduces memory usage from 800 MB to 600 MB. Changing it to 25 reduces memory usage to 300 - 350 MB. It should be noted that reducing maxRotatedCdImages seems to also speed up the rotation, which may or may not be desirable. Please note that I DO NOT recommend changing this setting unless kbuffington gives his blessing as there may be undesirable side effects I have not yet identified. I should also note that, in addition to increasing cpu and memory usage, power usage is also significantly increased when using this feature. As always, thank you to kbuffington for giving us this wonderful theme.
No real undesirable side effects for futzing with those numbers. I'd originally used 36, but found it too jerky. 72 obviously doubles memory usage, and is still a little jerky but I thought the effect was decent. I'm thinking for the rotated images, since they're spinning I can maybe store slightly lower-res versions, but again I haven't played with it too much. For now I'm kinda just looking for feedback and to see what people like/don't like.
If you think enough people will be using this feature, you might want to consider adding a configuration option to change maxRotatedCdImages since it effects both rotation speed and memory usage. I've already added it, but others might find it useful, particularly if memory is an issue.
Okay, so I did a bunch of tests to see what could be done here. First issue is that resizing an image at draw time is SLOW. I knew this, but had forgotten how slow it was. To draw an 800x800 image at 800x800 even if it's a partially transparent png takes about 1-2ms. Drawing a 600x600 image at 800x800 is about 3x slower. That means that we must store the full size images when rotating and can't scale up to save memory.
I then did a bunch of tests on a custom build of FSM to see if I could speed up rotation and only save the first 90degrees and then use some optimizations to draw them rotated very fast at multiples of 90 or 180. This worked, but the rotation was terribly. It looked like the cd was jumping around a few pixels as it spun. Terrible results so I reverted the FSM code.
So then only thing we can do is continue to cache all images at full size. I added in options to change the # of images saved and the rotation speed of the cd so Georgia should be able to handle low memory, slow CPU setups now and still give you that sweet spinny goodness.
First of all, I'd like to thank @kbuffington, again, for this amazing skin and all the work that you have put into it. I also appreciate the insane documentation that you recently added and your willingness to make changes based on suggestions from the community.
I had another suggestion about a feature that could be added.
Currently, when CD art is present, the only way the CD changes is either if "cycle artwork" is enabled, or a the track moves on, which causes a rotation of a couple degrees.
In concept what I am proposing is quite simple, adding a option for the CD to rotate continuously during playback, then pause when playback is paused.
I've tried myself to "hack this" into place myself, but usually there are certain bugs that I am not knowledgeable enough to squash.
I would appreciate it if you could add this, although I understand if this is lower on your priority list.
Thanks again! -Vedansh