Add support for skip_encryption stream descriptor #219

sylt · 2017-03-24T14:36:13Z

This option makes it possible to control encryption for specific streams, i.e. for audio and video separately.

In my set-up, I want to have the ability to control which tracks gets to be encrypted or not (in my case, I wanted the sound to be in clear). I though that others perhaps wanted this capability as well, hence the pull request.

It's possible though that the maintainers think otherwise :) Let's have a try.

kqyang

Thanks for working on this. Look goods to me. I just have one comment.

kqyang · 2017-03-24T23:31:08Z

packager/app/packager_main.cc

-    "    Name of the playlist for the stream. Usually ends with '.m3u8'.\n";
+    "    Name of the playlist for the stream. Usually ends with '.m3u8'.\n"
+    "  - skip_encryption: Optional value which specifies if encryption of\n"
+    "    the stream shall be skipped. By default, if encryption is enabled,\n"


Make it clear that it expects a numeric value instead of "true|false", e.g.

skip_encryption: Optional, if set to nonzero, encryption of the stream shall be skipped. ...

Yes, good point! I'll have a go at fixing it.

I'm actually considering if one should allow setting skip_encryption=true, since it perhaps reads a bit better. Let me know if you have any particular preference here!

I also have to mention: Thank you for the friendly and good feedback I've gotten from you during the last few days! :) I personally find it very encouraging!

I am fine with both. I am slightly leaning towards 0|1 for consistency since we also use 0|1 in udp options.

I also have to mention: Thank you for the friendly and good feedback I've gotten from you during the last few days! :) I personally find it very encouraging!

No problem. Thanks for your work on improving shaka-packager :-)

kqyang · 2017-05-18T23:27:48Z

@sylt Are you still working on this? Are you still interested in completing this pull request? Thanks.

sylt · 2017-05-19T08:37:29Z

Thanks for the reminder! I must have done something wrong or misunderstood something, but I did update the pull-request with your suggestions in March (as in, the diff looked up-to-date when I checked this PR). I thought that github would send some sort of notification of this being done, but maybe that's not the case. Sigh, I need to read up on this :)

Anyway, I have rebased my change-set, so the pull-request should be up-to-date now (with your suggestions incorporated).

kqyang

Github won't send out a notification on updated commits automatically. You'll need to post a message - typically reply on review comments :)

A few more nit comments.

kqyang · 2017-05-19T17:08:06Z

packager/app/test/packager_test.py

@@ -593,6 +593,16 @@ def testPackageWithLiveProfileAndKeyRotationAndNonDashIfIop(self):
        self.mpd_output,
        'bear-640x360-av-live-cenc-rotation-non-iop-golden.mpd')

+  def testPackageWithEncryptionOfOnlyVideoStream(self):
+    self.packager.Package(
+        self._GetStreams(['audio', 'video'],


Prefer something like

self.packager.Package(self._GetStreams(['audio,skip_encryption=1','video']), ....)

It is more flexible and probably clearer as well.

The changes in _GetStreams is not needed.

kqyang · 2017-05-19T17:10:24Z

packager/app/test/packager_test.py

+        self._GetFlags(encryption=True))
+    self._DiffGold(self.output[0], 'bear-640x360-a-golden.mp4')
+    self._DiffGold(self.output[1], 'bear-640x360-v-cenc-golden.mp4')
+    self._DiffGold(self.mpd_output, 'bear-640x360-a-clear-v-venc-golden.mpd')


s/venc/cenc/

kqyang · 2017-05-19T17:11:51Z

packager/app/test/packager_test.py

@@ -593,6 +593,16 @@ def testPackageWithLiveProfileAndKeyRotationAndNonDashIfIop(self):
        self.mpd_output,
        'bear-640x360-av-live-cenc-rotation-non-iop-golden.mpd')

+  def testPackageWithEncryptionOfOnlyVideoStream(self):


Can you move this function after line 242, i.e. after testPackageWithEncryption?

kqyang · 2017-05-19T17:18:11Z

packager/app/packager_main.cc

@@ -107,7 +107,10 @@ const char kUsage[] =
    "    The group ID for the output stream. For HLS this is used as the\n"
    "    GROUP-ID attribute for EXT-X-MEDIA.\n"
    "  - playlist_name: Required for HLS output.\n"
-    "    Name of the playlist for the stream. Usually ends with '.m3u8'.\n";
+    "    Name of the playlist for the stream. Usually ends with '.m3u8'.\n"
+    "  - skip_encryption: Optional. If this is set to a non-zero value,\n"


This description does not match what is implemented, rephrase it as:

skip_encryption=0|1: Optional. Default to 0 if not specified. If it is set to 1, ...

This option makes it possible to control encryption for specific streams, i.e. for audio and video separately.

sylt · 2017-05-19T20:34:16Z

All comments accepted/handled! Much better :) Pull request has been updated. Let's see!

kqyang · 2017-05-19T20:48:30Z

@sylt LGTM. Thanks for coming up with the idea and working on this! Your contribution is appreciated!

kqyang · 2017-05-19T20:49:46Z

Will merge once continuous integration tests pass.

kqyang reviewed Mar 24, 2017

View reviewed changes

sylt force-pushed the skip_encryption_per_stream branch from f05557a to 76d22fd Compare March 27, 2017 08:17

sylt force-pushed the skip_encryption_per_stream branch from 76d22fd to d4b3fac Compare May 19, 2017 08:21

sylt force-pushed the skip_encryption_per_stream branch from d4b3fac to d3472f0 Compare May 19, 2017 09:27

kqyang reviewed May 19, 2017

View reviewed changes

sylt force-pushed the skip_encryption_per_stream branch from d3472f0 to 4418b6c Compare May 19, 2017 19:15

Add support for skip_encryption stream descriptor

dbbf600

This option makes it possible to control encryption for specific streams, i.e. for audio and video separately.

sylt force-pushed the skip_encryption_per_stream branch from 4418b6c to dbbf600 Compare May 19, 2017 19:50

kqyang merged commit bfe302b into shaka-project:master May 22, 2017

sylt deleted the skip_encryption_per_stream branch May 22, 2017 18:08

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for skip_encryption stream descriptor #219

Add support for skip_encryption stream descriptor #219

sylt commented Mar 24, 2017

kqyang left a comment

kqyang Mar 24, 2017

sylt Mar 25, 2017

kqyang Mar 28, 2017

kqyang commented May 18, 2017

sylt commented May 19, 2017

kqyang left a comment

kqyang May 19, 2017

kqyang May 19, 2017

kqyang May 19, 2017

kqyang May 19, 2017

sylt commented May 19, 2017

kqyang commented May 19, 2017

kqyang commented May 19, 2017

Add support for skip_encryption stream descriptor #219

Add support for skip_encryption stream descriptor #219

Conversation

sylt commented Mar 24, 2017

kqyang left a comment

Choose a reason for hiding this comment

kqyang Mar 24, 2017

Choose a reason for hiding this comment

sylt Mar 25, 2017

Choose a reason for hiding this comment

kqyang Mar 28, 2017

Choose a reason for hiding this comment

kqyang commented May 18, 2017

sylt commented May 19, 2017

kqyang left a comment

Choose a reason for hiding this comment

kqyang May 19, 2017

Choose a reason for hiding this comment

kqyang May 19, 2017

Choose a reason for hiding this comment

kqyang May 19, 2017

Choose a reason for hiding this comment

kqyang May 19, 2017

Choose a reason for hiding this comment

sylt commented May 19, 2017

kqyang commented May 19, 2017

kqyang commented May 19, 2017