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.

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] Linear to sRGB Gamma filter implementation. #35388

Merged
merged 18 commits into from Aug 18, 2022

Conversation

betrevisan
Copy link
Contributor

@betrevisan betrevisan commented Aug 12, 2022

The changes proposed in this PR implement the Linear Gamma to sRGB filter in Impeller alongside accompanying tests.

Below is a screenshot of the playground. The image on the left is the unfiltered image and the one on the right is the filtered one.

Screen Shot 2022-08-16 at 10 12 43 AM

Solves flutter/flutter#109738.

  • 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 Hixie said 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.

@betrevisan betrevisan changed the title [Impeller] sRGB to Linear Gamma filter implementation. [Impeller] Linear Gamma to sRGB filter implementation. Aug 16, 2022
@betrevisan betrevisan requested review from bdero and iskakaushik Aug 16, 2022
@betrevisan betrevisan marked this pull request as ready for review Aug 16, 2022
@bdero
Copy link
Member

bdero commented Aug 16, 2022

This needs to be rebased as RenderFilter has changed. It no longer takes a RenderPass and it now returns a Snapshot.

@bdero
Copy link
Member

bdero commented Aug 17, 2022

Just realized this is a separate PR to #35441. I don't think these need to be landed as totally distinct filters given the only difference between the two should be the fragment shader (and the fragment shader should require the exact same uniform/varying layout).

I recommend renaming this filter to ColorSpaceFilterContents, adding an enum option to specify the conversion operation (i.e. kLinearToSrgb and kSrgbToLinear), and do the command creation in a template with a Pipeline type parameter. See the AdvancedBlend function in blend_filter_contents.cc for an example of this.

bdero
bdero approved these changes Aug 17, 2022
Copy link
Member

@bdero bdero left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Since these work, we should definitely land them. These conversions can be combined later in follow-ups.

@betrevisan betrevisan added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 18, 2022
@auto-submit auto-submit bot merged commit da611f8 into flutter:main Aug 18, 2022
27 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 18, 2022
@betrevisan betrevisan changed the title [Impeller] Linear Gamma to sRGB filter implementation. [Impeller] Linear to sRGB Gamma filter implementation. Aug 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App impeller
Projects
Status: Done
3 participants