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

Conversation

@ColdPaleLight
Copy link
Member

@ColdPaleLight ColdPaleLight commented Aug 23, 2022

fix flutter/flutter#104718

Add a class SnapshotController for make snapshot, subclass SnapshotControllerSkia and SnapshotControllerImpeller to implement the function. In addition, the API of SnapshotDelegate:: MakeRasterSnapshot was modified, the parameter draw_callback was replaced by display_list

I guess Impeller's implementation doesn't need to generate a raster image, right?

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 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.

@ColdPaleLight ColdPaleLight marked this pull request as draft August 23, 2022 13:15
@ColdPaleLight ColdPaleLight self-assigned this Aug 23, 2022
@ColdPaleLight ColdPaleLight marked this pull request as ready for review August 30, 2022 12:32
@ColdPaleLight ColdPaleLight changed the title [Impeller] Implement Picture.toImage() [Impeller] Implement ui.Picture.toImage() Aug 30, 2022
@ColdPaleLight
Copy link
Member Author

Would like to get some comments, cc @dnfield @chinmaygarde @jonahwilliams

const Delegate& GetDelegate() { return delegate_; }

private:
const Delegate& delegate_;
Copy link
Contributor

Choose a reason for hiding this comment

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

FML_DISALLOW_COPY_AND_ASSIGN(SnapshotController)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


namespace flutter {

sk_sp<DlImage> SnapshotControllerImpeller::MakeRasterSnapshot(
Copy link
Contributor

Choose a reason for hiding this comment

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

This method seems like it should be guarded by the GPU sync switch...

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

Overall looks good, would like Chinmay or Jason to get a chance to have a look at it too.

@jonahwilliams
Copy link
Contributor

Can we autosubmit this, or are there additional reviewers that should take a look?

@ColdPaleLight
Copy link
Member Author

Can we autosubmit this, or are there additional reviewers that should take a look?

I think we can autosubmit it.

@ColdPaleLight ColdPaleLight added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 1, 2022
@auto-submit auto-submit bot merged commit 0738285 into flutter:main Sep 1, 2022
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

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

[Impeller] Implement Picture.toImage().

5 participants