Support for offline data with an online license request #878

gscragg · 2017-06-13T21:54:57Z

Carrying on from the discussion in Issue #873

Currently only ChromeOS truly supports offline storage and playback.

This change allows other browsers to support offline storage of media only. A remote/temporary license request will be made during playback, so the user must still be online.

This will help users to pre-download content before traveling to a location with a patchy/expensive internet connection.

… license

googlebot · 2017-06-13T21:55:00Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
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.

joeyparrish

Looks okay otherwise.

joeyparrish · 2017-06-13T22:07:54Z

lib/offline/storage.js

@@ -298,6 +298,9 @@ shaka.offline.Storage.prototype.remove = function(content) {
        drmEngine = new shaka.media.DrmEngine(
            netEngine, onError, function() {}, function() {});
        drmEngine.configure(this.player_.getConfiguration().drm);
+        // if the license is not stored, we dont need to enlist drmEngine in removing the license
+        if (!this.config_.isPersistentLicense) return Promise.resolve();


Current config should not affect the remove() operation. If the content you're removing has persistent licenses, you should still call drmEngine.init() and do the rest.

In particular, if there is no persistent license stored, sessionIds will be empty and drmEngine.removeSessions() will do nothing. So I think this check can be removed.

joeyparrish · 2017-06-13T22:09:03Z

lib/offline/storage.js

@@ -378,13 +381,14 @@ shaka.offline.Storage.prototype.list = function() {
 * @param {string} manifestUri
 * @param {function(*)} onError
 * @param {!shakaExtern.ManifestParser.Factory=} opt_manifestParserFactory
+ * @param {!boolean=} opt_storeDataOnly


This parameter appears to be unused.

googlebot · 2017-06-13T22:22:15Z

CLAs look good, thanks!

joeyparrish

Looks good to me! Can you also update the tests?

See test/offline/*.js and run python build/test.py to run the tests locally.

gscragg · 2017-06-13T22:27:29Z

Working on tests now. I appreciate the quick feedback.

gscragg · 2017-06-14T01:16:34Z

Added some tests in.
I had a go at testing whether the underlying DrmEngine inside Storage#store was getting initialized correctly but I couldn't figure it out.

joeyparrish · 2017-06-19T19:23:17Z

test/offline/offline_integration.js

@@ -157,4 +157,45 @@ describe('Offline', function() {
        .catch(fail)
        .then(done);
  });
+
+  it('stores, plays, and deletes protected content with a temporary license', function(done) {


Please change these it() calls to drm_it(). We only run DRM tests with the --drm flag, and drm_it() does the filtering for us.

We do it this way so that tests can still be run without solid network connectivity. --drm enables DRM tests (some networking required for licensing) and --external enables external asset tests (solid network required to pull content from the internet).

Fixed. Thanks.

joeyparrish · 2017-06-19T19:27:58Z

test/offline/storage_unit.js

+    describe('temporary license', function(){
+
+      beforeEach(function(){
+        storage.configure({ isPersistentLicense: false });


This doesn't configure DRM, so the tests below are all exercising clear content paths. See the drmEngine setup in the 'stores DRM info' test and do the same here in beforeEach.

joeyparrish · 2017-06-19T19:28:58Z

test/offline/storage_unit.js

+            })
+            .then(function(manifestDb) {
+              expect(manifestDb).toBeTruthy();
+              expect(manifestDb.sessionIds.length).toEqual(0);


Also verify that manifestDb.drmInfo exists and matches the one you create in beforeEach, or else this is passing for the wrong reasons (clear content).

joeyparrish · 2017-06-19T19:29:30Z

test/offline/storage_unit.js

+              expect(data.duration).toBe(0);  // There are no segments.
+              expect(data.size).toEqual(0);
+              expect(data.tracks).toEqual(tracks);
+            })


You should also load the content again and verify that is has drmInfo (see below) so that you know you aren't just testing clear content.

I have added a new 'stores drm info' test (along with the additions to beforeEach) to check the drmInfo.

Is this what you meant?

Kind of. I was thinking of putting checks for drmInfo into the offline sessions test below. Otherwise, running that test in isolation could result in a pass even if it was clear content.

Since the feature under test is that we do temporary licenses instead of persistent licenses, I think all three of these could be one test. In my opinion, the test above ("stores basic manifest") is irrelevant, and these other two should be combined into "does not store offline sessions").

Ok, that makes sense. As this creates a describe with a single it in it, would you like that to be flattened into a standalone it?

Either way.

Dropped down to the single test inside the describe group.

joeyparrish

Looks good to me! Thanks!

shaka-bot · 2017-06-21T17:16:43Z

Test Failure:

Generating Closure dependencies...
Running Closure linter...
----- FILE  :  /var/lib/jenkins/jobs/Manual PR Test (local-tests)/workspace/lib/offline/storage.js -----
Line 301, E:0110: Line too long (90 characters).
Line 425, E:0110: Line too long (90 characters).
----- FILE  :  /var/lib/jenkins/jobs/Manual PR Test (local-tests)/workspace/test/offline/offline_integration.js -----
Line 161, E:0110: Line too long (98 characters).
Line 201, E:0010: Missing semicolon at end of line
----- FILE  :  /var/lib/jenkins/jobs/Manual PR Test (local-tests)/workspace/test/offline/storage_unit.js -----
Line 873, E:0002: Missing space before "{"
Line 876, E:0002: Missing space before "{"
Line 998, E:0002: Missing space before "{"
Found 7 errors, including 1 new errors, in 3 files (183 files OK).
Build step 'Execute shell' marked build as failure

joeyparrish

The build bot found some linter errors. Run python build/all.py to see this locally.

joeyparrish

Looks good to me. I'll have the build bot take another look.

shaka-bot · 2017-06-21T21:39:15Z

All tests passed!

The new isPersistentLicense option was not documented in #878. This adds docs and renames it to usePersistentLicense. Closes #873 Change-Id: I59ed32c98660ad7c155392a1708eab98f63131c3

gscragg added 4 commits June 14, 2017 08:40

add support for only storing data for offline viewing

a232333

improve code based on closure errors

58b6508

flip isDataOnly logic for easier reading

27939d8

use a config approach for specifying if offline data has a persistent…

417c390

… license

joeyparrish requested changes Jun 13, 2017

View reviewed changes

gscragg added 2 commits June 14, 2017 10:15

removed unused param

123c1ce

use config flag for isOffline param when initing drmEngine

46a0ed7

joeyparrish requested changes Jun 13, 2017

View reviewed changes

gscragg added 4 commits June 14, 2017 11:07

integration test for offline content with a temporary license

b378ef1

integration tests for storing offline data with a temporary license

6e8a467

add tests for storing temporary licensed content

b393ab0

ensure removal of temporary license content works

b27de0a

joeyparrish requested changes Jun 19, 2017

View reviewed changes

gscragg added 3 commits June 20, 2017 14:03

use drm_it for temp license offline integration test

b3fad61

make sure storage tests are running on drm content

d72445e

removing superfluous offline tests

9f6201c

joeyparrish approved these changes Jun 21, 2017

View reviewed changes

joeyparrish requested changes Jun 21, 2017

View reviewed changes

gscragg added 3 commits June 22, 2017 08:32

fix linting errors

626be94

fix lint error in offline integration test and confirm content deleted

917d178

space before {

8086e41

joeyparrish approved these changes Jun 21, 2017

View reviewed changes

joeyparrish merged commit 59fe4e5 into shaka-project:master Jun 21, 2017

joeyparrish mentioned this pull request Jun 21, 2017

Tizen 2017 misreports support for persistent licenses #894

Closed

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

Support for offline data with an online license request #878

Support for offline data with an online license request #878

gscragg commented Jun 13, 2017

googlebot commented Jun 13, 2017

joeyparrish left a comment

joeyparrish Jun 13, 2017

joeyparrish Jun 13, 2017

googlebot commented Jun 13, 2017

joeyparrish left a comment

gscragg commented Jun 13, 2017

gscragg commented Jun 14, 2017

joeyparrish Jun 19, 2017

gscragg Jun 20, 2017

joeyparrish Jun 19, 2017

joeyparrish Jun 19, 2017

joeyparrish Jun 19, 2017

gscragg Jun 20, 2017

joeyparrish Jun 20, 2017

gscragg Jun 20, 2017

joeyparrish Jun 20, 2017

gscragg Jun 21, 2017

joeyparrish left a comment

shaka-bot commented Jun 21, 2017

joeyparrish left a comment

joeyparrish left a comment

shaka-bot commented Jun 21, 2017

Support for offline data with an online license request #878

Support for offline data with an online license request #878

Conversation

gscragg commented Jun 13, 2017

googlebot commented Jun 13, 2017

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

googlebot commented Jun 13, 2017

joeyparrish left a comment

Choose a reason for hiding this comment

gscragg commented Jun 13, 2017

gscragg commented Jun 14, 2017

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 Jun 21, 2017

joeyparrish left a comment

Choose a reason for hiding this comment

joeyparrish left a comment

Choose a reason for hiding this comment

shaka-bot commented Jun 21, 2017