Pssh boxes #24
Pssh boxes #24
Conversation
| // !!! Note: this function will accept any wvHeader value !!! | ||
| func (as *AdaptationSet) AddNewContentProtectionSchemeWidevine(wvHeaders ...[]byte) (*WidevineContentProtection, error) { | ||
| if len(wvHeaders) > 1 { | ||
| panic("AddNewContentProtectionSchemeWidevine accepts at most 1 wvHeader slice") |
There was a problem hiding this comment.
Hmm, panic? Why not just return this as error?
There was a problem hiding this comment.
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.
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
| cp.XMLNS = Strptr(CENC_XMLNS) | ||
| prSystemID, err := hex.DecodeString(CONTENT_PROTECTION_PLAYREADY_SCHEME_HEX) | ||
| if err != nil { | ||
| panic(err.Error()) |
There was a problem hiding this comment.
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.
Objection noted.
|
LGTM |
<cenc:pssh>elements within Playready and Widevine ContentProtection schemes