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

[Impeller] Linear to sRGB Gamma filter implementation. #35388

Merged
auto-submit merged 18 commits into from
Aug 18, 2022
Merged

[Impeller] Linear to sRGB Gamma filter implementation. #35388

auto-submit merged 18 commits into from
Aug 18, 2022

Conversation

@betrevisan
Copy link
Contributor

@betrevisan betrevisan commented Aug 12, 2022 •
edited
Loading

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 marked this pull request as ready for review August 16, 2022 17:53
@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
View reviewed changes
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
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 18, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 18, 2022
@engine-flutter-autoroll
da611f8 [Impeller] Linear Gamma to sRGB filter implementation. (flutt…
110e2b5 
…er/engine#35388)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 18, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 18, 2022
@engine-flutter-autoroll
da611f8 [Impeller] Linear Gamma to sRGB filter implementation. (flutt…
6690a2b 
…er/engine#35388)
@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 engine-flutter-autoroll mentioned this pull request Aug 18, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 18, 2022
@engine-flutter-autoroll
da611f8 [Impeller] Linear Gamma to sRGB filter implementation. (flutt…
7299937 
…er/engine#35388)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@iskakaushik iskakaushik Awaiting requested review from iskakaushik

1 more reviewer

@bdero bdero bdero approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@betrevisan betrevisan

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller

Projects

No open projects
Impeller
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@betrevisan @bdero @chinmaygarde