[Proposal] Update mediaCapabilitiesProber API #1472

peaBerberian · 2024-06-26T17:15:28Z

After some brainstorming related to #1470, we noticed that as is, our old mediaCapabilitiesProber experimental tool could gain by being more flexible.

Especially, the getCompatibleDRMConfigurations method checked multiple EME MediaKeySystemConfiguration at 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 MediaKeySystemConfiguration per method call (and rename that method to checkDrmConfiguration).
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), use Promise.race to 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 timeout argument to it, but realized that it might be very complex to maintain, as theoretically checkDrmConfiguration could call multiple browser API sequentially in the future (as other mediaCapabilitiesProber'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.

Florent-Bouisset · 2024-10-22T14:03:27Z

doc/api/Tools/MediaCapabilitiesProber.md

+    console.log("This device is compatible with the given configuration!");
+    console.log("Wanted configuration:", configuration);
+    console.log("Compatible configuration:", compatibleConfiguration);


this example is not working, configuration and compatibleConfiguration are not defined

You're right, it's directly the compatible one instead. I'll update.

Florent-Bouisset · 2024-10-22T14:30:36Z

src/experimental/tools/mediaCapabilitiesProber/probers/isTypeSupportedWithFeatures.ts

+  const result: ISupportWithFeatures = (
+    globalScope as IGlobalScopeWithMSMediaKeysFeatures
+  ).MSMediaKeys.isTypeSupportedWithFeatures(type, features);


I'm not sure how this API works, do you have any documentation on this one?

It was an old edge feature only present on MSMediaKeys and was able to check for more precize policy support.

I also have trouble finding documentation on it today, my searches for now only lead to the RxPlayer and complaints from Netflix users :p

TypeScript at some point had it in its type definitions: https://raw.githubusercontent.com/Microsoft/TypeScript/v2.9.2/lib/lib.es6.d.ts

Though not anymore

Florent-Bouisset · 2024-10-22T14:39:46Z

src/experimental/tools/mediaCapabilitiesProber/probers/HDCPPolicy.ts

+    }
+  } catch (err) {
+    const error = err instanceof Error ? err : new Error("Unknown Error");
+    log.error("MCP: `probeHDCPPolicy` didn't succeed to create a MediaKeys", error);


From what I read, the error log here is not accurate:
We can get here if the API getStatusForPolicy does not exist, not because the MediaKeys failed to be created.

You're right requestMediaKeySystemAccess and createMediaKeys don't even have their try/catch.

I'll fix that

Florent-Bouisset · 2024-10-22T14:57:09Z

src/experimental/tools/mediaCapabilitiesProber/probers/__tests__/decodingInfo.test.ts

+    try {
+      await probeDecodingInfos(configuration);
+      resetDecodingInfos();
+    } catch (error) {
+      const message = error instanceof Error ? error.message : "";
+      thrownException = true;
+      expect(message).toEqual("Not enough arguments for calling mediaCapabilites.");
+      expect(decodingInfoStub).not.toHaveBeenCalled();
+      resetDecodingInfos();
+    }


remark about resetDecodingInfos(), it can be done only once outside the try..catch

try { await probeDecodingInfos(configuration); } catch (error) { const message = error instanceof Error ? error.message : ""; thrownException = true; expect(message).toEqual("Not enough arguments for calling mediaCapabilites."); expect(decodingInfoStub).not.toHaveBeenCalled(); } resetDecodingInfos();

But a similar code is runned in the afterEach() hook.
So we can delete that code from every test since it's already done in the hook and check that it works well

Florent-Bouisset · 2024-10-22T15:01:45Z

src/experimental/tools/mediaCapabilitiesProber/probers/__tests__/decodingInfo.test.ts

@@ -82,22 +80,21 @@ describe("MediaCapabilitiesProber probers - decodingInfo", () => {
      },
    };

-    expect.assertions(2);


I just learned about expect.assertions() is it something that should be avoided?

I don't like using it because it's something easy to forget about and it's not really a meaningful test beside ensuring that a code path with assertions has been taken, in which case I prefer a more explicit check like a boolean.

Florent-Bouisset · 2024-10-22T15:15:48Z

src/experimental/tools/mediaCapabilitiesProber/probers/__tests__/decodingInfo.test.ts

-    // eslint-disable-next-line @typescript-eslint/no-floating-promises
-    expect(probeDecodingInfos({})).rejects.toThrowError(
-      "MediaCapabilitiesProber >>> API_CALL: MediaCapabilities API not available",
+    return probeDecodingInfos({}).then(


why is there a return ?
Also there is jest modifier .rejects that help with this kind of test:

await expect(probeDecodingInfos({})).rejects.toThrowError("navigator.mediaCapabilites.decodingInfo is not available");

When tests return a promise in most testing frameworks, the test will reject when the promise rejects and resolve when the promise resolve.

I know about rejects, and that was what was there before, but for some reason I found returning the promise more readable.

Florent-Bouisset · 2024-10-22T15:16:02Z

src/experimental/tools/mediaCapabilitiesProber/probers/__tests__/decodingInfo.test.ts

+    return probeDecodingInfos({}).then(
+      () => {
+        throw new Error("Should not succeed");
+      },
+      (err: Error) => {
+        expect(err).toBeInstanceOf(Error);
+        expect(err.message).toEqual(
+          "navigator.mediaCapabilites.decodingInfo is not available",
+        );
+      },


Florent-Bouisset · 2024-10-22T15:40:37Z

src/experimental/tools/mediaCapabilitiesProber/probers/__tests__/mediaContentType.test.ts

-    expect(probeMediaContentType({})).rejects.toThrowError(
-      "MediaCapabilitiesProber >>> API_CALL: " + "MediaSource API not available",
-    );
+    expect(() => probeMediaContentType({})).toThrowError("MediaSource API not available");
  });


the eslint error could have been fixed by adding await before expect

Florent-Bouisset · 2024-10-22T15:41:54Z

src/experimental/tools/mediaCapabilitiesProber/probers/__tests__/mediaContentType.test.ts

+    expect(() => probeMediaContentType({})).toThrowError(
+      "MediaSource.isTypeSupported API not available",
    );


Florent-Bouisset · 2024-10-22T15:48:26Z

src/experimental/tools/mediaCapabilitiesProber/probers/__tests__/mediaDisplayInfos.test.ts

-      globalScope as { matchMedia: typeof globalScope.matchMedia | undefined }
-    ).matchMedia = origMatchMedia;
+    expect(() => probeMediaDisplayInfos({})).toThrowError("matchMedia API not available");
+    globalScope.matchMedia = origMatchMedia;


setting the matchMedia API should be done in the afterEach / beforeEach hooks

Florent-Bouisset · 2024-10-22T16:01:12Z

src/experimental/tools/mediaCapabilitiesProber/probers/isTypeSupportedWithFeatures.ts

+    support: ISupportWithFeatures,
+  ): "NotSupported" | "Unknown" | "Supported" {
+    if (support === "") {
+      throw new Error("Bad arguments for calling isTypeSupportedWithFeatures");


The API returns empty string if arguments are invalid ? Weird one

Florent-Bouisset · 2024-10-22T16:06:14Z

src/experimental/tools/mediaCapabilitiesProber/api.ts

+   * @param {Object} mediaConfig
+   * @returns {Promise.<string>}
+   */
+  async getDecodingCapabilities(mediaConfig: IMediaConfiguration): Promise<string> {


don't the type should be narrowed to "Supported" / "Unknown" / "NotSupported" instead of being a string ?

This reverts commit fdd172a.

peaBerberian added the proposal This Pull Request or Issue is only a proposal for a change with the expectation of a debate on it label Jun 26, 2024

peaBerberian force-pushed the misc/mcp-refacto branch from a78ca09 to bf9e326 Compare June 27, 2024 13:42

peaBerberian added this to the 4.2.0 milestone Jun 27, 2024

peaBerberian force-pushed the misc/mcp-refacto branch from bf9e326 to 52bf6ac Compare July 11, 2024 09:54

peaBerberian force-pushed the misc/mcp-refacto branch from 17e1a36 to 597dcb3 Compare July 24, 2024 16:44

peaBerberian added the Priority: 2 (Medium) This issue or PR has a medium priority. label Jul 26, 2024

peaBerberian force-pushed the misc/mcp-refacto branch 3 times, most recently from 99059eb to 538aff1 Compare September 4, 2024 16:57

peaBerberian modified the milestones: 4.2.0, 4.3.0 Oct 4, 2024

peaBerberian force-pushed the misc/mcp-refacto branch from 538aff1 to 0ea518d Compare October 8, 2024 15:02

Florent-Bouisset reviewed Oct 22, 2024

View reviewed changes

peaBerberian added 12 commits November 15, 2024 18:07

MCP: addCheckDrmConfiguration API

f73d9f3

MCP: Remove confusing log methods

c439578

MCP: rewrite getStatusForHDCP method

f56b78d

MCP: simplify checkDrmConfiguration implementation

52b60f7

MCP: re-implement getDecodingCapabilities

667bfc5

MCP: Remove getCompatibleDRMConfigurations

7ac5dc3

MCP: Rewrite getDisplayCapabilities

988b38f

Revert "MCP: Remove confusing log methods"

4d03e3d

This reverts commit fdd172a.

MCP: Remove now unneeded code

bc8ca1b

Update MediaCapabilitiesProber documentation

9f57df7

MCP: Fix unit tests and move around files

5c6fb35

MCP: Remove too complex to maintain timeout option

be7450a

peaBerberian added 2 commits November 15, 2024 18:29

MCP: fix integration tests

dec5c4f

Last unit test fix

a2ea9bf

peaBerberian force-pushed the misc/mcp-refacto branch from 0ea518d to a2ea9bf Compare November 15, 2024 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Proposal] Update mediaCapabilitiesProber API #1472

[Proposal] Update mediaCapabilitiesProber API #1472

peaBerberian commented Jun 26, 2024

Florent-Bouisset Oct 22, 2024

peaBerberian Oct 22, 2024

Florent-Bouisset Oct 22, 2024

peaBerberian Oct 22, 2024

peaBerberian Oct 22, 2024

Florent-Bouisset Oct 22, 2024

peaBerberian Oct 22, 2024

Florent-Bouisset Oct 22, 2024 •

edited

Loading

Florent-Bouisset Oct 22, 2024

peaBerberian Oct 22, 2024

Florent-Bouisset Oct 22, 2024

peaBerberian Oct 22, 2024 •

edited

Loading

Florent-Bouisset Oct 22, 2024

Florent-Bouisset Oct 22, 2024

Florent-Bouisset Oct 22, 2024

Florent-Bouisset Oct 22, 2024

Florent-Bouisset Oct 22, 2024

Florent-Bouisset Oct 22, 2024

[Proposal] Update mediaCapabilitiesProber API #1472

Are you sure you want to change the base?

[Proposal] Update mediaCapabilitiesProber API #1472

Conversation

peaBerberian commented Jun 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Florent-Bouisset Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peaBerberian Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Florent-Bouisset Oct 22, 2024 •

edited

Loading

peaBerberian Oct 22, 2024 •

edited

Loading