Set-up a comprehensive check when trying to reuse MediaKeySystemAccess #1597
Conversation
peaBerberian
force-pushed
the
fix/comprehensive-cached-check
branch
from
de73819 to
ce6a11f
Compare
peaBerberian
force-pushed
the
fix/comprehensive-cached-check
branch
from
ce6a11f to
4be0f01
Compare
| * @param {Object} newConfiguration | ||
| * @param {Object} prevConfiguration | ||
| * @param {MediaKeySystemAccess} currentKeySystemAccess | ||
| * @param {Object} currentKeySystemOptions | ||
| * @returns {null|Object} |
There was a problem hiding this comment.
completing jsdoc would help understand the behavior of this function
There was a problem hiding this comment.
While doing that I realized that it was unnecessarily complicated. I added a commit that renamed that function to isNewMediaKeySystemConfigurationCompatibleToPreviousOne and simplified its call site
There was a problem hiding this comment.
I think isNewMediaKeySystemConfigurationCompatibleWithPreviousOne sounds better
There was a problem hiding this comment.
You're right, I fixed it
| const prevDistinctiveIdentifier = prevConfiguration.distinctiveIdentifier ?? "optional"; | ||
| const newDistinctiveIdentifier = newConfiguration.distinctiveIdentifier ?? "optional"; | ||
| if (prevDistinctiveIdentifier !== newDistinctiveIdentifier) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Why does the prevConfiguration matters ?
Shoudn't we just look at the newConfiguration and see if it's compatible with the currentKeySystemAccess?
There was a problem hiding this comment.
I found it easier to reason about as currentKeySystemAccess.getConfiguration() has different rules.
Like for audioCapabilities and videoCapabilities for example, where the result is filtered but where you don't want it to be filtered in this specific case.
Also I would guess that most applications will provide similar keySystems parameters for each load
| const prevSessionTypes = prevConfiguration.sessionTypes ?? []; | ||
| const newSessionTypes = newConfiguration.sessionTypes ?? []; | ||
| if (!isArraySubsetOf(newSessionTypes, prevSessionTypes)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
From what I understand it's working like that, am I wrong?
1st checkCachedMediaKeySystemAccess call:
newConfiguration: ["temporary", "persistant"]
prevSessionTypes: []
-> creating session1 of type ["temporary", "persistant"]
2nd checkCachedMediaKeySystemAccess call:
newConfiguration: ["temporary",]
prevSessionTypes: ["temporary", "persistant"]
-> reusing session 1, ("temporary" is a subset of ["temporary", "persistant"])
3rd checkCachedMediaKeySystemAccess call:
newConfiguration: ["temporary", "persistant"]
prevSessionTypes: ["temporary",]
-> creating session 2, but we could have used session1 !!
There was a problem hiding this comment.
I think prevConfiguration will still be the old ["temporary", "persistant"] one in this case
There was a problem hiding this comment.
Maybe we can add a unit test for getMediaKeySystemAccess and check that it returns correctly an event of type IReuseMediaKeySystemAccessEvent in this case?
There was a problem hiding this comment.
I think it may be a biggest bang for the buck if I check for MediaKeySytemAccess reusage in various keySystems configuration in an integration test as allowed here #1478
I mean we could do both kinds of tests and it would be theoretically better, but decrypt/ code changes often and I don't like adding supplementary friction here.
We could also just test the "should we reuse cache" function, which would not need any mock and thus not have the issues of testing getMediaKeySystemAccess (which requires mocking requestMediaKeySystemAccess and its return value, MediaKeySystemAttacher, and maybe some other utils), but it seems simple enough to understand the various cases that here I'm unsure of the benefits.
There was a problem hiding this comment.
Here's an example of how those integration tests could be implemented:
e52bc61
I did not add tests for everything yet, it's just to show you what I mean here.
There was a problem hiding this comment.
I tried on my own to make a test, it needs to mock the requestMediaKeySystemAccess API, I think this is reasonably simple:
#1603
|
This is outside of the PR but the jsdoc is wrong:
The name of the key is |
While working on cases where an application wants to set-up different DRM configurations depending on the content, yet still keep the same `MediaKeys` instance if possible (to keep our `MediaKeySession` cache), I tried to make sure we support at worse a case where a more constrained configuration would still be considered as compatible to a less constrained one - as long as there's no loss of features / capabilities between the two. After attempts specific to the RxPlayer's `keySystems` API format, I ended up relying on the asked `MediaKeySystemConfiguration`s (both the new wanted one and the one that led to the current `MediaKeySystemAccess`) instead, as the corresponding logic seemed easier to me to maintain.
peaBerberian
force-pushed
the
fix/comprehensive-cached-check
branch
from
4be0f01 to
4410d6d
Compare
|
I updated the comments and also removed some of them that was redundant. |
peaBerberian
force-pushed
the
fix/comprehensive-cached-check
branch
from
e5b2cde to
87df748
Compare
While working on cases where an application wants to set-up different DRM configurations depending on the content, yet still keep the same
MediaKeysinstance if possible (to keep ourMediaKeySessioncache), I tried to make sure we support at worse a case where a more constrained configuration would still be considered as compatible to a less constrained one - as long as there's no loss of features / capabilities between the two.After attempts specific to the RxPlayer's
keySystemsAPI format, I ended up relying on the askedMediaKeySystemConfigurations (both the new wanted one and the one that led to the currentMediaKeySystemAccess) instead, as the corresponding logic seemed easier to me to maintain.