Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adding support for segments in Period and Representation. #19

Merged
merged 9 commits into from
Feb 26, 2018
Merged

Adding support for segments in Period and Representation. #19

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
be6b06f
Adding support for segments in Period and Representation
ishita12 Feb 13, 2018
d5e4cbd
Testing for getting atmost one segment at each level
ishita12 Feb 13, 2018
a7877d5
Adding tests for checking presence of multiple segment tags at a par…
ishita12 Feb 14, 2018
59e4b48
Updating the tests
ishita12 Feb 16, 2018
705b713
Updating the tests
ishita12 Feb 20, 2018
90c0595
Changing order of attributes
ishita12 Feb 20, 2018
bd9d668
Updating shallowMerge to be a deepMerge function
ishita12 Feb 20, 2018
984c176
Updating shallowMerge to be a deepMerge function
ishita12 Feb 20, 2018
425eaf9
Adding tests for testing merge function
ishita12 Feb 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
205 changes: 197 additions & 8 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
"src/"
],
"devDependencies": {
"babel-eslint": "^8.2.1",
"babel-plugin-external-helpers": "^6.22.0",
"babel-plugin-transform-object-assign": "^6.8.0",
"babel-preset-es2015": "^6.14.0",
Expand Down Expand Up @@ -86,6 +87,7 @@
"version": "5.0.0"
},
"dependencies": {
"deepmerge": "^2.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should update the shallowMerge function to be a deepMerge and remove this dependency

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"global": "^4.3.0",
"url-toolkit": "^2.1.1"
}
Expand Down
19 changes: 12 additions & 7 deletions src/inheritAttributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { parseDuration } from './utils/time';
import { findChildren, getContent } from './utils/xml';
import resolveUrl from './utils/resolveUrl';
import errors from './errors';
import merge from 'deepmerge';

/**
* Builds a list of urls that is the product of the reference urls and BaseURL values
Expand Down Expand Up @@ -131,14 +132,16 @@ export const getSegmentInformation = (adaptationSet) => {
* Callback map function
*/
export const inheritBaseUrls =
(adaptationSetAttributes, adaptationSetBaseUrls, segmentInfo) => (representation) => {
(adaptationSetAttributes, adaptationSetBaseUrls, periodAdaptationSetInfo) => (representation) => {
const repBaseUrlElements = findChildren(representation, 'BaseURL');
const repBaseUrls = buildBaseUrls(adaptationSetBaseUrls, repBaseUrlElements);
const attributes = shallowMerge(adaptationSetAttributes, getAttributes(representation));
const segmentInfoFromRepresenation = getSegmentInformation(representation);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should call this representationSegmentInfo

const segmentRepresentationInfo = merge(periodAdaptationSetInfo, segmentInfoFromRepresenation);

return repBaseUrls.map(baseUrl => {
return {
segmentInfo,
segmentInfo: segmentRepresentationInfo,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could just do segmentInfo: merge(adaptationSetSegmentInfo, representationSegmentInfo) (using the naming from my other suggestions) and then you don't need the segmentRepresentationInfo variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I will make that change

attributes: shallowMerge(attributes, { baseUrl })
};
});
Expand Down Expand Up @@ -167,7 +170,7 @@ export const inheritBaseUrls =
* Callback map function
*/
export const toRepresentations =
(periodAttributes, periodBaseUrls) => (adaptationSet) => {
(periodAttributes, periodBaseUrls, periodInfo) => (adaptationSet) => {
const adaptationSetAttributes = getAttributes(adaptationSet);
const adaptationSetBaseUrls = buildBaseUrls(periodBaseUrls,
findChildren(adaptationSet, 'BaseURL'));
Expand All @@ -178,9 +181,10 @@ export const toRepresentations =
roleAttributes);
const segmentInfo = getSegmentInformation(adaptationSet);
const representations = findChildren(adaptationSet, 'Representation');
const periodAdaptationSetInfo = merge(periodInfo, segmentInfo);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should call this adaptationSetSegmentInfo


return flatten(
representations.map(inheritBaseUrls(attrs, adaptationSetBaseUrls, segmentInfo)));
representations.map(inheritBaseUrls(attrs, adaptationSetBaseUrls, periodAdaptationSetInfo)));
};

/**
Expand Down Expand Up @@ -210,10 +214,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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const adaptationSets = findChildren(period, 'AdaptationSet');
const periodInfo = getSegmentInformation(period);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should call this periodSegmentInfo


return flatten(adaptationSets.map(toRepresentations(periodAttributes, periodBaseUrls)));
return flatten(adaptationSets.map(toRepresentations(periodAttributes, periodBaseUrls, periodInfo)));
};

/**
Expand Down Expand Up @@ -243,4 +249,3 @@ export const inheritAttributes = (mpd, manifestUri = '') => {

return flatten(periods.map(toAdaptationSets(mpdAttributes, mpdBaseUrls)));
};

22 changes: 10 additions & 12 deletions src/toPlaylists.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leftover comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont forget to remove this comment

export const generateSegments = (segmentInfo, attributes) => {
if (segmentInfo.template) {
return segmentsFromTemplate(
shallowMerge(segmentInfo.template, attributes),
segmentInfo.timeline
if (segmentInfo.list) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a specific reasoning for switching the order of list and template here? I think either order should be fine, just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just checking for the SegmentList output.

return segmentsFromList(
shallowMerge(attributes, segmentInfo.list), segmentInfo.timeline
);
}

if (segmentInfo.base) {
return segmentsFromBase(shallowMerge(segmentInfo.base, attributes));
return segmentsFromBase(shallowMerge(attributes, segmentInfo.base));
}

if (segmentInfo.list) {
return segmentsFromList(
shallowMerge(segmentInfo.list, attributes), segmentInfo.timeline
if (segmentInfo.template) {
return segmentsFromTemplate(
shallowMerge(attributes, segmentInfo.template),
segmentInfo.timeline
);
}
};

export const toPlaylists = representations => {
export const toPlaylists = (representations) => {
return representations.map(({ attributes, segmentInfo }) => {
const segments = generateSegments(segmentInfo, attributes);

Expand Down
Loading