-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[Impeller] In advanced blends, unpremultiply and apply src_input_alpha only if there is no foreground color #50260
Conversation
v_src_texture_coords // texture coordinates | ||
)); | ||
src *= blend_info.src_input_alpha; | ||
)) * blend_info.src_input_alpha; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When applying blend_info.src_input_alpha
to an unpremultiplied color, only the a
component should be adjusted, otherwise the color components will become darker as it becomes more transparent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -41,13 +41,12 @@ void main() { | |||
v_dst_texture_coords // texture coordinates | |||
)); | |||
dst *= blend_info.dst_input_alpha; | |||
f16vec4 src = IPHalfUnpremultiply( | |||
f16vec4 src = | |||
blend_info.color_factor > 0.0hf | |||
? blend_info.color |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change makes sense... this color never gets premultiplied on the CPU side before we get here. Could you add a comment saying this color is expected to be unpremultiplied in the shader struct where it's defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That comment already exists:
https://github.com/flutter/engine/blob/main/impeller/entity/shaders/blending/advanced_blend.frag#L18
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah whoops
0303177
to
6dd4fc2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
…a only if there is no foreground color See flutter/flutter#142620
6dd4fc2
to
1c44c07
Compare
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
…input_alpha only if there is no foreground color (flutter/engine#50260)
…142848) flutter/engine@fee0214...23763db 2024-02-03 chinmaygarde@google.com [Impeller] Fix blown stack due to always out of date window dimensions on high-dpi devices. (flutter/engine#50307) 2024-02-02 jason-simmons@users.noreply.github.com [Impeller] In advanced blends, unpremultiply and apply src_input_alpha only if there is no foreground color (flutter/engine#50260) 2024-02-02 30870216+gaaclarke@users.noreply.github.com [Impeller] new blur: clamp downsample scalar to 1/16 (flutter/engine#50262) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC matanl@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
See flutter/flutter#142620