Adding support for segments in Period and Representation. #19
Conversation
package.json
Outdated
| @@ -86,6 +87,7 @@ | |||
| "version": "5.0.0" | |||
| }, | |||
| "dependencies": { | |||
| "deepmerge": "^2.0.1", | |||
There was a problem hiding this comment.
I think we should update the shallowMerge function to be a deepMerge and remove this dependency
There was a problem hiding this comment.
Do you think that we should make video.js a dependency so that we can use videojs.mergeOptions? Its excessive to make that whole project a dependency just for a merge, but as long as this is being used in another project that is using videojs, rollup or other bundlers should be able to handle and remove the redundancy? I'm not sure what best practices are for this case.
There was a problem hiding this comment.
When building with video.js's mergeOptions it seems like the rollup did not remove the unnecessary code and mpd-parser.js shot up to 680KB. I think it would be better to write a simple deep merge function instead.
src/toPlaylists.js
Outdated
| @@ -2,27 +2,25 @@ import { shallowMerge } from './utils/object'; | |||
| import { segmentsFromTemplate } from './segment/segmentTemplate'; | |||
| import { segmentsFromList } from './segment/segmentList'; | |||
| import { segmentsFromBase } from './segment/segmentBase'; | |||
|
|
|||
| // import merge from 'deepmerge'; | |||
There was a problem hiding this comment.
dont forget to remove this comment
src/inheritAttributes.js
Outdated
| const adaptationSets = findChildren(period, 'AdaptationSet'); | ||
| const periodInfo = getSegmentInformation(period); |
There was a problem hiding this comment.
I think we should call this periodSegmentInfo
src/inheritAttributes.js
Outdated
| @@ -178,9 +181,10 @@ export const toRepresentations = | |||
| roleAttributes); | |||
| const segmentInfo = getSegmentInformation(adaptationSet); | |||
| const representations = findChildren(adaptationSet, 'Representation'); | |||
| const periodAdaptationSetInfo = merge(periodInfo, segmentInfo); | |||
There was a problem hiding this comment.
I think we should call this adaptationSetSegmentInfo
src/inheritAttributes.js
Outdated
| const repBaseUrlElements = findChildren(representation, 'BaseURL'); | ||
| const repBaseUrls = buildBaseUrls(adaptationSetBaseUrls, repBaseUrlElements); | ||
| const attributes = shallowMerge(adaptationSetAttributes, getAttributes(representation)); | ||
| const segmentInfoFromRepresenation = getSegmentInformation(representation); |
There was a problem hiding this comment.
I think we should call this representationSegmentInfo
src/inheritAttributes.js
Outdated
|
|
||
| return repBaseUrls.map(baseUrl => { | ||
| return { | ||
| segmentInfo, | ||
| segmentInfo: segmentRepresentationInfo, |
There was a problem hiding this comment.
You could just do segmentInfo: merge(adaptationSetSegmentInfo, representationSegmentInfo) (using the naming from my other suggestions) and then you don't need the segmentRepresentationInfo variable.
There was a problem hiding this comment.
Sure I will make that change
package.json
Outdated
| @@ -86,6 +87,7 @@ | |||
| "version": "5.0.0" | |||
| }, | |||
| "dependencies": { | |||
| "deepmerge": "^2.0.1", | |||
There was a problem hiding this comment.
Do you think that we should make video.js a dependency so that we can use videojs.mergeOptions? Its excessive to make that whole project a dependency just for a merge, but as long as this is being used in another project that is using videojs, rollup or other bundlers should be able to handle and remove the redundancy? I'm not sure what best practices are for this case.
src/toPlaylists.js
Outdated
| return segmentsFromTemplate( | ||
| shallowMerge(segmentInfo.template, attributes), | ||
| segmentInfo.timeline | ||
| if (segmentInfo.list) { |
There was a problem hiding this comment.
Was there a specific reasoning for switching the order of list and template here? I think either order should be fine, just curious.
There was a problem hiding this comment.
I was just checking for the SegmentList output.
test/inheritAttributes.test.js
Outdated
| @@ -197,6 +198,7 @@ QUnit.test('end to end - basic', function(assert) { | |||
| height="404" | |||
| id="test" | |||
| width="720"> | |||
| <SegmentTemplate></SegmentTemplate> | |||
test/inheritAttributes.test.js
Outdated
| @@ -272,6 +274,7 @@ QUnit.test('end to end - inherits BaseURL from all levels', function(assert) { | |||
| height="404" | |||
| id="test" | |||
| width="720"> | |||
| <SegmentTemplate></SegmentTemplate> | |||
There was a problem hiding this comment.
Why was this one added as well?
test/inheritAttributes.test.js
Outdated
| startWithSAP: '1' | ||
|
|
||
| }, | ||
| segments: undefined |
There was a problem hiding this comment.
This should not be undefined, it should have a segment list built from the segment information provided in the adaptation set
| assert.deepEqual(actual, expected); | ||
| }); | ||
|
|
||
| QUnit.test(' End to End test for checking support of segments in period ', function(assert) { |
There was a problem hiding this comment.
Since this test is checking for support for segment information in the period, we should have the mpd not have any segment information contained in the adaptation set or representation to show that we can build a segment list when the segment info is just in the period
test/inheritAttributes.test.js
Outdated
| segmentInfo: { | ||
| base: undefined, | ||
| list: undefined, | ||
| template: undefined, |
There was a problem hiding this comment.
This should not be undefined, should have used template from containing adaptation set
test/inheritAttributes.test.js
Outdated
| segmentInfo: { | ||
| base: undefined, | ||
| list: undefined, | ||
| template: undefined, |
There was a problem hiding this comment.
This should not be undefined, should have used template from containing adaptation set
test/inheritAttributes.test.js
Outdated
|
|
||
| // Test for only periodAdaptationSetInfo | ||
|
|
||
| QUnit.test(' End to End test for returning period information', function(assert) { |
There was a problem hiding this comment.
This test is nearly identical to the one above it . Is it testing something specifically different?
test/inheritAttributes.test.js
Outdated
| height= "404" | ||
| id= "test" | ||
| width= "720" > | ||
| <SegmentBase></SegmentBase> |
test/inheritAttributes.test.js
Outdated
| assert.deepEqual(actual, expected); | ||
| }); | ||
|
|
||
| QUnit.test(' Test for checking presence of atmost one segment at each level', function(assert) { |
There was a problem hiding this comment.
This description is a bit misleading maybe something along the lines of "uses only one set of segment information when multiple are present" and then add a code comment saying that although the spec states that at each level at most one shall be present, we still handle the case to prevent throwing errors
| <Representation audioSamplingRate="48000" bandwidth="63000" codecs="mp4a.40.2" id="63000"></Representation> | ||
| <Representation audioSamplingRate="48000" bandwidth="125000" codecs="mp4a.40.2" id="125000"></Representation> | ||
| <Representation audioSamplingRate="48000" bandwidth="63000" codecs="mp4a.40.2" id="63000"> | ||
| <SegmentTemplate></SegmentTemplate> |
There was a problem hiding this comment.
all these <segment*> tags added to these mpd manifests shouldn't be necessary to add
There was a problem hiding this comment.
If you wanted to test your use case within an actual manifest, you could create a fresh one, and add that and the accompanying json to index.test.js.
src/inheritAttributes.js
Outdated
|
|
||
| return repBaseUrls.map(baseUrl => { | ||
| return { | ||
| segmentInfo: segmentRepresentationInfo, | ||
| segmentInfo: { | ||
| list: adaptationSetSegmentInfo.list && representationSegmentInfo.list ? merge(adaptationSetSegmentInfo.list, representationSegmentInfo.list) : adaptationSetSegmentInfo.list && !representationSegmentInfo.list ? adaptationSetSegmentInfo.list : !adaptationSetSegmentInfo.list && representationSegmentInfo.list ? representationSegmentInfo.list : undefined, |
There was a problem hiding this comment.
This is a nightmare to read. I think its best we try to come at this with a different approach.
The main issue with the merge is that if the second object in the merge has a property that is undefined, it overwrites that property with undefined, so removing those properties from the object before we even get to merging should solve the issue without needing all this complex logic.
I think that we should return to what you had before this commit of just merging the full segment info objects instead of merging these properties individually.
Then, to fix the issue of a property being undefined, in getSegmentInformation, before returning the segment info object, save it to a variable and then delete properties that are undefined
const segmentInfo = {
template,
timeline: ...
....
};
Object.keys(segmentInfo).forEach(key => {
if (!segmentInfo[key]) {
delete segmentInfo[key];
}
});
return segmentInfo;
src/inheritAttributes.js
Outdated
| @@ -181,10 +185,15 @@ export const toRepresentations = | |||
| roleAttributes); | |||
| const segmentInfo = getSegmentInformation(adaptationSet); | |||
| const representations = findChildren(adaptationSet, 'Representation'); | |||
| const periodAdaptationSetInfo = merge(periodInfo, segmentInfo); | |||
| const adaptationSetSegmentInfo = { | |||
| list: periodSegmentInfo.list && segmentInfo.list ? merge(periodSegmentInfo.list, segmentInfo.list) : periodSegmentInfo.list && !segmentInfo.list ? periodSegmentInfo.list : !periodSegmentInfo.list && segmentInfo.list ? segmentInfo.list : undefined, | |||
There was a problem hiding this comment.
Same strategy as my other comment here
src/inheritAttributes.js
Outdated
| @@ -210,10 +221,12 @@ export const toRepresentations = | |||
| */ | |||
| export const toAdaptationSets = (mpdAttributes, mpdBaseUrls) => (period, periodIndex) => { | |||
| const periodBaseUrls = buildBaseUrls(mpdBaseUrls, findChildren(period, 'BaseURL')); | |||
| const periodAttributes = shallowMerge({ periodIndex }, mpdAttributes); | |||
| const periodAtt = getAttributes(period); | |||
| const periodAttributes = shallowMerge(periodAtt, { periodIndex }, mpdAttributes); | |||
There was a problem hiding this comment.
Lets switch the order of the arguments here to be mpdAttributes, periodAtt, { periodIndex }. Off the top of my head I'm not sure if there are any overlapping attributes between the mpd or period, but to be on the safe side, switching those around will make sure any overlapping attributes get overwritten correctly from the period.
src/inheritAttributes.js
Outdated
| import { parseDuration } from './utils/time'; | ||
| import { findChildren, getContent } from './utils/xml'; | ||
| import resolveUrl from './utils/resolveUrl'; | ||
| import errors from './errors'; | ||
| import merge from 'deepmerge'; | ||
| // import merge from 'deepmerge'; |
src/inheritAttributes.js
Outdated
| @@ -1,10 +1,11 @@ | |||
| import { flatten } from './utils/list'; | |||
| import { shallowMerge, getAttributes } from './utils/object'; | |||
| import { getAttributes } from './utils/object'; | |||
There was a problem hiding this comment.
you can import merge along with getAttributes here
src/inheritAttributes.js
Outdated
| import { parseDuration } from './utils/time'; | ||
| import { findChildren, getContent } from './utils/xml'; | ||
| import resolveUrl from './utils/resolveUrl'; | ||
| import errors from './errors'; | ||
| import merge from 'deepmerge'; | ||
| // import merge from 'deepmerge'; | ||
| import { merge } from './utils/object'; |
src/toPlaylists.js
Outdated
| @@ -2,27 +2,25 @@ import { shallowMerge } from './utils/object'; | |||
| import { segmentsFromTemplate } from './segment/segmentTemplate'; | |||
| import { segmentsFromList } from './segment/segmentList'; | |||
| import { segmentsFromBase } from './segment/segmentBase'; | |||
|
|
|||
| // import merge from 'deepmerge'; | |||
There was a problem hiding this comment.
dont forget to remove this comment
src/utils/object.js
Outdated
| export const shallowMerge = (...objects) => { | ||
| export const merge = (...objects) => { | ||
|
|
||
| const isObject = (obj) => { |
There was a problem hiding this comment.
this function can be defined outside of merge
src/utils/object.js
Outdated
| export const merge = (...objects) => { | ||
|
|
||
| const isObject = (obj) => { | ||
| return obj && typeof obj === 'object'; |
There was a problem hiding this comment.
lets be explicit and use !!obj
src/utils/object.js
Outdated
| const isObject = (obj) => { | ||
| return obj && typeof obj === 'object'; | ||
| }; | ||
|
|
||
| return objects.reduce((x, y) => { |
There was a problem hiding this comment.
I think it would be nice to update this to use more descriptive names for readability. maybe result for x and source for y
| @@ -8,25 +8,25 @@ import QUnit from 'qunit'; | |||
|
|
|||
| QUnit.module('utils'); | |||
|
|
|||
| QUnit.module('shallowMerge'); | |||
| QUnit.module('merge'); | |||
There was a problem hiding this comment.
Might want to add a test or two for the new merge functionality
No description provided.