-
Notifications
You must be signed in to change notification settings - Fork 131
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
Implement MediaKeys.getMetrics() #4837
base: main
Are you sure you want to change the base?
Conversation
3968bb9
to
7298bc4
Compare
static_idl_files_in_modules += get_path_info( | ||
[ "//third_party/blink/renderer/modules/mediasource/source_buffer_write_head.idl" ], | ||
"abspath") | ||
|
||
# MediaKeys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: You could combine these two into a single addition to static_idl_files_in_modules
but leave the comments.
LGTM, though you may need to move the idl/implementation somewhere else depending on what Holden/Andrew think |
"media_keys_get_metrics.cc", | ||
"media_keys_get_metrics.h", | ||
] | ||
deps += [ "//starboard($starboard_toolchain)" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use //starboard:starboard_group
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, looking good.
auto* cdm_context = cdm_context_ref->GetCdmContext(); | ||
DCHECK(cdm_context); | ||
|
||
auto sbdrm = cdm_context->GetSbDrmSystem(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/sbdrm/sb_drm/ ?
|
||
int metrics_size = -1; | ||
const void* metrics = SbDrmGetMetrics(sbdrm, &metrics_size); | ||
DCHECK(metrics_size > 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: DCHECK_GT(metrics_size, 0);
SecureContext, | ||
ImplementedAs=MediaKeysGetMetrics | ||
] partial interface MediaKeys { | ||
[RaisesException] Uint8Array getMetrics(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be a DOMString ? It would simplify the conversion.
int metrics_size = -1; | ||
const void* metrics = SbDrmGetMetrics(sbdrm, &metrics_size); | ||
DCHECK(metrics_size > 0); | ||
metrics_results.assign(static_cast<const char*>(metrics), metrics_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this function signature std::string ...::GetMetrics() {
and then here just do return std::string(static_cast<const char*>(metrics), metrics_size);
?
ActiveScriptWrappable, | ||
SecureContext, | ||
ImplementedAs=MediaKeysGetMetrics | ||
] partial interface MediaKeys { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add an hmlt test file in or around https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/media/encrypted-media/ ?
@@ -54,6 +54,10 @@ class PLATFORM_EXPORT WebContentDecryptionModuleImpl | |||
void GetStatusForPolicy(const WebString& min_hdcp_version_string, | |||
WebContentDecryptionModuleResult result) override; | |||
|
|||
#if BUILDFLAG(USE_STARBOARD_MEDIA) | |||
void GetMetrics(std::string &metrics_results) override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a unit test for this? Not sure the mocking capabilities of SbDrmGetMetrics
but the CdmAdapter might be a pickle, and perhaps is not injectable.
b/381298846
Extend MediaKeys API with getMetrics.
Fixes the YTS EME getMetrics test, which is one of the golden ATV tests, required for fishfood.