tvOS: Enable Platform Service extension#9313
Conversation
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enables the Platform Service extension for tvOS, specifically for the dev.cobalt.coat.clientloginfo service, and implements logic to calculate and report the display refresh rate using CADisplayLink. A critical thread-safety issue was identified, as accessing UIKit properties from a background thread could lead to application crashes, potentially exploitable for a Denial of Service (DoS) attack. Additionally, the review highlighted a critical memory leak due to a retain cycle with CADisplayLink, a potential correctness issue with run loop selection, and areas for improvement in parameter handling and consistency within the new platform service implementation.
jkim-julie
force-pushed
the
platform-service
branch
2 times, most recently
from
343dd99 to
ad79394
Compare
This enables PlatformService cobalt extension and adds ‘dev.cobalt.coat.clientloginfo’ which has the same behaviour with C25. Bug: 487879114
jkim-julie
force-pushed
the
platform-service
branch
from
ad79394 to
eaecb03
Compare
| const char* kClientLogInfoServiceName = "dev.cobalt.coat.clientloginfo"; | ||
|
|
||
| bool Has(const char* name) { | ||
| if (!name) { | ||
| return false; | ||
| } | ||
|
|
||
| if (strcmp(name, kClientLogInfoServiceName) == 0) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
Let's take the opportunity and use more C++ here:
constexpr std::string_view kClientLogInfoServiceName = "...";
bool Has(const char* name) {
if (!name) {
return false;
}
return name == kClientLogInfoServiceName;
}| #include "starboard/extension/platform_service.h" | ||
| #import "starboard/tvos/shared/starboard_application.h" | ||
|
|
||
| typedef struct CobaltExtensionPlatformServicePrivate { |
There was a problem hiding this comment.
I don't think this needs to exist at all, context and receive_callback are not used anywhere, so Send()'s implementation could be merged directly into Send() below.
| uint64_t* output_length, | ||
| bool* invalid_state) { | ||
| if (!output_length || !invalid_state) { | ||
| return NULL; |
| SBDGetApplication().displayRefreshRate); | ||
|
|
||
| *output_length = string.size() + 1; | ||
| char* output = (char*)malloc(*output_length); |
There was a problem hiding this comment.
| char* output = (char*)malloc(*output_length); | |
| auto* output = malloc(*output_length); |
| char* output = (char*)malloc(*output_length); | ||
| if (!output) { | ||
| *invalid_state = true; | ||
| return NULL; |
There was a problem hiding this comment.
| return NULL; | |
| return nullptr; |
| SBDGetApplication().maximumFramesPerSecond, | ||
| SBDGetApplication().displayRefreshRate); | ||
|
|
||
| *output_length = string.size() + 1; |
There was a problem hiding this comment.
| *output_length = string.size() + 1; | |
| // Other extensions do not include a NUL-terminating byte. This is done here for compatibility with C25. | |
| *output_length = string.size() + 1; |
| } | ||
|
|
||
| *invalid_state = false; | ||
| std::string string = starboard::FormatString( |
There was a problem hiding this comment.
| std::string string = starboard::FormatString( | |
| const std::string string = starboard::FormatString( |
| } | ||
| } | ||
|
|
||
| - (void)update:(CADisplayLink*)sender { |
There was a problem hiding this comment.
I haven't tried it out myself, but I think it should be possible to plug into https://source.chromium.org/chromium/chromium/src/+/main:components/viz/common/frame_sinks/external_begin_frame_source_ios.mm which already sets up a CADisplayLink, performs the interval calculation and dispatches it.
If you subclass SimpleBeginFrameObserver, you should be able to retrieve interval as frame_interval. If this works, you can just send this result to SBDGetApplication() and perform the division when the value is requested, as I don't think Kabuki will need this on every frame update.
| NSInteger _maximumFramesPerSecond; | ||
|
|
||
| // The last display refresh rate. | ||
| std::atomic<double> _lastDisplayRefreshRate; |
There was a problem hiding this comment.
I don't think this needs to be an atomic variable, especially if you use PostTask() from cobalt shell to make the writes sequential.
2 participants
This enables PlatformService cobalt extension and adds ‘dev.cobalt.coat.clientloginfo’ which has the same behaviour with C25.
Bug: 487879114