[metal] Darwin unified external metal textures #24157

iskakaushik · 2021-02-03T01:13:35Z

No description provided.

flutter-dashboard · 2021-02-03T17:52:15Z

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.

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.

chinmaygarde

I have a few doubts about the memory management of certain CF objects. Other than that, I only eyeballed the code that was moved which look fine to me.

chinmaygarde · 2021-02-03T20:36:05Z

shell/platform/darwin/graphics/BUILD.gn

@@ -7,19 +7,23 @@ assert(is_ios || is_mac)
 import("//flutter/common/config.gni")

 source_set("graphics") {
-  cflags_objc = flutter_cflags_objc
-  cflags_objcc = flutter_cflags_objcc
+  cflags_objcc = [ "-fobjc-arc" ]


Shouldn't this be in addition to the flutter_cflags_objc? I am not sure what those flags currently are but we are no longer applying them to this target.

I noticed that the other targets that enable arc, only had this will verify.

I created a new high level param for arc targets and unified it.

chinmaygarde · 2021-02-03T20:37:32Z

shell/platform/darwin/graphics/FlutterDarwinContextMetal.mm

@@ -33,32 +33,39 @@ - (instancetype)initWithMTLDevice:(id<MTLDevice>)device

    if (!_device) {
      FML_DLOG(ERROR) << "Could not acquire Metal device.";
-      [self release];
      return nil;


Can we decorate translation units with FLUTTER_ASSERT_ARC and FLUTTER_ASSERT_NON_ARC now that we are mixing the two in the engine?

Added it to the ARC targets that i've touched.

chinmaygarde · 2021-02-03T20:38:05Z

shell/platform/darwin/graphics/FlutterDarwinContextMetal.h

+/**
+ * Creates an external texture with the specified ID and contents.
+ */
+- (FlutterDarwinExternalTextureMetal*)externalTextureWithID:(int64_t)textureID


IMO createExternalTexture:withIdentifier: seems more idiomatic.

chinmaygarde · 2021-02-03T20:42:41Z

shell/platform/darwin/graphics/FlutterDarwinContextMetal.mm

+                                                  nil,      // cache attributes (nil default)
+                                                  _device,  // metal device
+                                                  nil,      // texture attributes (nil default)
+                                                  &_textureCache  // [out] cache


CoreFoundation objects need to be explicitly released. IIRC, this would leak the texture cache in the dealloc unless __bridge_transfered

I was worried about the same. I will try to add a test to see that everything that was allocated gets collected.

I wasn't able to come up with a way to test de-alloc in a nice way. I audited the code and made sure that are CoreFoundation objects are released and retained as necessary.

chinmaygarde · 2021-02-03T20:45:30Z

shell/platform/darwin/graphics/FlutterDarwinExternalTextureMetal.mm

+                           textureID:(int64_t)textureID
+                             texture:(NSObject<FlutterTexture>*)texture {
+  if (self = [super init]) {
+    _textureCache = textureCache;


Isn't this missing a CFRetain and subsequent release in the dealloc? We might end up with a dangling reference otherwise.

chinmaygarde · 2021-02-03T20:47:07Z

shell/platform/darwin/graphics/FlutterDarwinExternalTextureMetal.mm

+  if (needsUpdatedTexture) {
+    CVPixelBufferRef pixelBuffer = [_externalTexture copyPixelBuffer];
+    if (!pixelBuffer) {
+      pixelBuffer = std::move(_lastPixelBuffer);


What does std::move with ARC even do?

Doesn't make sense :-) I've used CVPixelBufferRetain and CVPixelBufferRelease as needed.

…e#24157)

This reverts commit 6a2df7f.

) This reverts commit 6a2df7f.

…e#24157)

)

…er#24157)" (flutter#24244)" This reverts commit 2e10a97.

)

…" (flutter#24223) This reverts commit 6a2df7f.

…" (flutter#24244)

…" (flutter#24350)

[metal] Darwin unified external metal textures

b219ced

flutter-dashboard bot added the platform-ios label Feb 3, 2021

google-cla bot added the cla: yes label Feb 3, 2021

iskakaushik marked this pull request as ready for review February 3, 2021 17:52

iskakaushik requested a review from chinmaygarde February 3, 2021 17:52

chinmaygarde reviewed Feb 3, 2021

View reviewed changes

iskakaushik added 2 commits February 4, 2021 10:34

Address memory management issues and review comments

e2607e0

create a gn flag for arc

13ce5db

iskakaushik requested a review from chinmaygarde February 4, 2021 18:40

fix licenses

f40579c

chinmaygarde approved these changes Feb 4, 2021

View reviewed changes

iskakaushik added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Feb 4, 2021

fluttergithubbot merged commit 6a2df7f into flutter:master Feb 4, 2021

engine-flutter-autoroll mentioned this pull request Feb 4, 2021

Roll Engine from 8076607c3ea3 to 6a2df7f1c9f6 (4 revisions) flutter/flutter#75450

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 4, 2021

6a2df7f [metal] Darwin unified external metal textures (flutter/engin…

d752a38

…e#24157)

engine-flutter-autoroll mentioned this pull request Feb 4, 2021

Roll Engine from 8076607c3ea3 to 5f9ab78aabe9 (8 revisions) flutter/flutter#75459

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 4, 2021

6a2df7f [metal] Darwin unified external metal textures (flutter/engin…

2a014b8

…e#24157)

engine-flutter-autoroll mentioned this pull request Feb 5, 2021

Roll Engine from 8076607c3ea3 to 99eba1b8860b (10 revisions) flutter/flutter#75461

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 5, 2021

6a2df7f [metal] Darwin unified external metal textures (flutter/engin…

3b16282

…e#24157)

zanderso added a commit that referenced this pull request Feb 5, 2021

Revert "[metal] Darwin unified external metal textures (#24157)"

3ef75bc

This reverts commit 6a2df7f.

zanderso mentioned this pull request Feb 5, 2021

Revert "[metal] Darwin unified external metal textures" #24223

Merged

iskakaushik pushed a commit that referenced this pull request Feb 5, 2021

Revert "[metal] Darwin unified external metal textures (#24157)" (#24223

30bb5f1

) This reverts commit 6a2df7f.

engine-flutter-autoroll mentioned this pull request Feb 5, 2021

Roll Engine from b04955656c87 to 60b436d4f09b (18 revisions) flutter/flutter#75505

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 5, 2021

6a2df7f [metal] Darwin unified external metal textures (flutter/engin…

e8096ea

…e#24157)

engine-flutter-autoroll mentioned this pull request Feb 5, 2021

Roll Engine from b04955656c87 to 60b436d4f09b (18 revisions) flutter/flutter#75507

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 5, 2021

6a2df7f [metal] Darwin unified external metal textures (flutter/engin…

9180547

…e#24157)

engine-flutter-autoroll mentioned this pull request Feb 5, 2021

Roll Engine from b04955656c87 to 30bb5f1cbd5e (20 revisions) flutter/flutter#75511

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 5, 2021

6a2df7f [metal] Darwin unified external metal textures (flutter/engin…

7149278

…e#24157)

iskakaushik added a commit to iskakaushik/engine that referenced this pull request Feb 5, 2021

[metal] Darwin unified external metal textures (flutter#24157)

b1a9609

iskakaushik added a commit to iskakaushik/engine that referenced this pull request Feb 5, 2021

[metal] Darwin unified external metal textures (flutter#24157)

6fe1c8e

iskakaushik added a commit that referenced this pull request Feb 9, 2021

Reland "[metal] Darwin unified external metal textures (#24157)" (#24244

2e10a97

)

iskakaushik added a commit to iskakaushik/engine that referenced this pull request Feb 10, 2021

Revert "Reland "[metal] Darwin unified external metal textures (flutt…

6ffd28c

…er#24157)" (flutter#24244)" This reverts commit 2e10a97.

iskakaushik added a commit to iskakaushik/engine that referenced this pull request Feb 11, 2021

Reland "[metal] Darwin unified external metal textures (flutter#24157)"

e563100

iskakaushik added a commit that referenced this pull request Feb 11, 2021

Reland "[metal] Darwin unified external metal textures (#24157)" (#24350

a322e69

)

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

[metal] Darwin unified external metal textures (flutter#24157)

ab00872

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

Revert "[metal] Darwin unified external metal textures (flutter#24157)…

3cceb9a

…" (flutter#24223) This reverts commit 6a2df7f.

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

Reland "[metal] Darwin unified external metal textures (flutter#24157)…

2cc0b0d

…" (flutter#24244)

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

Reland "[metal] Darwin unified external metal textures (flutter#24157)…

12b2a43

…" (flutter#24350)

lsegal mentioned this pull request Jun 16, 2021

macos desktop Not able to display textures using FlutterTextureRegistry anymore on latest master, but works on stable. flutter/flutter#81120

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[metal] Darwin unified external metal textures #24157

[metal] Darwin unified external metal textures #24157

iskakaushik commented Feb 3, 2021

flutter-dashboard bot commented Feb 3, 2021

chinmaygarde left a comment

chinmaygarde Feb 3, 2021

iskakaushik Feb 4, 2021

iskakaushik Feb 4, 2021

chinmaygarde Feb 3, 2021

iskakaushik Feb 4, 2021

chinmaygarde Feb 3, 2021

iskakaushik Feb 4, 2021

chinmaygarde Feb 3, 2021

iskakaushik Feb 3, 2021

iskakaushik Feb 4, 2021

chinmaygarde Feb 3, 2021

iskakaushik Feb 4, 2021

chinmaygarde Feb 3, 2021

iskakaushik Feb 4, 2021

[metal] Darwin unified external metal textures #24157

[metal] Darwin unified external metal textures #24157

Conversation

iskakaushik commented Feb 3, 2021

flutter-dashboard bot commented Feb 3, 2021

chinmaygarde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment