[Impeller] fixed colorspace for metal screenshots #49941
Conversation
|
After this patch Vulkan and Metal more closely match: metal
vulkan
|
gaaclarke
force-pushed
the
metal-golden-colorspace
branch
from
715f609 to
84849f6
Compare
flar
left a comment
flar
left a comment
There was a problem hiding this comment.
LGTM, though I'm not familiar enough with Metal to confirm the Ref management aspects.
|
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. |
|
I haven't heard anything from @mdebbar yet on the golden issues. I'll rebase to see if the next run works. |
gaaclarke
force-pushed
the
metal-golden-colorspace
branch
from
84849f6 to
542afef
Compare
|
Golden file changes are available for triage from new commit, Click here to view. |
|
I looked at some of the tests that I wrote that always looked a little faded in the goldens and they now look much more like I was expecting. |
|
|
||
| CIImage* ciImage = [[CIImage alloc] initWithMTLTexture:metal_texture | ||
| options:@{}]; | ||
| CGColorSpaceRef color_space = CGColorSpaceCreateDeviceRGB(); |
There was a problem hiding this comment.
Rather than create a device dependent color space which might vary over time as we change our CI hardware, should we perhaps create a standardized color space such as with one of these constants?
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm not sure if that would be appropriate. It may be more appropriate that the colorspace matches the device one since that's what the texture will reflect? I'd rather keep this similar to vulkan and as written. If it ever breaks, we'll know quickly.
There was a problem hiding this comment.
If the machines in the CI lab are homogenous then it might not cause problems now, but if we ever go through a transition with different types of machines that happen to disagree on their native colorspace we might have some headaches at that time.
gaaclarke
added
the
autosubmit
|
auto label is removed for flutter/engine/49941, due to This PR has not met approval requirements for merging. Changes were requested by {flar}, please make the needed changes and resubmit this PR.
|
auto-submit
bot
removed
the
autosubmit
flar
left a comment
flar
left a comment
There was a problem hiding this comment.
Approving with caveat described in the comments
…142023) flutter/engine@d3dbd42...df6b15d 2024-01-22 [email protected] [Impeller] dont emulate command buffers for compute on Metal/Vulkan. (flutter/engine#49922) 2024-01-22 [email protected] [Impeller] fixed colorspace for metal screenshots (flutter/engine#49941) 2024-01-22 [email protected] [Impeller] remove frame counter that was unused in Vulkan allocator. (flutter/engine#49913) 2024-01-22 [email protected] [Impeller] Create the golden test screenshotter after setting the Vulkan ICD environment variable (flutter/engine#49948) 2024-01-22 [email protected] [Windows] Rename and move EGL types (flutter/engine#49900) 2024-01-22 [email protected] Roll Skia from fa5f2461554f to 58973cca0edd (2 revisions) (flutter/engine#49947) 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 [email protected],[email protected],[email protected] 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
2 participants
fixes flutter/flutter#141903
The golden tests were generating images in the wrong colorspace which left things brighter than they should have been. This fixes that issue.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.