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.

Sign up for GitHub

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

Jump to bottom

[Impeller] Rasterizer::ScreenshotLayerTreeAsImage uses Skia rendering when running with Impeller. #135052

Closed
jonahwilliams opened this issue Sep 19, 2023 · 8 comments · Fixed by flutter/engine#46171 or flutter/engine#47356
Closed

[Impeller] Rasterizer::ScreenshotLayerTreeAsImage uses Skia rendering when running with Impeller. #135052

jonahwilliams opened this issue Sep 19, 2023 · 8 comments · Fixed by flutter/engine#46171 or flutter/engine#47356
Labels
e: impeller Impeller rendering backend issues and features requests engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list team-engine Owned by Engine team triaged-engine Triaged by Engine team

Comments

@jonahwilliams
Copy link
Member

jonahwilliams commented Sep 19, 2023 •
edited

Only part of the screen seems to be captured on Wonderous. On iOS 17 the native screenshot tool is broken so we're falling back to rasterizer screenshots. These aren't setting up an aiks context, so its rendering skia but no images.

Expected:

flutter_01

Actual:

flutter_02
@jonahwilliams jonahwilliams added engine flutter/engine repository. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Sep 19, 2023
@huycozy huycozy added the team-engine Owned by Engine team label Sep 20, 2023
@chinmaygarde chinmaygarde changed the title Screenshots broken on iOS 17 when targeting Impeller [Impeller] Screenshots broken on iOS 17. Sep 20, 2023
@jason-simmons
Copy link
Member

How are you obtaining the screenshots?

I noticed that if I use flutter screenshot --type=rasterizer, then any images drawn in the scene will not be captured in the screenshot.

Rasterizer::ScreenshotLayerTreeAsImage is not setting up an AiksContext. So CompositorContext::ScopedFrame::Raster is actually going through the Skia painting code path instead of using Impeller.

@jonahwilliams
Copy link
Member Author

This is using the s button in the flutter tool, I'm not actually sure what that is using but I thought it was using some cmdline flutter utility.

@jonahwilliams
Copy link
Member Author

Oh, maybe on iOS17 the cmd line tool for screenshots isn't working so we're falling back to the rasterizer screenshot? @christopherfujino does this sound right?

@christopherfujino
Copy link
Member

Hmm, I'm actually surprised we got a screenshot at all, I thought this was completely broken per: #128598. Was this with a physical phone?

cc @vashworth

@jonahwilliams
Copy link
Member Author

Yes!

@jonahwilliams
Copy link
Member Author

Ahh:

/// If the device type does not support a "native" screenshot, then this
/// will fallback to a rasterizer screenshot from the engine. This has the
/// downside of being unable to display the contents of platform views.

resident_runner L974

@jonahwilliams
Copy link
Member Author

So we either need to fix Rasterizer::ScreenshotLayerTreeAsImage to use impeller correctly, or disable this functionality when using impeller entirely. Probably no action needed on tools side.

@jonahwilliams jonahwilliams changed the title [Impeller] Screenshots broken on iOS 17. [Impeller] Rasterizer::ScreenshotLayerTreeAsImage uses Skia rendering when running with Impeller. Sep 21, 2023
@jonahwilliams jonahwilliams mentioned this issue Sep 21, 2023
Merged
@chinmaygarde chinmaygarde added P2 Important issues not at the top of the work list triaged-engine Triaged by Engine team labels Sep 25, 2023
@jonahwilliams jonahwilliams mentioned this issue Sep 25, 2023
Merged
auto-submit bot pushed a commit that referenced this issue Sep 29, 2023
@jonahwilliams
[flutter_tools] remove VmService screenshot for native devices. (#135462
d13cd88 
)

* This is completely broken on the Impeller renderer, see: #135052
* Even on the Skia renderer, this gives a software rasterized screenshot which will absolutely look different from a native rendering screenshot.

I plan to remove this functionality from the engine.
Mairramer pushed a commit to Mairramer/flutter that referenced this issue Oct 10, 2023
@jonahwilliams @Mairramer
[flutter_tools] remove VmService screenshot for native devices. (flut…
2b55dc3 
…ter#135462)

* This is completely broken on the Impeller renderer, see: flutter#135052
* Even on the Skia renderer, this gives a software rasterized screenshot which will absolutely look different from a native rendering screenshot.

I plan to remove this functionality from the engine.
auto-submit bot pushed a commit to flutter/engine that referenced this issue Oct 26, 2023
@jonahwilliams
[Impeller] Disable layer tree screenshots. (#46171)
9927539 
The "rasterizer screenshot" VM service API uses the Skia software backend, even when running with impeller. At best, this leads to confusing half working screenshots that are missing images.

See flutter/flutter#135052

We should disable this functionality when running with impeller. Perhaps this could be reworked to use picture toImage instead of the offscreen so that it uses the "real" backend regardless of the platform it is running on.

Fixes flutter/flutter#135052
@auto-submit auto-submit bot mentioned this issue Oct 26, 2023
Merged
auto-submit bot added a commit to flutter/engine that referenced this issue Oct 26, 2023
@auto-submit
Reverts "[Impeller] Disable layer tree screenshots." (#47356)
9788bb9 
Reverts #46171
Initiated by: jonahwilliams
This change reverts the following previous change:
Original Description:
The "rasterizer screenshot" VM service API uses the Skia software backend, even when running with impeller. At best, this leads to confusing half working screenshots that are missing images.

See flutter/flutter#135052

We should disable this functionality when running with impeller. Perhaps this could be reworked to use picture toImage instead of the offscreen so that it uses the "real" backend regardless of the platform it is running on.

Fixes flutter/flutter#135052
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 9, 2023

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 Nov 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
e: impeller Impeller rendering backend issues and features requests engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list team-engine Owned by Engine team triaged-engine Triaged by Engine team
Projects
No open projects
Impeller
Archived in project
Milestone
No milestone
5 participants
@chinmaygarde @christopherfujino @jonahwilliams @jason-simmons @huycozy