live stream can switch from clear content to encrypted content #1094 #1217

SemihGk · 2018-01-06T23:18:20Z

As you may read more details in #1094 , this PR fixes the issue that occurs when live stream switches from unencrypted content to encrypted content during playback.This issue can be fixed by drm configuration setup as you may find it in this PR. So, I tried to keep simple as possible as I can although there are a couple more ways to implement a fix that is based on drm configuration, but those ways require little bit more changes which I did not want it. I will also explain some minor drawbacks under the code changes with this fix. Also, a note that I could not add an integration test because I could not find any public sample live stream that switches from clear content to encrypted content.

On the other hand, I have considered to reinitialize drmEngine when player receives encrypted content if drmEngine is not activated. Although this seems more proper fix, I could not get it work. You may see this workaround in this link drmReinitialization . I just left the major changes in this branch comparization to just show how it can be handled. However, browser is crashed when live stream switches to encrypted content. Therefore, I cannot receive any error or cannot troubleshoot the issue although I debugged the all player lifecycle. Anyway, if you consider this solution rather than drmConfiguration based fix, I may need more help on this since I have no idea what is wrong. This bug became very urgent. Thus, I would be grateful if you can help me to fix this issue soon. Thank you.

googlebot · 2018-01-06T23:18:22Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

SemihGk · 2018-01-06T23:27:27Z

lib/media/drm_engine.js

+      // drm configuration overrides DrmInfo so drmEngine can be activated.
+      // Thus, player can play encrypted content if live stream switches from
+      // unencrypted content to encrypted content during live stream.
+      if (!!configDrmInfos) {


Only if manifest contains clear content, configDrmInfos won't be null. The drawback is that adds drmInfos to each variant although those do not break a playback. I considered to update configsByKeySystem directly instead of modifying manifest, but this way would cause a duplicated code issue. Also, keys are changed for chromecast in this code block. So, I preferred more simple way to add this fix.

SemihGk · 2018-01-06T23:55:34Z

lib/media/drm_engine.js

+      // Thus, player can play encrypted content if live stream switches from
+      // unencrypted content to encrypted content during live stream.
+      if (!!configDrmInfos) {
+        variant.drmInfos = configDrmInfos;


This drmInfos is going to be generated with default drmInfos. When stream switches to encrypted content, encrypted content might have a different configured drmInfos. At this situation, I do not know if there is any risk that having different drm info settings may break the playback. As for my tests in our live streams, I could not reproduce any issue.

SemihGk · 2018-01-07T00:30:52Z

lib/media/drm_engine.js

+shaka.media.DrmEngine.prototype.getDrmInfosByConfig_ =
+    function(manifest) {
+  var config = this.config_;
+  if (!config) {


@joeyparrish suggested me a way that adding a propery to drm config may be a better way to understand this expectation. I did not add this property in this PR just in case you may be interested the "reinitialization drm" solution. If you do not, I will update this PR with a new config property that allows this method.

You shouldn't need to deal with a null config_, since that should only be true before init() and after destroy().

TheModMaker · 2018-01-12T00:24:43Z

Why not just change how the manifest.dash.ignoreDrmInfo configuration works? Right now it only activates if there is existing ContentProtection elements. But you could change it to always assume encrypted content. Couldn't you just do:

diff --git a/lib/dash/content_protection.js b/lib/dash/content_protection.js
index 5a3c4440c..054e8b99e 100644
--- a/lib/dash/content_protection.js
+++ b/lib/dash/content_protection.js
@@ -170,7 +170,7 @@ shaka.dash.ContentProtection.parseFromAdaptationSet = function(
 
   // If there are only CENC element(s) or ignoreDrmInfo flag is set, assume all
   // key-systems are supported.
-  if (parsed.length && (ignoreDrmInfo || !parsedNonCenc.length)) {
+  if (ignoreDrmInfo || (parsed.length && !parsedNonCenc.length)) {
     var keySystems = ContentProtection.defaultKeySystems_;
     drmInfos =
         MapUtils.values(keySystems)

This has the disadvantage of requiring DRM initialization on clear content, but at least it is behind a flag.

SemihGk · 2018-01-12T16:51:21Z

Thank you for your reply, @TheModMaker . It is actually pretty good idea. I should have noticed that. However, this approach is failed after played encrypted content for a couple min with error code 4011(UNPLAYABLE_PERIOD). Also, it throws errors at https://github.com/google/shaka-player/blob/master/lib/media/drm_engine.js#L688 if we did not configure for primetime and clearkey although this error is ignorable. Let me test more and I will try to find another way that is based on this approach. Thanks again.

joeyparrish

This is great! Thanks for your contribution. Just a few things to clean up.

joeyparrish · 2018-01-16T23:07:37Z

lib/media/drm_engine.js

+ * Returns the DrmInfo that is generated by drm configation.
+ * It activates DrmEngine if drm configs have keySystems.
+ * @param {!shakaExtern.Manifest} manifest
+ * @return {(Array.<{object}>|null)}


This should be: * @return {Array.<shakaExtern.DrmInfo>}

I think <{object}> is bad syntax. I think the {} in that context look like a record type. And you don't need the alternation with null (|null) because Arrays are nullable by default. (To make it non-nullable, add a ! to the front.) Finally, the more specific type shakaExtern.DrmInfo should be used, because Object is too loose.

joeyparrish · 2018-01-16T23:08:09Z

lib/media/drm_engine.js

+ * @private
+ */
+shaka.media.DrmEngine.prototype.getDrmInfosByConfig_ =
+    function(manifest) {


style nit: It looks like this should fit on the previous line

joeyparrish · 2018-01-16T23:09:11Z

lib/media/drm_engine.js

+shaka.media.DrmEngine.prototype.getDrmInfosByConfig_ =
+    function(manifest) {
+  var config = this.config_;
+  if (!config) {


You shouldn't need to deal with a null config_, since that should only be true before init() and after destroy().

joeyparrish · 2018-01-16T23:10:09Z

lib/media/drm_engine.js

+    return null;
+  }
+
+  var serverKeys = Object.keys(config.servers || {});


You don't need to worry about null config.servers, either, since Player maintains a complete config object at all times.

joeyparrish · 2018-01-16T23:23:38Z

lib/media/drm_engine.js

+    return null;
+  }
+
+  var isEncryptedContent = manifest.periods.every(function(period) {


I don't think every() is what we want here. If any period has encrypted variants, the content is encrypted. This should be some().

joeyparrish · 2018-01-16T23:24:30Z

lib/media/drm_engine.js

+  }
+
+  var isEncryptedContent = manifest.periods.every(function(period) {
+    period.variants.every(function(variant) {


Same here. I believe this should be some(), as well. Also, you forgot a return here, so the outermost every() function has no return value.

Thank you for reviewing, @joeyparrish . Last min I reversed this behavior from unencrypted to encrypted. I should have noticed this mistake. Apologies. On the other hand, it seems @TheModMaker 's suggestion more simple than mine, but it throws an error. I was going to trace that error, but I was interrupted by something else and I could not spend enough time. Do you think it is not necessary to chase his solution? Thanks again.

It is simpler, but I think we should keep ignoreDrmInfo separate. It serves a different purpose, and it isn't on by default. What you are changing here should, in my opinion, be default behavior to match developer expectations.

joeyparrish · 2018-01-16T23:24:55Z

lib/media/drm_engine.js

+
+  var isEncryptedContent = manifest.periods.every(function(period) {
+    period.variants.every(function(variant) {
+      return !!variant.drmInfos.length;


No need to do !!, since length is a number which will implicitly convert to boolean.

joeyparrish · 2018-01-16T23:25:42Z

lib/media/drm_engine.js

+    });
+  });
+
+  // Only creates for clear contents


This comment seems a little bit vague. How about this one:

// We only want to create phony DrmInfos when none are provided by the manifest.

joeyparrish · 2018-01-16T23:27:27Z

lib/media/drm_engine.js

+
+  return serverKeys.map(function(keySystem) {
+    return {
+      // ignoring other properties


With the recommended change to return Array.<shakaExtern.DrmInfo>, the compiler will require you to provide all fields here. Use the same values returned from the bottom of configureClearKey_().

joeyparrish · 2018-01-16T23:27:45Z

test/media/drm_engine_unit.js

      drmEngine.init(manifest, /* offline */ false).then(function() {
        expect(drmEngine.initialized()).toBe(true);
        expect(drmEngine.keySystem()).toBe('');
        expect(requestMediaKeySystemAccessSpy.calls.count()).toBe(0);
      }).catch(fail).then(done);
    });

+    it('makes queries for clear content if key is configured', function(done) {


Awesome! Thank you for writing tests!

…thub.com/SemihGk/shaka-player into feature/switchingEncryptedToClearContent

…EncryptedToClearContent

…thub.com/SemihGk/shaka-player into feature/switchingEncryptedToClearContent

joeyparrish

This all looks good to me. I'll have our build bot test it now.

shaka-bot · 2018-01-17T21:32:36Z

All tests passed!

joeyparrish · 2018-01-17T21:44:56Z

Okay, looks good! Thank you for contributing!

joeyparrish · 2018-01-17T22:47:27Z

Looks like this is breaking something in our demo app. Load any custom, clear manifest URI, and it fails with DRM.NO_LICENSE_SERVER_GIVEN. I'm looking into a fix.

SemihGk · 2018-01-17T23:33:22Z

@joeyparrish Thanks for quick merge. This was a priority for us. I am sorry about that. I did not test the demo app, I tested our contents only. Anyway, I just found the issue. It seems server keys are assigned, but licenseServerUri are empty in drm config. I can fix it by adding this to my method.

if (!config.servers[keySystem]) return null; // in a loop

should not allow to return phony drmInfos. What do you think?

joeyparrish · 2018-01-17T23:35:38Z

We just fixed it in the demo app, by having the demo avoid filling the servers dictionary with empty strings. It should be pushed to github shortly.

PR #1217 broke the demo app for custom clear content. It caused DRM errors by setting blank license server URIs in the config for custom assets. The fix is not to set license servers if the URI field is blank. Change-Id: Ia11ff5f49d246eeb5f17a18bd5ca4a4670a7e106

joeyparrish · 2018-01-17T23:37:18Z

The fix is out now. In future, we would like to write automated tests with WebDriver that make sure the demo app is working correctly. For now, we're still doing manual QA on the demo itself. It's lucky that I happened to use the demo this afternoon with a custom asset. That might have gone unnoticed for some time.

SemihGk · 2018-01-17T23:42:39Z

Yeah, it sounds great. However, this might happen to other users as well if they are not aware that they should not configure drm keys even though keys are empty. Maybe, we should add a warning to documentation?

Yeah, selenium based test cases might be very useful. I would love to contribute it if you decide it to add. Thanks again.

In some cases with dynamic, multi-period DASH manifests and insertion of clear ad periods, it is not known from the manifest if the content will be encrypted. From now on, if an application configures license servers, DRM content will be implied and MediaKeys will be set up, regardless of what is in the manifest. Closes #1094

PR #1217 broke the demo app for custom clear content. It caused DRM errors by setting blank license server URIs in the config for custom assets. The fix is not to set license servers if the URI field is blank. Change-Id: Ia11ff5f49d246eeb5f17a18bd5ca4a4670a7e106

joeyparrish · 2018-01-19T22:27:25Z

Cherry-picked to v2.3.1.

SemihGk and others added 3 commits January 6, 2018 17:19

live stream can switch from clear content to encrypted content

809444f

live stream can switch from clear content to encrypted content

e69bca7

live stream can switch from clear content to encrypted content

e6f6f72

SemihGk commented Jan 6, 2018

View reviewed changes

SemihGk and others added 6 commits January 6, 2018 18:38

cla check update

bcc217f

cla check update

66780a6

cla check update

71df416

email update in the commits

cac93de

email update in the commits

40339d7

email update in the commits

b4cc59c

SemihGk commented Jan 6, 2018

View reviewed changes

SemihGk commented Jan 7, 2018

View reviewed changes

TheModMaker added the needs triage label Jan 12, 2018

joeyparrish requested changes Jan 16, 2018

View reviewed changes

joeyparrish self-assigned this Jan 16, 2018

joeyparrish added type: enhancement New feature or request and removed needs triage labels Jan 16, 2018

joeyparrish added this to the v2.4.0 milestone Jan 16, 2018

SemihGk added 8 commits January 17, 2018 10:36

Merge branch 'feature/switchingEncryptedToClearContent' of https://gi…

6ecf267

…thub.com/SemihGk/shaka-player into feature/switchingEncryptedToClearContent

Merge branch 'feature/switchingEncryptedToClearContent' of https://gi…

9f95fbb

…thub.com/SemihGk/shaka-player into feature/switchingEncryptedToClearContent

implement requested changes of PR

3d44c36

implement requested changes of PR

fefc493

fix config variable

db39469

fix config variable

e12cf4b

Merge remote-tracking branch 'upstream/master' into feature/switching…

fc8f6e2

…EncryptedToClearContent

Merge branch 'feature/switchingEncryptedToClearContent' of https://gi…

e729711

…thub.com/SemihGk/shaka-player into feature/switchingEncryptedToClearContent

joeyparrish approved these changes Jan 17, 2018

View reviewed changes

joeyparrish merged commit c69ac32 into shaka-project:master Jan 17, 2018

github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

live stream can switch from clear content to encrypted content #1094 #1217

live stream can switch from clear content to encrypted content #1094 #1217

SemihGk commented Jan 6, 2018 •

edited

Loading

googlebot commented Jan 6, 2018

SemihGk Jan 6, 2018 •

edited

Loading

SemihGk Jan 6, 2018 •

edited

Loading

SemihGk Jan 7, 2018

joeyparrish Jan 16, 2018

TheModMaker commented Jan 12, 2018

SemihGk commented Jan 12, 2018 •

edited

Loading

joeyparrish left a comment

joeyparrish Jan 16, 2018

joeyparrish Jan 16, 2018

joeyparrish Jan 16, 2018

joeyparrish Jan 16, 2018

joeyparrish Jan 16, 2018

joeyparrish Jan 16, 2018

SemihGk Jan 17, 2018

joeyparrish Jan 17, 2018

joeyparrish Jan 16, 2018

joeyparrish Jan 16, 2018

joeyparrish Jan 16, 2018

joeyparrish Jan 16, 2018

joeyparrish left a comment

shaka-bot commented Jan 17, 2018

joeyparrish commented Jan 17, 2018

joeyparrish commented Jan 17, 2018

SemihGk commented Jan 17, 2018 •

edited

Loading

joeyparrish commented Jan 17, 2018

joeyparrish commented Jan 17, 2018

SemihGk commented Jan 17, 2018

joeyparrish commented Jan 19, 2018

live stream can switch from clear content to encrypted content #1094 #1217

live stream can switch from clear content to encrypted content #1094 #1217

Conversation

SemihGk commented Jan 6, 2018 • edited Loading

googlebot commented Jan 6, 2018

SemihGk Jan 6, 2018 • edited Loading

Choose a reason for hiding this comment

SemihGk Jan 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheModMaker commented Jan 12, 2018

SemihGk commented Jan 12, 2018 • edited Loading

joeyparrish 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

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

joeyparrish left a comment

Choose a reason for hiding this comment

shaka-bot commented Jan 17, 2018

joeyparrish commented Jan 17, 2018

joeyparrish commented Jan 17, 2018

SemihGk commented Jan 17, 2018 • edited Loading

joeyparrish commented Jan 17, 2018

joeyparrish commented Jan 17, 2018

SemihGk commented Jan 17, 2018

joeyparrish commented Jan 19, 2018

SemihGk commented Jan 6, 2018 •

edited

Loading

SemihGk Jan 6, 2018 •

edited

Loading

SemihGk Jan 6, 2018 •

edited

Loading

SemihGk commented Jan 12, 2018 •

edited

Loading

SemihGk commented Jan 17, 2018 •

edited

Loading