-
Couldn't load subscription status.
- Fork 68
Pssh boxes #24
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
Pssh boxes #24
Conversation
mpd/mpd.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, panic? Why not just return this as error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why call the variable wvHeaders if it can only hold 1 byte slice? The comment above even calls it wvHeader. Let's consider using that name or changing the function signature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For backwards compatibility. It makes wvHeaders optional, if more than 1 argument is passed to the function it's more of a syntax error, rather than something that can/should be handled by the applicaiton.
...th and without PSSH box
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I'm okay with this in this situation due to the author's vehement defense of this panic and the peculiarity of this code path.
However, as a general rule of thumb, avoid panics. I probably would not accept this code as is in a future PR without a more compelling special case explanation of why a consumer of this library would want a panic in the critical path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Objection noted.
LGTM
<cenc:pssh>elements within Playready and Widevine ContentProtection schemes