-
Notifications
You must be signed in to change notification settings - Fork 68
Add SCTE-35 support #106
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
Add SCTE-35 support #106
Conversation
🎉 Snyk checks have passed. No issues have been found so far.
✅ security/snyk check is complete. No issues have been found. (View Details)
✅ license/snyk check is complete. No issues have been found. (View Details)
✅ code/snyk check is complete. No issues have been found. (View Details)
mpd/scte35_test.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.
Rather than test this here, you should have a hardcoded mpd in in the fixtures/ folder, and compare the actualString to the fixture itself much like the event stream tests https://github.com/zencoder/go-dash/blob/master/mpd/events_test.go#L55
The way you have this assertion implemented can actually hide bugs. If there was a bug in say the xml tags you added such that the xml encoded value of the struct is invalid, you would never catch it here because both the expected and actual are generated by encoding a struct to xml string, both would encode with the same bug. Comparing against a static fixture can catch any encoding bugs.
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.
Updated the test to load the fixtures/scte35.mpd file as the expected mpd, and compared it to the generated one here: 3754381
mpd/fixtures/scte35.mpd
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.
is it expected to still have a <Binary></Binary> tag if there is no data within it, or should we expect this to be omitted? not sure if it is required by the spec or to satisfy precondition requirements?
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.
Good point, updated the library, to only keep the Event field, as that contains the presentationTime for an event whenever the binary is empty (also updated a bunch of other stuff to better support scte-35).
Description
This pr adds support for SCTE-35 on the go-dash library.