-
Notifications
You must be signed in to change notification settings - Fork 54
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 support for mpd parsing #2
Conversation
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.
Given this is a midway, I'm assuming you haven't gotten to it (mostly because things will change) but it would be neat to get file level and function level comments in.
src/fns.js
Outdated
@@ -0,0 +1,52 @@ | |||
export const shallowMerge = (...objects) => { |
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.
This file is small now but it might grow in the future as we add more features. It might be good to split this file up a bit more (either now or in a future PR). fns.js is a bit generic and I think we could make these files a bit more specific.
You could create a util folder, then make files like object.js (for shallow merge, flatten, range, maybe getAttributes), something like time.js? (for parseDuration, you could also export the constants in that function if you wanted to). This would also give a place for future util-ish files to go and might prevent the file from getting out of hand.
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 if you wanted to reference one util file when importing you could put a file there like util.js or index.js that imports files in that folder.
src/index.js
Outdated
|
||
export const VERSION = version; | ||
|
||
export const parse = s => [ |
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.
Nice way of handling this piping.
One potential issue here (I'm guessing it won't be an issue) is if anything here become asynchronous. It's probably fine, but felt like that should be pointed out.
src/stringToMpdXml.js
Outdated
|
||
export const stringToMpdXml = (manifestString) => { | ||
if (manifestString === '') { | ||
throw new Error('DASH_EMPTY_MANIFEST'); |
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.
We should make sure to unit test this and the below to ensure the error gets thrown. It also might not be bad to extract these error strings somewhere else (you'll be able to consume them in the test and it will limit the number of magic strings).
src/fns.js
Outdated
const SECONDS_IN_DAY = 24 * 60 * 60; | ||
const SECONDS_IN_HOUR = 60 * 60; | ||
const SECONDS_IN_MIN = 60; | ||
const match = durationRegex.exec(str); |
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.
Should we do anything if the string that's passed in is malformed? Or is assuming the value is 0 enough?
src/inheritAttributes.js
Outdated
throw new Error('INVALID_NUMBER_OF_PERIOD'); | ||
} | ||
|
||
const representationsByPeriod = periods.map((period, periodIndex) => { |
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.
If you wanted to you could probably set up each of these functions for unit testing as well if you extract them/export them from here. This would let you write specific tests for the anonymous function for representationsByPeriod, as well as the one that is setting representationsByAdaptationSet.
That might help to make unit testing on inheritAttributes trivia. since you can cover testing a bulk of the logic in the above mentioned functions.
src/toM3u8.js
Outdated
segments: [], | ||
mediaGroups: { | ||
AUDIO: { | ||
main: { |
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.
This is a bit different from the out put of the m3u8 parser. Have we figured out how we plan to handle matching the video and audio playlists?
In HLS, this would look like
AUDIO: {
GROUP-ID: {
NAME: playlist
}
}
example
AUDIO: {
LOW: {
English: playlist,
Spanish: playlist
},
HIGH: {
English: playlist,
Spanish: playlist
}
}
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.
Yep that's just a stub, the highest audio rendition for each track will live in a hard coded HIGH group which will link to each of the video playlists. In the future we can see if we could pair low quality audio with video.
c813f53
to
0451ebe
Compare
634346a
to
de4e43f
Compare
58c2c8d
to
e0498ad
Compare
const uri = attributes.media.replace(/\$Number\$/gi, i) | ||
.replace(/\$RepresentationID\$/gi, attributes.id); | ||
|
||
return { |
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.
This will need to have the initialization segment added as a property
map: { uri, byteRange }
It's the only way right now that segment-loader uses to get the initialization segment detect if it needs to transmux from ts to mp4
f51ca09
to
769ad46
Compare
No description provided.