[embedder] [metal] Embedder API can support externally composited Metal textures #24327
Conversation
|
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. |
iskakaushik
force-pushed
the
embedder_external_texture_metal
branch
3 times, most recently
from
4fe5381 to
7d232ef
Compare
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
I think some more clarity about the ownership of the texture handles in the documentation for embedder.h would be great. Other than that, a few nits about error handling and such.
Can we write a test for this on the Mac embedder_unittests harness?
shell/platform/embedder/embedder.h
Outdated
| /// Supported textures are YUVA and RGBA, in case of YUVA we expect 2 texture | ||
| /// handles to be provided by the embedder, Y first and UV next. In case of | ||
| /// RGBA only one should be passed. | ||
| /// These are individually aliases for id<MTLTexture> which will be released |
There was a problem hiding this comment.
Can you clarify the lifecycle of these handles? Is it transfer-in or transfer-none (in which case the engine will retain them)? I'd prefer the latter as its easier to reason about in case of errors.
There was a problem hiding this comment.
Yup, these are transfer-none, I improved the documentation.
iskakaushik
force-pushed
the
embedder_external_texture_metal
branch
from
7d232ef to
a855fa4
Compare
|
@chinmaygarde i've also added a test |
|
@chinmaygarde friendly ping on this :-) |
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
Sorry about the slow reviews, this is a lot of context to page in and the stability guarantees around the embedder API need careful review. I think other than the minor handle leak and the const-ness of the textures struct field, this patch is good to go. I'd still like to have a unit-test that covers the use of this API from the embedders perspective but maybe we can handle that in a followup.
shell/platform/embedder/embedder.cc
Outdated
| size_t height) -> std::unique_ptr<FlutterMetalExternalTexture> { | ||
| FlutterMetalExternalTexture* texture = | ||
| new FlutterMetalExternalTexture(); | ||
| if (!ptr(user_data, texture_identifier, width, height, texture)) { |
There was a problem hiding this comment.
In case of error, the texture struct is leaked. IMO, it would be better to use a unique pointer to begin with instead of using a raw pointer first and the wrapping it later in a an auto pointer.
| int64_t /* texture identifier */, | ||
| size_t /* width */, | ||
| size_t /* height */, | ||
| FlutterMetalExternalTexture* /* texture out */); |
There was a problem hiding this comment.
Can we remove the out here? We aren't actually passing ownership of the texture out to the embedder. The embedder is only meant to populate the fields on this structs.
There was a problem hiding this comment.
Oh, I just realized. If the caller is meant to populate the fields of this struct, the textures handle cannot be const. Sorry about that suggestion. Can we back just that one bit out? BTW, an embedder_unittest that creates an engine instance using the Metal renderer config and populates the texture would have caught this. I realize that you have a test that just covers the new subsystem, but there are no tests that cover the embedders use of this API. Can we close this gap as well please?
shell/platform/embedder/embedder.cc
Outdated
| int64_t texture_identifier, size_t width, | ||
| size_t height) -> std::unique_ptr<FlutterMetalExternalTexture> { | ||
| FlutterMetalExternalTexture* texture = | ||
| new FlutterMetalExternalTexture(); |
There was a problem hiding this comment.
This constructor will not populate the struct_size field of FlutterMetalExternalTexture. Just like the engine cares about not reading past the end of the struct in case of versioning mismatches, the embedder might perform similar checks.
This moves the texture resolution out of embedder.cc and into embedder external_texture_gl.cc
iskakaushik
force-pushed
the
embedder_external_texture_metal
branch
from
4d8d025 to
967e0a4
Compare
|
Thanks for the updates. Looks great! |
…ited Metal textures (flutter/engine#24327)
…ited Metal textures (flutter/engine#24327)
…ited Metal textures (flutter/engine#24327)
…ited Metal textures (flutter/engine#24327)
…ited Metal textures (flutter/engine#24327)
…ited Metal textures (flutter/engine#24327)
…ited Metal textures (flutter/engine#24327)
3 participants
No description provided.