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] Support impeller::MipFilter::kBase for sampling from the base mip level. #147259

Closed
chinmaygarde opened this issue Apr 23, 2024 · 6 comments · Fixed by flutter/engine#52779
Assignees
Labels
e: impeller Impeller rendering backend issues and features requests P1 High-priority issues at the top of the work list team-engine Owned by Engine team triaged-engine Triaged by Engine team

Comments

@chinmaygarde
Copy link
Member

SkMipmapMode::kNone is denotes sampling from the base mip level. Impeller today can either sample from the nearest or use trilinear filtering. To support what Skia does, Impeller must add a new mip filter mode for sampling from the base.

In terms of implementation, Metal supports this via MTLSamplerMipFilterNotMipmapped and Vulkan supports this via VkSamplerCreateInfo::maxLod set to 0.

@chinmaygarde chinmaygarde added e: impeller Impeller rendering backend issues and features requests team-engine Owned by Engine team labels Apr 23, 2024
@chinmaygarde
Copy link
Member Author

Apparently, even Metal has [MTLSamplerDescriptor lodMaxClamp]. So the same technique can be used in both backends.

@matanlurey matanlurey self-assigned this Apr 23, 2024
@matanlurey matanlurey added the P1 High-priority issues at the top of the work list label Apr 23, 2024
@matanlurey
Copy link
Contributor

matanlurey commented Apr 23, 2024

I am going to take a shot at fixing/implementing this after a little debrief from @jonahwilliams.

Breadcrumbs:

For transparency, I'm OOO Th-Fr and most of Monday, but I'll try to get out an early PR and/or updates on this bug.

@matanlurey
Copy link
Contributor

I'm finally back after a few weeks of totally unconnected PTO and sick-time, and everything else is cleared off my plate. I plan to pick this up starting tomorrow morning.

@matanlurey
Copy link
Contributor

I spent most of the this afternoon restoring my ability to compile the engine after the RBE transition.

As a starting point, so I can iterate/observe how this works, I'm planning to add the following tests:

// testing/dart/image_filter_test.dart

group('ImageFilter|FilterQuality', () async {
  final ImageComparer comparer = await ImageComparer.create();

  Future<Image> makeImage(FilterQuality quality) async {
    final PictureRecorder recorder = PictureRecorder();
    final Canvas recorderCanvas = Canvas(recorder);
    recorderCanvas.drawRect(const Rect.fromLTRB(1.0, 1.0, 2.0, 2.0), Paint()
      ..color = green
      ..imageFilter = makeBlur(1.0, 1.0, TileMode.decal)
      ..filterQuality = quality
    );
    final Picture picture = recorder.endRecording();
    return picture.toImage(3, 3);
  }

  test('FilterQuality.low', () async {
    final Image image = await makeImage(FilterQuality.low);
    await comparer.addGoldenImage(image, 'FilterQuality.low');
  });

  test('FilterQuality.medium', () async {
    final Image image = await makeImage(FilterQuality.medium);
    await comparer.addGoldenImage(image, 'FilterQuality.medium');
  });

  test('FilterQuality.high', () async {
    final Image image = await makeImage(FilterQuality.high);
    await comparer.addGoldenImage(image, 'FilterQuality.high');
  });

  test('FilterQuality.none', () async {
    final Image image = await makeImage(FilterQuality.none);
    await comparer.addGoldenImage(image, 'FilterQuality.none');
  });
});

I might need to play around a bit with the generated picture & sizing to see the filter quality, but as a starting point.

auto-submit bot pushed a commit to flutter/engine that referenced this issue May 10, 2024
Work towards flutter/flutter#147259.

Most of this is just me understanding how `FilterQuality.*` feeds into the rest of the engine, and updating our testing documentation so we all understand how to re-run these tests in the future. Here is an example output of both Impeller and non-Impeller output of `image_filter_test`:

<img width="485" alt="Screenshot 2024-05-08 at 2 25 53�PM" src="https://github.com/flutter/engine/assets/168174/58e6a4f5-98e2-4b9e-b181-4ff613ad4d29">
@matanlurey
Copy link
Contributor

Initial golden-file test landed (^), I'll work on the actual Impeller implementation next.

auto-submit bot pushed a commit to flutter/engine that referenced this issue May 15, 2024
Closes flutter/flutter#147259.

Changes:
- Make sure the default of `kNearest` is used consistently
- Partial revert of #40491, re-adding `kBase`
- Added some documentation about relevant enums, and why the default is `kNearest`
- Wired up `kBase` in the Metal, Vulkan, and OpenGLES backends

Expecting an update to the `dart_ui_filter_quality_none` golden file:

> ![Screenshot 2024-05-13 at 2 47 49�PM](https://github.com/flutter/engine/assets/168174/68df4c1a-9b2b-4201-9a6c-f78361a5aa30)

For breadcrumbs around the mapping, see flutter/flutter#148253.
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
e: impeller Impeller rendering backend issues and features requests P1 High-priority issues at the top of the work list team-engine Owned by Engine team triaged-engine Triaged by Engine team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants