fix: excessive requests when using HDCP fallback with LDL #225

harisha-swaminathan · 2024-06-30T18:37:49Z

When we have HDCP fallback and short-duration license, there are excessive requests because we're renewing licenses and subsequently fetching new licenses if the renewal fails. This can be avoided by taking an approach similar to Shaka player by not requesting a new license when the old one expires.

The PR also provides an option to limit license renewals when the player is idle (before playback begins, after playback ends and player abandoned in paused state).

License is not renewed if it expires before playback begins if limitRenewalsBeforePlay is set to true. New requests are made on 'play'
License is not renewed after playback ends by default. New requests are made on 'replay' or 'seek back'
License is not renewed if player remains paused for more than a specified duration that's set using limitRenewalsMaxPauseDuration option

adrums86

I really like this change! License expiration/renewal cannot be effectively used for concurrency enforcement and detection if we attempt a renewal automatically on expiration.
One small thing, boolean flags make me nervous for some irrational reason, could we get a test around that flag and the new 'ended' logic?

src/plugin.js

harisha-swaminathan · 2024-07-16T21:46:19Z

src/eme.js

@@ -160,6 +161,10 @@ export const makeNewRequest = (player, requestOptions) => {
          return;
        }

+        if (event.messageType === 'license-renewal' && (!player.hasStarted() || player.paused())) {
+          return;
+        }


This is an attempt to reduce the number of license renewals made when the player is idle (before playback begins) or when the player is paused for too long.

A more ideal solution to this would probably be closing and removing the sessions if the player is abandoned in a paused state and then making a new request when unpaused. Maybe such a behavior should be behind an option where the amount of time to wait (before closing and removing the session) is specified ? But this might introduce some complications.

It seems the paused check could cause a license to expire mid stream. Has this been tested? Curious if the player can recover the session after playback is resumed.

To provide further context to what I'm asking here, I'm curious if the player sits either paused or before starting playback for the duration of a license and it expires, will we request a new license and recover upon resuming playback?

Made some changes to the previous logic. I've added an option limitRenewalsOnPauseTime (which probably needs a better name) to define the number of seconds to wait in paused state before rejecting renewals and closing the session.

The player will request new license when playback resumes.

Session is closed when license-renewal is attempted after playback ends. New requests will be made on replay or seek-back

Maybe it should be something like: limitRenewalsMaxPauseDuration

adrums86

Looks good! Just a couple more questions.

src/plugin.js

adrums86 · 2024-07-19T05:28:18Z

src/eme.js

@@ -160,6 +161,10 @@ export const makeNewRequest = (player, requestOptions) => {
          return;
        }

+        if (event.messageType === 'license-renewal' && (!player.hasStarted() || player.paused())) {
+          return;
+        }


It seems the paused check could cause a license to expire mid stream. Has this been tested? Curious if the player can recover the session after playback is resumed.

wseymour15

This change looks great! I left a few comments mainly relating to documentation.

wseymour15 · 2024-08-26T15:58:32Z

src/plugin.js

+      handleEncryptedEvent(player, mockEncryptedEvent, getOptions(player), player.eme.sessions, player.tech_, emeError);
+    };
+
+    if (videojs.browser.IS_FIREFOX) {


Would a comment describing why we are doing this only on Firefox be useful? I guess at first glance it isn't obvious why we are only sending the mock encrypted event in Firefox

Added a comment!

wseymour15 · 2024-08-26T16:00:41Z

src/eme.js

+  let pauseTimer;
+
+  player.on('pause', () => {
+    if (options.limitRenewalsOnPauseTime && typeof options.limitRenewalsOnPauseTime === 'number') {


Do we need to document the limitRenewalsBeforePlay and limitRenewalsOnPauseTime in the README?

Updated README in the latest commit!

wseymour15 · 2024-08-26T16:06:46Z

src/eme.js

@@ -231,29 +276,7 @@ export const makeNewRequest = (player, requestOptions) => {
        if (expired) {
          // Close session and remove it from the session list to ensure that a new
          // session can be created.
-          videojs.log.debug('Session expired, closing the session.');


We removed this debug statement and didn't add it back in closeAndRemoveSession .. should we add something similar?

Good catch! Added it back in closeAndRemoveSession

dzianis-dashkevich · 2024-08-26T16:37:44Z

src/eme.js

+
+          if (!player.hasStarted() && limitRenewalsBeforePlay ||
+            (player.paused() && typeof limitRenewalsOnPauseTime === 'number' && timeElapsed >= limitRenewalsOnPauseTime) ||
+            player.ended()) {


nit: I would split it into several consts eg:
ignoreLinceRenewal (before playback, max pause duration reached, ended)

fix: excessive requests when using HDCP fallback with LDL

9d3ec26

harisha-swaminathan marked this pull request as draft June 30, 2024 18:37

fix test

baa09f2

harisha-swaminathan marked this pull request as ready for review July 2, 2024 03:27

harisha-swaminathan requested a review from adrums86 July 2, 2024 13:58

adrums86 requested changes Jul 11, 2024

View reviewed changes

src/plugin.js Outdated Show resolved Hide resolved

harisha-swaminathan added 2 commits July 16, 2024 01:08

add mock encrypted event

b1e0e2b

do not renew license when player hasn't started or is paused

0e81954

harisha-swaminathan commented Jul 16, 2024

View reviewed changes

src/plugin.js Outdated Show resolved Hide resolved

harisha-swaminathan commented Jul 16, 2024

View reviewed changes

harisha-swaminathan requested a review from adrums86 July 16, 2024 21:46

adrums86 reviewed Jul 19, 2024

View reviewed changes

harisha-swaminathan marked this pull request as draft July 22, 2024 21:22

harisha-swaminathan and others added 9 commits July 22, 2024 17:23

add tests

d488416

Test handleEncryptedEvent gets fired after ended

e99939f

add limitRenewalsOnPause and limitRenewalsbeforePlay

c664833

rename variable

0ea624e

update tests

dd7caf3

skip test

dae6b84

remove tests

8ad96b1

add test1

b6418a9

add test

e774014

harisha-swaminathan marked this pull request as ready for review August 23, 2024 03:54

harisha-swaminathan requested review from Essk and adrums86 August 23, 2024 03:54

add condition

65453bd

wseymour15 reviewed Aug 26, 2024

View reviewed changes

dzianis-dashkevich reviewed Aug 26, 2024

View reviewed changes

review changes

3ba8ce3

harisha-swaminathan requested a review from wseymour15 August 26, 2024 17:54

harisha-swaminathan requested a review from dzianis-dashkevich August 26, 2024 17:54

adrums86 approved these changes Aug 26, 2024

View reviewed changes

wseymour15 approved these changes Aug 26, 2024

View reviewed changes

harisha-swaminathan merged commit 348935e into main Aug 27, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: excessive requests when using HDCP fallback with LDL #225

fix: excessive requests when using HDCP fallback with LDL #225

harisha-swaminathan commented Jun 30, 2024 •

edited

Loading

adrums86 left a comment

harisha-swaminathan Jul 16, 2024

adrums86 Jul 19, 2024

adrums86 Aug 6, 2024

harisha-swaminathan Aug 23, 2024 •

edited

Loading

dzianis-dashkevich Aug 26, 2024

adrums86 left a comment

adrums86 Jul 19, 2024

wseymour15 left a comment

wseymour15 Aug 26, 2024

harisha-swaminathan Aug 26, 2024

wseymour15 Aug 26, 2024

harisha-swaminathan Aug 26, 2024

wseymour15 Aug 26, 2024

harisha-swaminathan Aug 26, 2024

dzianis-dashkevich Aug 26, 2024

harisha-swaminathan Aug 26, 2024

fix: excessive requests when using HDCP fallback with LDL #225

fix: excessive requests when using HDCP fallback with LDL #225

Conversation

harisha-swaminathan commented Jun 30, 2024 • edited Loading

adrums86 left a 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

harisha-swaminathan Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrums86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wseymour15 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harisha-swaminathan commented Jun 30, 2024 •

edited

Loading

harisha-swaminathan Aug 23, 2024 •

edited

Loading