Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added partial DRM system support. Some typo fixes. #5

Merged
merged 4 commits into from
Nov 30, 2018

Conversation

Heronyme
Copy link
Contributor

@Heronyme Heronyme commented Nov 20, 2018

The partial support includes: key ID, system ID and content protection data handling.

Copy link
Contributor

@sandersaares sandersaares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary of most important points:

  • The term "DRM system" is misapplied, needs correction.
  • Let's implement the entire element, please. It is a very simple element, after all.
  • If we now support this element, we should add it to the test vectors generator, improving some existing examples and adding new ones. Maybe reference the work by Laurent, maybe make up own examples - I have not thought about what makes good examples yet.
  • Code looks nice and clean!

I did not check it from a "did anything not get touched that should have" angle. I guess readme should be updated to match new features.

Cpix/CpixDocument.cs Outdated Show resolved Hide resolved
Cpix/DrmSystem.cs Show resolved Hide resolved
Cpix/DrmSystem.cs Outdated Show resolved Hide resolved
Cpix/DrmSystem.cs Show resolved Hide resolved
Cpix/DrmSystem.cs Outdated Show resolved Hide resolved
Cpix/DrmSystem.cs Outdated Show resolved Hide resolved
Cpix/DrmSystem.cs Outdated Show resolved Hide resolved
Cpix/DrmSystem.cs Show resolved Hide resolved
Cpix/DrmSystemCollection.cs Show resolved Hide resolved
Cpix/DrmSystemCollection.cs Outdated Show resolved Hide resolved
- Also added a unique constraint and a default value related HLSSignalingData element and "playlist" attribute. This way schema validation fails if multiple playlist value are the same (even of both not present).
Copy link
Contributor

@sandersaares sandersaares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lookin' good! Just a couple more comments before perfection.

Cpix/HlsPlaylistType.cs Outdated Show resolved Hide resolved
TestVectorGenerator/Complex.cs Outdated Show resolved Hide resolved
- Automatic PR data signaling generation. Much of code was taken from Makemedia and Axinom.Toolkit projects, with some modifications. This doesn't cover HDS and SS data generation.
- Added Protobuf lib as dependency, for Widevine PSSH data generation.
- "Variant" playlist renamed to "Media" playlist as per HLS spec.
Copy link
Contributor

@sandersaares sandersaares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me except for one super minor nit.

@@ -3,6 +3,6 @@
public class HlsPlaylistType
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason this is not a sealed class? Feels like it should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason. Will seal.

@Heronyme Heronyme merged commit 472efb0 into Axinom:master Nov 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants