-
Notifications
You must be signed in to change notification settings - Fork 136
[Proposal] Update mediaCapabilitiesProber API #1472
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
Conversation
a78ca09 to
bf9e326
Compare
bf9e326 to
52bf6ac
Compare
17e1a36 to
597dcb3
Compare
99059eb to
538aff1
Compare
538aff1 to
0ea518d
Compare
src/experimental/tools/mediaCapabilitiesProber/probers/isTypeSupportedWithFeatures.ts
Show resolved
Hide resolved
src/experimental/tools/mediaCapabilitiesProber/probers/HDCPPolicy.ts
Outdated
Show resolved
Hide resolved
src/experimental/tools/mediaCapabilitiesProber/probers/__tests__/decodingInfo.test.ts
Show resolved
Hide resolved
src/experimental/tools/mediaCapabilitiesProber/probers/__tests__/decodingInfo.test.ts
Show resolved
Hide resolved
src/experimental/tools/mediaCapabilitiesProber/probers/__tests__/decodingInfo.test.ts
Show resolved
Hide resolved
src/experimental/tools/mediaCapabilitiesProber/probers/__tests__/decodingInfo.test.ts
Show resolved
Hide resolved
src/experimental/tools/mediaCapabilitiesProber/probers/__tests__/mediaContentType.test.ts
Show resolved
Hide resolved
src/experimental/tools/mediaCapabilitiesProber/probers/__tests__/mediaContentType.test.ts
Show resolved
Hide resolved
src/experimental/tools/mediaCapabilitiesProber/probers/__tests__/mediaDisplayInfos.test.ts
Outdated
Show resolved
Hide resolved
src/experimental/tools/mediaCapabilitiesProber/probers/isTypeSupportedWithFeatures.ts
Show resolved
Hide resolved
0ea518d to
a2ea9bf
Compare
a2ea9bf to
12248ee
Compare
00fc806 to
b7216b4
Compare
076c092 to
aaec181
Compare
This reverts commit fdd172a.
aaec181 to
4156a49
Compare
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
| export default function probeHDCPPolicy( | ||
| config: IMediaConfiguration, | ||
| ): Promise<[ProberStatus]> { | ||
| export default async function probeHDCPPolicy( |
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.
I feel that this function can either throw or return "NotSupported"
I'm wondering if we should catch any errors and return "NotSupported"
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.
Complex to say.
It fails if EME API failed to be called.
NotSupported would be if there's a high priority of the HDCP version being unsupported but here we're not so sure.
Maybe Unknown? I'm under the impression that for now Unknown is only for when the getStatusForPolicy API is not available, which is slightly different than e.g. requestMediaKeySystemAccess failing yet both would just indicate that it is unknown.....
Then, when applications handle promises, they generally also handle rejections (even linters often force that). So I'm not sure.
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.
I removed the Unkown and it only returns "Supported" / "NotSupported" or reject
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
ProbeHDCPPolicy can either, succeed with "Supported" | "NotSupported" | "Unknown" or reject. Success with "unknown" or reject both occurs when the navigator API failed to execute and the support of HDCP cannot be determined. To standardize the behavior of this function, the "Unknown" return is removed and replaced by a promise rejection.
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
After some brainstorming related to #1470, we noticed that as is, our old
mediaCapabilitiesProberexperimental tool could gain by being more flexible.Especially, the
getCompatibleDRMConfigurationsmethod checked multiple EMEMediaKeySystemConfigurationat once and returned information on actual configurations compatible to them once all of them have been checked.We wondered whether this method would not be more useful by making it less powerful: we'll now only check for a single
MediaKeySystemConfigurationper method call (and rename that method tocheckDrmConfiguration).Doing this signals to an application that it can just combine multiple calls if it wants the old behavior (e.g. through a
Promise.all), usePromise.raceto just obtain the first one etc. (this would still be possible with the previous API, but seemed unnatural to application developers).I also added a
timeoutargument to it, but realized that it might be very complex to maintain, as theoreticallycheckDrmConfigurationcould call multiple browser API sequentially in the future (as othermediaCapabilitiesProber's methods are doing).In that case, where are we supposed to apply the
timeout: to each browser API call or globally on the method?I also profited from this work to rewrite the implementation of other
mediaCapabilitiesProber's method without changing their API, as I found that a lot of its data-processing code was unneeded and difficult to follow.