Add texture support for macOS shell.

[cloudwebrtc]

Used to resolve flutter/flutter#30717 and google/flutter-desktop-embedding#107.

[stuartmorgan]

I've reviewed as much as I could (most of the comments are just small style things), but it seems that there is at least one important file missing from the PR so I couldn't fully review the structure.

[cloudwebrtc]

@stuartmorgan I solved all the problems, please review again.

[stuartmorgan]

Sorry for the delay on this, I'll be able to review the next round much sooner.

[stuartmorgan]

This appears to be identical to FlutterTexture.h other than the prefixes; now that macOS is in the same repo as iOS the goal is to share code whenever possible (and move away from the FLE prefix). You can just add that file to the frameworkSshared.gni list, and use the existing protocols in the macOS code.

(If you want to add these comments to FlutterTexture.h, ideally as a separate PR, that would be great, since the existing file isn't commented at all.)

[cloudwebrtc]

@chinmaygarde @stuartmorgan
I found in the macos test that CVOpenGLTextureCacheCreateTextureFromImage creates a GL_TEXTURE_RECTANGLE object instead of GL_TEXTURE_2D, which causes the texture to not be properly scaled to fill the flutter widget when rendering, so I used width/height in texture->copyPixelBuffer when the plugin receives the request, it scales the texture to the corresponding size according to the widget size provided by the engine, and then becomes normal when rendering.
I'm not sure to add width/height to ios/framework/Headers/FlutterTexture.h, or continue to use FLETexture.h, or should report a new issue in flutter engine.

[chinmaygarde]

You should be able to specify that the texture target is GL_TEXTURE_RECTANGLE by specifying the target property of FlutterOpenGLTexture. All the copying ought to be unnecessarily inefficient.

[stuartmorgan]

Please provide more context in this comment:

• Bridge between what and what?
• What texture copy request?

Providing some high-level explanation of how this class fits into the overall flow of passing textures back and forth wit the engine would be very helpful.

[stuartmorgan]

I'm having trouble parsing this comment; what is returning an FLEExternalTextureGL here?

[stuartmorgan]

The uses of the word "texture" in this class (header and implementation) need disambiguation. The class itself is a texture, and accepts two different objects that are also textures. In this method, kind of texture is the parameter? (The type provides the answer here, but not at a call site).

[stuartmorgan]

Looking more closely at the implementation, it looks like the texture being populated is the texture that is the last parameter; the current naming makes it sounds like some other internal texture is being populated from these arguments. As a general rule, you should never be naming the same parameter twice in the signature.

So unless I'm misunderstanding, this should be populateTexture:withWidth:height:.

[stuartmorgan]

No need to comment on the meaning of width/height, since this is just forwarding. (Specifically it seems like this comment is talking about implementation details that are in a different class.)

[stuartmorgan]

Please add more explanation to this comment:

• What is the texture being populated from?
• What is the meaning of the return value?

[chinmaygarde]

@cloudwebrtc Are you saying that CVOpenGLTextureGetTarget is not returning GL_TEXTURE_RECTANGLE? I am a little bit confused. There should be no issues with having to convert to GL_TEXTURE_2D here.

[cloudwebrtc]

@chinmaygarde See CVOpenGLTextureGetTarget from the code context
  It should be the returned GL_TEXTURE_RECTANGLE, but the problem is with the engine's texture rendering.
The whole picture of the problem is:

• 1, when I patched Texture for flutter/engine to macOS 10.14.4 (xcode 10.2)
  The following error will appear in the console at runtime, without displaying any texture frames.
  
• 2, so I tried to find the reason
  I solved it by patching skia

diff --git a/src/sksl/SkSLGLSLCodeGenerator.cpp b/src/sksl/SkSLGLSLCodeGenerator.cpp
index d003dc9397..04357e2a4c 100644
--- a/src/sksl/SkSLGLSLCodeGenerator.cpp
+++ b/src/sksl/SkSLGLSLCodeGenerator.cpp
@@ -656,7 +656,7 @@ void GLSLCodeGenerator::writeFunctionCall(const FunctionCall& c) {
                         proj = false;
                         break;
                     case SpvDimRect:
-                        dim = "Rect";
+                        dim = "2DRect";
                         proj = false;
                         break;
                     case SpvDimBuffer:

Then I can see the texture display, but the picture is not correct.

• 3, then I use the width/height in TextureFrameCallback to resized the video frame to this size on the plugin side, then get the texture display of the correct zoom size, and feel free to stretch ExampleEmbedder.app to display correctly.

shell/platform/embedder/embedder.h#L214

Typedef bool (*TextureFrameCallback)(void* /* user data */,
                                      Int64_t /* texture identifier */,
                                      Size_t /* width */,
                                      Size_t /* height */,
                                      FlutterOpenGLTexture* /* texture out */);

Looks like this:
plugins/flutter_webrtc/macos/FlutterRTCVideoRenderer.m#L52

I am not sure that the type of dim in SpvDimRect is a bug in skia on macos.

[stuartmorgan]

What's the current status on this? Is it waiting on more input from @chinmaygarde regarding the last comments?

[cloudwebrtc]

@stuartmorgan
Yes, I think we need to wait for @chinmaygarde reply, here is the necessity of the width/height parameter (about GL_TEXTURE_RECTANGLE rendering error and size scaling), if not, we can use the FlutterTexture.h header file directly from the iOS framework.

[stuartmorgan]

@chinmaygarde The questions above are not resolved; it's not clear to me why you approved this as-is.

We should absolutely not be adding a new FLETexture.h class. Ideally we can share them as discussed above, but if not then we need to come up with a solution that doesn't involve a new class using the legacy extension.

[cbracken]

@cloudwebrtc and @stuartmorgan since the PR is not currently in shape to be landed, how would you like to proceed with this PR?

[stuartmorgan]

Per my recent comments, we still need feedback from @chinmaygarde about how to resolve the Skia behavior in a way that is compatible with the existing FutterTexture.h if possible, and if not whether what the minimal possible platform divergence we can have in that header is.

[stuartmorgan]

Chinmay just flagged that I missed an ABI break. Updating review state just to make sure this doesn't accidentally get landed before his review.

It doesn't look like there's a straightforward way to resolve that given the lack of size indicator; we may have to branch several APIs with new versions. Or maybe Chinmay will have a better idea after looking more.

[chinmaygarde]

We are 90% of the way there except for the ABI breakage and the resizing on the CPU. I have added suggestions for both issues inline. Let me know if you have any questions about the implementation or need help in carrying it to completion. Really appreciate your effort and I apologize for my late replies to this PR.

[chinmaygarde]

You are not using the width and the height argument in this method at all!

The way this API works it that the engine tells the embedder what size it needs the texture to be. If the texture is not of that exact size, the engine will attempt to clamp or resize the texture to get something usable. The clamping behavior is what you seem to be running into.

There is information being thrown away in this method. That is, when you ask the texture to copyPixelBuffer, you are not giving it any indication about what the size of the pixel buffer should be. This method acts as if size does not matter. When, in fact, the engine care about the size and it is asking you for a texture of that size. This method is just not using it.

So now you have a texture of some unspecified size that you need to give to the engine because you didn't use the dimensions the engine asked of the embedder. To solve this issue, we now need to resort to the expensive device to host transfer so we can resize on the CPU before transferring that information back to the device.

If we don't throw away the information the engine gives us, we can be a whole lot faster. This can be done in one of the following ways:

• Augment copyPixelBuffer to take the width and the height. This way, plugins will know what texture the engine expects and can create texture of the right size to begin with.
• If it is unreasonable for plugins to expect to create textures of the correct size, we can still make the FlutterOpenGLTexture take a width and height parameter (subject to ABI restrictions I mentioned below). That way, we give the plugins the best chance to match texture sizes. If however, the size the engine expects is not the same as the size the plugin gives back to us, instead of implementing the resize via a software render pass, we can achieve the same by applying a transformation to the draw on the GPU. This can be modified by pushing a CTM on the canvas here.

Whatever you choose, let do as many operations on the GPU as possible by utilizing all information the engine give to the embedder.

[cloudwebrtc]

@chinmaygarde I made a few changes, removed to use CPU to resize the image, and the canvas to scale the image, it seems to work very well, but I did not add the width/height parameter to copyPixelBuffer, because this will change the copyPixelBuffer interface in FlutterTexture.h Consistency in iOS/macOS.

[chinmaygarde]

I made a few changes, removed to use CPU to resize the image

Beautiful.

but I did not add the width/height parameter to copyPixelBuffer, because this will change the copyPixelBuffer interface in FlutterTexture.h

In that case, can you modify the populateTextureWithIdentifier... call to not take the width and height at all? It is confusing for some calls to require arguments that they don't end up using.

I think besides the unused arguments, everything else looks good. Thanks.

[cloudwebrtc]

@chinmaygarde @stuartmorgan Done, please review again.

[chinmaygarde]

LGTM. I'll land it when the tree is open.  Thanks for all the effort.

[chinmaygarde]

When you get a chance, can you update this to ToT? I am not able to patch this PR myself.

The concern was about the ABI break which has been addressed.

[cloudwebrtc]

@chinmaygarde I have resolved the conflict and wait for CI verification.

[cloudwebrtc]

@chinmaygarde Passed the CI test, waiting for you to merge. :)