Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: add support for dash manifests describing sidx boxes #455
feat: add support for dash manifests describing sidx boxes #455
Changes from 44 commits
3a9a95d
13c2f0a
5aa5d16
0fd7f87
85049e8
ff40727
67c92cc
eb14f52
1bb3dd4
9aab16d
65b9d0e
02c8595
6723894
6ebf1d2
fafa47e
b5f24dd
8a34323
f7195dc
ecd9699
e3109dc
55e02c2
38c11a3
e320129
c3d39f1
97e37d6
31c8ad8
420fe73
77a8e9f
7c74d4b
e80ad0a
4dd41fd
3d101f8
795f420
089d874
94fb586
859d111
ca459b4
d42cf8a
870c8cc
e2b8754
3bf1d12
7454652
0dafa87
0fe8dbb
4291943
9e00023
a9fc3b0
ef6f39c
60c009e
daa8c0f
e4155c6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
What if one of them has a map and the other does not? Doesn't that make them not equivalent?
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.
yes, the
map
here is the init segment for the playlistThere 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 should the check be
(a.map || b.map)
so that we check if either has a map?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.
I suppose the if condition is unnecessary as the value of
equivalentMap
would be falsy if either a or b did not havemap
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.
equivalentMap already checks that both have .map, so, I'm going to remove the if statement.
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.
Improved
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.
I don't quite understand what this function is filtering. It seems to be merging a new sidx mapping with an old one?
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.
it's removing sidx mappings that have changed (either url or indexRange) from the video and audio playlists
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.
Maybe rename it to
removeChangedMappings
for clarity?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.
I think filter is fine there because it parallels Array#filter. However, a comment can be added to clarify.
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 name a bit as well.
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.
perhaps it would make more sense to more the mediaRequest below this if statement into
if (!playlist.sidx)
and then return inside of it? That would keep similar code together and get rid of a big if scope.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.
Makes sense to me