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.

Sign up for GitHub

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

Jump to bottom

Fix for the bad palette performance #3978 #3979

Closed
wants to merge 1 commit into from
Closed

Fix for the bad palette performance #3978 #3979

wants to merge 1 commit into from

Conversation

jw2013
Copy link

@jw2013 jw2013 commented May 15, 2024

Fix for the bad performance #3978 caused by Segment::color_from_palette creating loading/creating palettes on each call.

@jw2013
Fix for the bad performance Aircoookie#3978 caused by Segment::color_…
4b2d62d 
…from_palette creating loading/creating palettes on each call.
@robertlipe robertlipe mentioned this pull request May 15, 2024
Closed
1 task
blazoncek
blazoncek requested changes May 15, 2024
View reviewed changes
Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I have to disapprove this PR as it negates effect and palette blending.

wled00/FX.h
@@ -91,6 +91,7 @@
//#define SEGLEN strip._segments[strip.getCurrSegmentId()].virtualLength()
#define SEGCOLOR(x) strip.segColor(x) /* saves us a few kbytes of code */
#define SEGPALETTE strip._currentPalette
#define SEGPALETTEIDX strip._currentPaletteIndex
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong.
Current palette is not a strip property, it is an effect property and needs to be stored in Segment object.
See explanation below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to use SEGPALETTE, which resolves to strip._currentPalette, as the palette cache.
Alternative would be to declare a new global variable for this cache, as I did before in the example.
(i thought that SEGPALETTE was still referred to, but not in active use, so wanted to 'reuse' it efficiently)

wled00/FX.h
@@ -571,7 +572,7 @@ typedef struct Segment {
uint8_t currentMode(void); // currently active effect/mode (while in transition)
uint32_t currentColor(uint8_t slot); // currently active segment color (blended while in transition)
CRGBPalette16 &loadPalette(CRGBPalette16 &tgt, uint8_t pal);
CRGBPalette16 &currentPalette(CRGBPalette16 &tgt, uint8_t paletteID);
inline CRGBPalette16 &currentPalette(uint8_t paletteID);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inlining will increase code size which is unacceptable as we are already at 98.5% flash size.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Segment::currentPalette is only ever called in WS2812FX::service and Segment::color_from_palette, so it might even reduce flash size (no function call overhead).

wled00/FX_fcn.cpp
loadPalette(targetPalette, pal);
unsigned prog = progress();
inline CRGBPalette16 &Segment::currentPalette(uint8_t pal) {
if ( SEGPALETTEIDX != pal ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work during mode (aka effect) blending. Each mode (effect) can have its own palette that needs to be blended during execution of mode (effect).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you think that if will not work for blending? It does not interfere with the effect and segment palettes, but just gets used as a cache?!?

wled00/FX_fcn.cpp
unsigned noOfBlends = ((255U * prog) / 0xFFFFU) - _t->_prevPaletteBlends;
for (unsigned i=0; i<noOfBlends; i++, _t->_prevPaletteBlends++) nblendPaletteTowardPalette(_t->_palT, targetPalette, 48);
targetPalette = _t->_palT; // copy transitioning/temporary palette
uint16_t noOfBlends = ((255U * prog) / 0xFFFFU) - _t->_prevPaletteBlends;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uint16_t will unnecessarily increase code size and slow execution down as it adds & 0xFFFF to every operation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, we can keep both vars as 'unsigned'

@jw2013
Copy link
Author

jw2013 commented May 15, 2024

@blazoncek , the change does not negate effect and palette blending, but just acts as a cache for loadPalette.

@softhack007
Copy link
Collaborator

softhack007 commented May 15, 2024 •
edited

Unfortunately I have to disapprove this PR as it negates effect and palette blending.

the change does not negate effect and palette blending, but just acts as a cache for loadPalette.

Hi @blazoncek and @jw2013, maybe before jumping directly into a possible solution, we could do some good old "design engineering"?

From what I understood,

  • the problem can be described as "re-loading fixed palettes on each Segment::color_from_palette is inefficient, especially if loading from flash"
  • Segment::color_from_palette is used very frequently in some effects
  • the proposal is to add a caching mechanism, to prevent repeated loading of exactly the same palettes
  • WLED has some features that lead to very frequently changing palettes: effect blending, palette blending, dynamic palettes (eg audioreactive palettes)
  • personally, I thing that frequently re-loading dynamic palettes in the middle of drawing an effect is a bit of overkill.

Assuming that some caching is possible there are design decisions to be made

  • where to place the cache memory (global, strip, segment) ?
  • when to update the cache (on-the fly in color_from_palette, upfront once per frame in strip.service, or inside loadPalette() ) ?
  • what are scenarios where caching must be skipped (like segment in transition)?
  • how to identify "exactly the same palette" especially when palettes are sometimes dynamic
  • are there side-effects on other segment functions to be considered - copy&move constructors, transition object, serialize/deSerialize, etc
  • alternative solutions ?

✏️ Opinions and comments, please!

@softhack007 softhack007 linked an issue May 15, 2024 that may be closed by this pull request
Palettes: Bad performance on 0.14.* and later #3978
Closed
1 task
@blazoncek
Copy link
Collaborator

Obsolete by 77e6ea8

@blazoncek blazoncek closed this May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blazoncek blazoncek blazoncek requested changes

Assignees
No one assigned
Labels
discussion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Palettes: Bad performance on 0.14.* and later
3 participants
@jw2013 @softhack007 @blazoncek