Rockbox Technical Forums
Support and General Use => Theming and Appearance Customization => Topic started by: chris_s on February 06, 2019, 07:24:54 PM
-
So I've noticed that if you switch between (unique) themes where each contains an album cover tag, that, as soon as you load up a fourth theme, no album covers will be displayed for this and any further themes (except the ones you'd previously loaded) anymore, until you restart the device.
If a theme contains two album cover tags (i.e. in both the SBS and WPS), you can only switch to one other theme afterwards, before no more album covers will be loaded for other themes. If the second theme also contains two album cover tags, only one of them (either the one in the SBS or WPS) will be loaded.
Is this a known restriction or a bug? Seems like some resources aren't deallocated there or can't be reused?
-
Interesting - I've noticed that art sometimes fails to display if you switch themes, but hadn't picked up on that 'three time max' thing. The only pattern I noticed was the art would display in a new theme only if that theme had the same art display dimensions as the previous one.
-
Interesting - I've noticed that art sometimes fails to display if you switch themes, but hadn't picked up on that 'three time max' thing. The only pattern I noticed was the art would display in a new theme only if that theme had the same art display dimensions as the previous one.
ok, that part I wasn’t aware of. So you're saying, as long as the dimensions match up with the album tag of previously loaded themes, you can switch to other themes as often as you’d like and the cover will always be displayed? Will have to try this myself. Probably useful for whoever may have a go at trying to fix this eventually.
-
EDIT: See reply #5.
So, I think I've fixed it. Would be good if one of the Rockbox developers could have a look at the solution. The (one line) patch is attached but it's possible there's a better way to go about it. To explain:
Turns out line 1809 in skin_parser.c (https://github.com/Rockbox/rockbox/blob/1d893a05c6b3e73a8cda660d4a4cbc209c0fe0d9/apps/gui/skin_engine/skin_parser.c#L1809) never actually gets executed, because at that point, wps_data->playback_aa_slot has already been set to -1 in line 121 of skin_engine.c (https://github.com/Rockbox/rockbox/blob/1d893a05c6b3e73a8cda660d4a4cbc209c0fe0d9/apps/gui/skin_engine/skin_engine.c#L121). Thus playback_release_aa_slot(int slot) in playback.c (https://github.com/Rockbox/rockbox/blob/1d893a05c6b3e73a8cda660d4a4cbc209c0fe0d9/apps/playback.c#L3717) is never called, aa_slot->used is never decremented and so, eventually, when a new theme is loaded up and once the number of album art slots has reached MAX_MULTIPLE_AA, playback_claim_aaslot(struct dim *dim) (https://github.com/Rockbox/rockbox/blob/1d893a05c6b3e73a8cda660d4a4cbc209c0fe0d9/apps/playback.c#L3685) won't be able to provide any more slots when it is called with a different album art dimension than the ones previously seen.
Also, I haven't looked into it yet, but using the same logic, that probably also means that skin_backdrop_unload(wps_data->backdrop_id) is never called in line 1786 of skin_parser.c (https://github.com/Rockbox/rockbox/blob/1d893a05c6b3e73a8cda660d4a4cbc209c0fe0d9/apps/gui/skin_engine/skin_parser.c#L1796) which may or may not lead to (other) issues.
-
I'm just realizing that while my explanation is valid, the patch needs a little bit more thought, as slot 0 appears to be simply overwritten every time now due to the default initialization to 0 (instead of explicitly to -1 as before). So that's not really great either.
-
This patch works like it should.
-
I've not had a chance for more than a cursory glance at this patch but it is much easier to see it in context
http://gerrit.rockbox.org/r/#/c/2077/
-
I've not had a chance for more than a cursory glance at this patch but it is much easier to see it in context
http://gerrit.rockbox.org/r/#/c/2077/
thanks, that’s great – No rush.
To me at least, it seems logically sound like this. And so far so good: Everything has behaved exactly as intended on iPod 4G hardware and using the iPod video simulator.
-
I hope this patch proves OK and gets mainlined, as this little bug has been a minor irritant since I first installed rockbox! I'd test the patch if only I had a dev environment still set up to compile it.
-
I hope this patch proves OK and gets mainlined, as this little bug has been a minor irritant since I first installed rockbox! I'd test the patch if only I had a dev environment still set up to compile it.
Here’s the latest dev build for the iPod video with the patch applied, if you'd like to try it out right now – let me know if you need it for another target.
https://ufile.io/0qc0o
-
Thanks! If you have it for 6/7th gen would be good, but I have a 5th gen as well so can try this one.
-
Thanks! If you have it for 6/7th gen would be good, but I have a 5th gen as well so can try this one.
:) for the classic: https://ufile.io/lhup4
-
So far no problems. Can switch themes without issues.
Is it that it only stores three different sizes for the cover? And now it correctly wraps back to the start and overwrites the oldest one with the 4th one, where before it just stopped bothering after 3?
-
So far no problems. Can switch themes without issues.
Great to hear! That's also been my experience so far.
Is it that it only stores three different sizes for the cover? And now it correctly wraps back to the start and overwrites the oldest one with the 4th one, where before it just stopped bothering after 3?
Yes, that’s pretty much my understanding. The maximum number of album covers with differing dimensions that are buffered at the same time is defined to be equal to the number of skinnable screens (i.e. custom status bar / WPS / FM Screen). So, the system will use three album art “slots” to keep track of the different dimensions used.
When a skin is parsed and an album art tag is encountered, the parser will try to either re-use one of the three existing slots (if it finds one with equal dimensions), or claim a new one, which means finding a slot whose used counter is -1. In either case, the used counter is then increased by one.
So, before the fix, when the theme was applied after booting and had album art tags whose dimensions differed in both the WPS and SBS, this would result in an increase of the used counter for slots 0 and 1 each from -1 to 0. If you then loaded up a different theme, only one slot with a usage counter of -1 was left. Once that slot’s counter was also incremented, additional album art whose dimensions weren’t equal to one of the slots already in use would be ignored.
The bugfix makes sure that when a new theme is applied, each previous skin (WPS/SBS/FMS and remote skins) first gets a chance to decrease the usage counter of their album art slot (if used), so that if both WPS and SBS were using slot 0 for example (because they displayed album art with the same dimensions), the slot’s usage counter is decreased by 2 from 1 back to -1 and can thus be re-claimed for a different dimension.
-
I submitted your patch it'll be available in dev builds after today
Thanks for your contribution
-
I submitted your patch it'll be available in dev builds after today
Thanks for your contribution
Thank you!