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

[embedder] Ensure destruction called on present #38078

Merged
merged 1 commit into from Dec 7, 2022

Conversation

cbracken
Copy link
Member

@cbracken cbracken commented Dec 5, 2022

This is a followup to #38038.

In that patch, a destruction callback for textures created via the FlutterRendererConfig callbacks (as opposed to by the FlutterCompositor callbacks) was added and passed through to the texture info attached to the SurfaceFrame generated in GPUSurfaceMetalSkia::AcquireFrameFromMTLTexture (called via GPUSurfaceMetalSkia::GetMTLTexture, which invokes the get_next_drawable_callback), however, in order for the destruction callback to make it to the presented Skia texture, it needs to be passed through to the present callback here:
https://github.com/flutter/engine/blob/5545ccf8719c9568d8f521f9f18a869ca9e10056/shell/gpu/gpu_surface_metal_skia.mm#LL233
which is invoked by the submit callback passed to Skia:
https://github.com/flutter/engine/blob/5545ccf8719c9568d8f521f9f18a869ca9e10056/shell/gpu/gpu_surface_metal_skia.mm#LL239

The present callback is implemented in EmbedderSurface::PresentTexture, which invokes the present callback registered in FlutterMetalRendererConfig.present_drawable_callback. This patch ensures that the destruction callback is passed through to Skia's present callback so destruction occurs in the right place.

Issue: flutter/flutter#116381

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@flutter-dashboard flutter-dashboard bot added embedder Related to the embedder API needs tests labels Dec 5, 2022
@cbracken cbracken marked this pull request as draft December 5, 2022 19:43
@cbracken cbracken requested review from iskakaushik and chinmaygarde and removed request for iskakaushik December 7, 2022 17:08
@cbracken cbracken marked this pull request as ready for review December 7, 2022 17:08
This is a followup to flutter#38038.

In that patch, a destruction callback for textures created via the
FlutterRendererConfig callbacks (as opposed to by the FlutterCompositor
callbacks) was added and passed through to the texture info attached to
the SurfaceFrame generated in
GPUSurfaceMetalSkia::AcquireFrameFromMTLTexture (called via
GPUSurfaceMetalSkia::GetMTLTexture, which invokes the
get_next_drawable_callback), however, in order for the destruction
callback to make it to the presented Skia texture, it needs to be passed
through to the present callback here:
https://github.com/flutter/engine/blob/5545ccf8719c9568d8f521f9f18a869ca9e10056/shell/gpu/gpu_surface_metal_skia.mm#LL233

which is invoked by the submit callback passed to Skia:
https://github.com/flutter/engine/blob/5545ccf8719c9568d8f521f9f18a869ca9e10056/shell/gpu/gpu_surface_metal_skia.mm#LL239

The present callback is implemented in EmbedderSurface::PresentTexture,
which invokes the present callback registered in
FlutterMetalRendererConfig.present_drawable_callback. This patch ensures
that the destruction callback is passed through to Skia's present
callback so destruction occurs in the right place.

Issue: flutter/flutter#116381
@cbracken
Copy link
Member Author

cbracken commented Dec 7, 2022

Found a way to update the test to ensure the timing of destruction: ensure it doesn't happen until after we get the surface present callback.

@cbracken cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 7, 2022
@auto-submit auto-submit bot merged commit 2f5b67e into flutter:main Dec 7, 2022
@cbracken cbracken deleted the destruction-callback-present branch December 8, 2022 02:17
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 9, 2022
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 9, 2022
…116802)

* bd8bcf956 Roll Fuchsia Mac SDK from crEcyXdyZ686cAqMV... to pMV6A0ykZQ8aA3NG2... (flutter/engine#38120)

* dec8b5221 Preliminary implementation of UIA for A11y on Windows (flutter/engine#37754)

* 5545ccf87 Roll Fuchsia Linux SDK from NlJGkMbtZqQ6_BCpu... to xn8ztWtp-zww-jObz... (flutter/engine#38122)

* 80a15a419 Create FlutterActivity/FlutterFragment using light weight engine with FlutterEngineGroup (flutter/engine#36963)

* 5caef8585 Full implementation of text-input-test (flutter/engine#37986)

* 8f6036e58 Reland fix wrong VSYNC event (flutter/engine#37865)

* 4101c363c [iOS] Change locale format for spell check (flutter/engine#38080)

* 2f5b67e4d [embedder] Ensure destruction called on present (flutter/engine#38078)

* 0bddc6045 [Impeller Scene] Depth attachment; baked lighting example (flutter/engine#38118)

* 6aa4ccd60 Remove dlCanvasRecorder from flutter::PictureRecorder (flutter/engine#38127)

* dbb5284f2 [Windows] Add more cursor plugin tests (flutter/engine#38112)

* 6e91204d9 Roll Fuchsia Mac SDK from pMV6A0ykZQ8aA3NG2... to 9SnrQ0vbR8IC7UIoP... (flutter/engine#38135)

* 3140ad924 [Impeller] order metal samplers according to declared order and not usage order (flutter/engine#38115)

* 84abf21d4 Remove autoninja. (flutter/engine#38136)

* 8a113d328 [embedder] Expose metal surface from test context (flutter/engine#38133)

* 1ef25b63f Roll Fuchsia Mac SDK from 9SnrQ0vbR8IC7UIoP... to aMW0DjntzFJj4RoR3... (flutter/engine#38139)

* 748b3bc15 Revert "Remove dlCanvasRecorder from flutter::PictureRecorder (#38127)" (flutter/engine#38137)

* b6daf3d06 [embedder] Consistent naming for GL/Metal tests (flutter/engine#38141)

* 339d04baf [web] Trivial fix for non-static interop JS interop class. (flutter/engine#38126)

* 1fcbb9c11 [tools] Eliminate version on Obj-C docs (flutter/engine#38145)

* 71928b6a4 [Impeller] Use DrawPath instead of Rect geometry when the paint style is stroke (flutter/engine#38146)

* 23ce8fdbc Roll Skia from dd3285a80b23 to f84dc9303045 (4 revisions) (flutter/engine#38123)

* 366f8663b Roll Skia from f84dc9303045 to 2691cd7b4110 (40 revisions) (flutter/engine#38151)

* 447e7013e Roll Skia from 2691cd7b4110 to 711396b81248 (1 revision) (flutter/engine#38152)

* cd5d91bf9 Pylint testing/run_tests.py (flutter/engine#38016)

* aafac083b Roll Skia from 711396b81248 to b253b10374e7 (7 revisions) (flutter/engine#38157)

* 799dc78e8 Roll Fuchsia Linux SDK from xn8ztWtp-zww-jObz... to rRJIjuO-dPNCpCTd9... (flutter/engine#38134)

* 3aa3d2a8f Massage the JS interop around `didCreateEngineInitializer` (flutter/engine#38147)

* 030950f30 Roll Skia from b253b10374e7 to ec407902999b (3 revisions) (flutter/engine#38158)
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
…lutter#116802)

* bd8bcf956 Roll Fuchsia Mac SDK from crEcyXdyZ686cAqMV... to pMV6A0ykZQ8aA3NG2... (flutter/engine#38120)

* dec8b5221 Preliminary implementation of UIA for A11y on Windows (flutter/engine#37754)

* 5545ccf87 Roll Fuchsia Linux SDK from NlJGkMbtZqQ6_BCpu... to xn8ztWtp-zww-jObz... (flutter/engine#38122)

* 80a15a419 Create FlutterActivity/FlutterFragment using light weight engine with FlutterEngineGroup (flutter/engine#36963)

* 5caef8585 Full implementation of text-input-test (flutter/engine#37986)

* 8f6036e58 Reland fix wrong VSYNC event (flutter/engine#37865)

* 4101c363c [iOS] Change locale format for spell check (flutter/engine#38080)

* 2f5b67e4d [embedder] Ensure destruction called on present (flutter/engine#38078)

* 0bddc6045 [Impeller Scene] Depth attachment; baked lighting example (flutter/engine#38118)

* 6aa4ccd60 Remove dlCanvasRecorder from flutter::PictureRecorder (flutter/engine#38127)

* dbb5284f2 [Windows] Add more cursor plugin tests (flutter/engine#38112)

* 6e91204d9 Roll Fuchsia Mac SDK from pMV6A0ykZQ8aA3NG2... to 9SnrQ0vbR8IC7UIoP... (flutter/engine#38135)

* 3140ad924 [Impeller] order metal samplers according to declared order and not usage order (flutter/engine#38115)

* 84abf21d4 Remove autoninja. (flutter/engine#38136)

* 8a113d328 [embedder] Expose metal surface from test context (flutter/engine#38133)

* 1ef25b63f Roll Fuchsia Mac SDK from 9SnrQ0vbR8IC7UIoP... to aMW0DjntzFJj4RoR3... (flutter/engine#38139)

* 748b3bc15 Revert "Remove dlCanvasRecorder from flutter::PictureRecorder (flutter#38127)" (flutter/engine#38137)

* b6daf3d06 [embedder] Consistent naming for GL/Metal tests (flutter/engine#38141)

* 339d04baf [web] Trivial fix for non-static interop JS interop class. (flutter/engine#38126)

* 1fcbb9c11 [tools] Eliminate version on Obj-C docs (flutter/engine#38145)

* 71928b6a4 [Impeller] Use DrawPath instead of Rect geometry when the paint style is stroke (flutter/engine#38146)

* 23ce8fdbc Roll Skia from dd3285a80b23 to f84dc9303045 (4 revisions) (flutter/engine#38123)

* 366f8663b Roll Skia from f84dc9303045 to 2691cd7b4110 (40 revisions) (flutter/engine#38151)

* 447e7013e Roll Skia from 2691cd7b4110 to 711396b81248 (1 revision) (flutter/engine#38152)

* cd5d91bf9 Pylint testing/run_tests.py (flutter/engine#38016)

* aafac083b Roll Skia from 711396b81248 to b253b10374e7 (7 revisions) (flutter/engine#38157)

* 799dc78e8 Roll Fuchsia Linux SDK from xn8ztWtp-zww-jObz... to rRJIjuO-dPNCpCTd9... (flutter/engine#38134)

* 3aa3d2a8f Massage the JS interop around `didCreateEngineInitializer` (flutter/engine#38147)

* 030950f30 Roll Skia from b253b10374e7 to ec407902999b (3 revisions) (flutter/engine#38158)
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 embedder Related to the embedder API
Projects
None yet
2 participants