Replaced Paint.setAlpha() with Paint.setColor() to reduce unnecessary allocations #1929
Conversation
| @@ -161,7 +162,12 @@ | |||
| return; | |||
| } | |||
| int alpha = (int) ((parentAlpha / 255f * ((IntegerKeyframeAnimation) opacityAnimation).getIntValue() / 100f) * 255); | |||
| paint.setAlpha(clamp(alpha, 0, 255)); | |||
| if (Build.VERSION.SDK_INT < Build.VERSION_CODES.R) { | |||
| int color = paint.getColor(); | |||
There was a problem hiding this comment.
getColor() could generate the same allocations as setAlpha() if the color is not RGB.
1 similar comment
|
@pablorengo I'm not seeing the same behavior as you. I tried your repro project (thank you for that, by the way) on API 30 and 31 and am not seeing any allocations other than the autoboxing Integer for the dynamic properties. This seems to be the |
|
@gpeal Sorry, I forgot to mention it here too. This problem happens on API level <= 29. It was fixed on Android 30 code. This is the problem on Android 29. The |
|
@pablorengo Lottie uses |
|
@gpeal Great. Let me check it and I'll come back with the modification. Thanks. |
|
@pablorengo Are you still interested in doing this? |
|
@gpeal Yes, I'll try to finish it today |
…amed[] allocations in Android 29 or lower.
pablorengo
force-pushed
the
fix_colorspace_allocations
branch
from
80d264a
to
d7d198d
Compare
| int alpha = (int) ((parentAlpha / 255f * opacityAnimation.getValue() / 100f) * 255); | ||
| paint.setAlpha(clamp(alpha, 0, 255)); | ||
| paint.setColor((clamp(alpha, 0, 255) << 24) | (color & 0xFFFFFF)); |
There was a problem hiding this comment.
@pablorengo You can revert this now right?
There was a problem hiding this comment.
@gpeal In this case, it avoids calling setColor() twice.
There was a problem hiding this comment.
Ah, I see. Got it. We could also make setColorAndAlpha on LPaint in case there are other usages of this in the future
| @@ -35,4 +38,13 @@ public LPaint(int flags, PorterDuff.Mode porterDuffMode) { | |||
| public void setTextLocales(@NonNull LocaleList locales) { | |||
| // Do nothing. | |||
| } | |||
|
|
|||
| @Override public void setAlpha(int alpha) { | |||
There was a problem hiding this comment.
Could you add a doc explaining why this is necessary so future people can understand?
There was a problem hiding this comment.
It's not a "memory issue". It's avoiding memory allocations.
| int alpha = (int) ((parentAlpha / 255f * opacityAnimation.getValue() / 100f) * 255); | ||
| paint.setAlpha(clamp(alpha, 0, 255)); | ||
| paint.setColor((clamp(alpha, 0, 255) << 24) | (color & 0xFFFFFF)); |
There was a problem hiding this comment.
Ah, I see. Got it. We could also make setColorAndAlpha on LPaint in case there are other usages of this in the future
pablorengo
force-pushed
the
fix_colorspace_allocations
branch
from
30144f5
to
1fb9f95
Compare
Resolves #1928
This pull request attempts to remove ColorSpace.Named[] allocations when calling Paint.setAlpha(). These changes will fix the issue only when using RGB colors.