Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] implemented golden image tests for opengles #50146

Merged
merged 13 commits into from
Jan 31, 2024

Conversation

gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Jan 29, 2024

fixes flutter/flutter#142354

This sets up impeller_golden_tests to run with ANGLE.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@gaaclarke gaaclarke force-pushed the impeller_opengl_goldens branch from e4f825b to 639691e Compare January 30, 2024 20:25
@gaaclarke
Copy link
Member Author

This fails now currently when shaders are attempted to be compiled because they are being compiled for OpenGL, not opengles. They have #version 120 in them, not #version 100.

@gaaclarke
Copy link
Member Author

This fails now currently when shaders are attempted to be compiled because they are being compiled for OpenGL, not opengles. They have #version 120 in them, not #version 100.

Fixed with impellerc flags.

@gaaclarke gaaclarke marked this pull request as ready for review January 30, 2024 22:09
@flutter-dashboard

This comment was marked as resolved.

@flutter-dashboard

This comment was marked as outdated.

@gaaclarke
Copy link
Member Author

@jonahwilliams I added the flip in on the CPU side that respects Texture::GetYCoordScale. Thanks for pointing that out. I briefly looked into doing the flip on the GPU but it was more involved. We can do that later.

@flutter-dashboard

This comment was marked as outdated.

@jonahwilliams
Copy link
Member

Thanks! it looks like there is a remaining color format issue BGRA vs RGBA?

@gaaclarke
Copy link
Member Author

Thanks! it looks like there is a remaining color format issue BGRA vs RGBA?

Yep, looks like it. Is there something in Texture I can used to decide that? Maybe the TextureDescriptor?

@gaaclarke
Copy link
Member Author

The conventional wisdom has been that BGRA is faster on some hardware. We should look to make sure we are taking advantage of that in our renderers.

@gaaclarke
Copy link
Member Author

Thanks! it looks like there is a remaining color format issue BGRA vs RGBA?

Yep, looks like it. Is there something in Texture I can used to decide that? Maybe the TextureDescriptor?

Yep, the texture descriptor format has the info we want.

The conventional wisdom has been that BGRA is faster on some hardware. We should look to make sure we are taking advantage of that in our renderers.

OpenGLES is the one using RGBA, we should fix that.

@gaaclarke
Copy link
Member Author

Thanks! it looks like there is a remaining color format issue BGRA vs RGBA?

Done.

@gaaclarke
Copy link
Member Author

The conventional wisdom has been that BGRA is faster on some hardware. We should look to make sure we are taking advantage of that in our renderers.

I've looked into this a bit. For Android RGBA may be better. It's not clear there is a need to change this considering who is using OpenGLES.

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #50146 at sha 327502b

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

It looks like there are a number of goldens on page 3-4 that aren't showing up at all, you should file a tracking issue to investigate if that is specific to the golden harness.

@gaaclarke
Copy link
Member Author

Here are the ones that rendered nothing.

impeller_Play_AiksTest_CanRenderClippedLayers_OpenGLES
impeller_Play_AiksTest_ClippedBlurFilterRendersCorrectly_OpenGLES
impeller_Play_AiksTest_GaussianBlurOneDimension_OpenGLES
impeller_Play_AiksTest_CanRenderBackdropBlurHugeSigma_OpenGLES
impeller_Play_AiksTest_CanDrawPaintMultipleTimes_OpenGLES
impeller_Play_AiksTest_CanDrawPaint_OpenGLES
impeller_Play_AiksTest_CanRenderColorFilterWithInvertColorsDrawPaint_OpenGLES
impeller_Play_AiksTest_GaussianBlurRotatedAndClipped_OpenGLES
impeller_Play_AiksTest_ImageFilteredUnboundedSaveLayerWithUnboundedContents_OpenGLES
impeller_Play_AiksTest_CanDrawPaintWithAdvancedBlend_OpenGLES
impeller_Play_AiksTest_SubpassWithClearColorOptimization_OpenGLES
impeller_Play_AiksTest_CanRenderClippedBlur_OpenGLES
impeller_Play_AiksTest_DrawScaledTextWithPerspectiveNoSaveLayer_OpenGLES
impeller_Play_AiksTest_CanRenderLinearGradientMaskBlur_OpenGLES
impeller_Play_AiksTest_GaussianBlurScaledAndClipped_OpenGLES
impeller_Play_AiksTest_CollapsedDrawPaintInSubpass_OpenGLES

I suspect the blur ones are probably covered under flutter/flutter#142355. I'll double check these against Metal since drawing nothing may be the correct behavior in some cases.

Note: I'm going to land the update of the TODO(tbd) in a separate PR because of the overhead of running CI.

@jonahwilliams
Copy link
Member

SGTM

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 31, 2024
@auto-submit auto-submit bot merged commit ba3218b into flutter:main Jan 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 31, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jan 31, 2024
…142656)

flutter/engine@5b89189...c83617e

2024-01-31 [email protected] Roll Dart SDK from 1f136c7b962d to 82936dcdaf4f (1 revision) (flutter/engine#50212)
2024-01-31 [email protected] [Impeller] implemented golden image tests for opengles (flutter/engine#50146)

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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller will affect goldens
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[Impeller] implement golden tests for opengles
2 participants