HLS-21: Review Encrypt function

Issue Information

Issue Type: Task
Priority: Major
Status: Closed

Reported By:
Ben Tasker
Assigned To:
Ben Tasker
Project: HLS Stream Creator (HLS)
Resolution: Done (2017-04-29 12:16:43)
Components: Encryption ,

Created: 2017-04-27 09:06:25
Time Spent Working
120 minutes
120 minutes
0 minutes

The ability to encrypt segments was introduced as a pull request in HLS-17

At the time I did a fairly cursory code review - scanned the patch, and ran against a video to check it worked. HLS-20 shows that this probably wasn't enough, so I want to take a close look at that function (including the HLS-20 changes) and make sure there aren't any other issues hiding.

Issue Links

Toggle State Changes


I've a feeling the HLS-20 changes still aren't quite up to scratch for adaptive bitrate streams. The for loop ( https://github.com/bentasker/HLS-Stream-Creator/blob/42de7b6cd9a344d6e57d2544f1b18376aaef7bc1/HLS-Stream-Creator.sh#L226 ) is written on the basis of there only being one bitrate, and so generates an IV based upon the segment number
    for file in ${OUTPUT_DIRECTORY}/*.ts

	INIT_VECTOR=$(printf '%032x' $count)

If we've got an ABR stream with 3 bitrates, containing 5 segments each, then when we move onto the second bitrate count is going to be at 6 (whilst we'll expect it to be 0).

So the function almost certainly needs adapting to take bitrates into account rather than simply iterating over files.

Apple's technote ( https://developer.apple.com/library/content/technotes/tn2288/_index.html ) specifies that

The default Initialization Vector for media encryption (if none is specified) is the sequence number of the media file. You should specify an Initialization Vector value, and not rely on sequence numbers. The main reason for this is portability. For example, if you change where the segment appears in the playlist (e.g. inserting an ad), that changes its sequence number, requiring a re-encrypt.

So this is almost certainly going to be an issue. Longer term, it'd be better to follow their advice and set a specific IV (which will mean rewriting the manifest), but in the short term need to make the sequence numbers work as a minimum

Raised as HLS-22

Repo: HLS-Stream-Creator
Commit: 40dc4fca5c0364ea9887ebd3b70ac7be3b7aaf04
Author: B Tasker <github@<Domain Hidden>>

Date: Sat Apr 29 12:10:23 2017 +0100
Commit Message: Switch variable name to something that isn't a command, as it was bugging me. HLS-21

Modified (-)(+)

Webhook User-Agent


View Commit

I can't see any other obvious issues with the function (though I've almost certainly missed something).

At some point the way the manifests are updated will almost certainly change (particularly if we move away from the IV being based upon the segment number), but other than that it now looks OK, and handles ABR streams correctly.
btasker changed status from 'Open' to 'Resolved'
btasker added 'Done' to resolution
btasker changed status from 'Resolved' to 'Closed'