Parse <SegmentList> and <SegmentBase> #18

OshinKaramian · 2018-01-26T22:33:40Z

This adds the ability for us to parse SegmentList and SegmentBase.

There were some minor reshuffling as well (I don't think it's a huge deal to keep it in here, although I can do it as a seperate PR if that's desirable).

As a note the reshuffling includes:

Moving some of the segment duration/timeline parsing out of segmentTemplate.js into its own file since SegmentBase shares that logic with SegmentTemplate.
Putting all <Segment*> tag parsing into its own folder.
Moving resolveUrl.js into the utils folder.

Merge in segment template changes

Support for inheriting BaseURL and alternate BaseURLs (#17)

…de around

forbesjo · 2018-01-26T22:41:01Z

src/toPlaylists.js

@@ -18,16 +13,13 @@ export const generateSegments = (segmentInfo, attributes) => {

  // TODO


probably not a TODO anymore

forbesjo · 2018-01-26T22:42:42Z

src/errors.js

@@ -2,5 +2,8 @@ export default {
  INVALID_NUMBER_OF_PERIOD: 'INVALID_NUMBER_OF_PERIOD',
  DASH_EMPTY_MANIFEST: 'DASH_EMPTY_MANIFEST',
  DASH_INVALID_XML: 'DASH_INVALID_XML',
-  UNSUPPORTED_SEGMENTATION_TYPE: 'UNSUPPORTED_SEGMENTATION_TYPE'
+  UNSUPPORTED_SEGMENTATION_TYPE: 'UNSUPPORTED_SEGMENTATION_TYPE',


No need for this error

mjneil · 2018-01-29T22:57:22Z

src/inheritAttributes.js

+            shallowMerge(getAttributes(segmentList),
+              {
+                segmentUrls: segmentUrls &&
+                  segmentUrls.map(s =>


you could move this map to findChildren(segmentList, 'SegmentURL').map(..., then this just becomes { segmentUrls }

mjneil · 2018-01-29T23:07:26Z

src/segment/segmentList.js

+    // Since we're mapping we should get rid of any blank segments (in case
+    // the given SegmentTimeline is handling for more elements than we have
+    // SegmentURLs for).
+  }).filter(segment => segment);


Is this common/supposed/allowed to happen? What cases cause this?

So... this shouldn't happen if the mpd is well constructed. But, let's say you have a segment timeline that defines r=10, but you only have 5 segments listed. We will have too many duration objects for the number of segments we have. I guess we could throw an error, but I felt like handling it and truncating made more sense in this case.

Makes sense. I suppose its either we crash up front, or play what we can parse and just have an incomplete video, which is better than no video

mjneil · 2018-01-29T23:29:12Z

src/toPlaylists.js

-
-    // return segmentsFromList(attributes);
+    return segmentsFromList(
+      shallowMerge(segmentInfo.list, attributes), segmentInfo.timeline


Can anything in attributes overlap with segmentInfo.list (and segmentInfo.base for above)? If so we probably want to swap the order of the arguments so any overlapping values take from segmentInfo. If not then no need

Which makes me realize, I should double check for segment template

In either case it seems safer to swap the order.

It doesn't look like there will be any overlap. Depending on how Initialization tag is handled, will have to keep that in mind for SegmentTemplate since it can have both the tag and attribute. I don't think we will encounter a manifest with SegmentTemplate that has both the attribute and tag, but if we do, we will want to make a choice on which takes precedence.

We shouldn't encounter that scenario, but it's a possibility. I didn't see anything in the spec with guidance where @initialization and <Initialization> relate to the same <SegmentTemplate>. So it seems it's up to our discretion. I made an assumption that if both are provided that the <Initialization> will take precedence (and added a comment noting as much). Given the tree structure I think that makes sense.

The other options are to just error if this is the case (less permissive, and it is probably easier to move from less permissive -> more permissive) or @initialization taking precedence.

Makes sense to me 👍

mjneil

This will need <Initialization> support. SegmentBase and SegmentList do not have an initialization attribute and require the tag. SegmentTemplate can have the tag or as an attribute. Right now template doesn't support the tag, you can add it to this pr if you want, or we can add it into another, but I think for List and Base support, we should have it. See Table 13 (5.3.9.2.2) for semantics, and 5.3.9.5.2 to see where it mentions List and Template can have the tag since the spec does not mention it in their respective definitions.

Edit: They also all seem to be able to have a <RepresentationIndex>, but I'm not quite sure what that is for or if we need it just yet.

OshinKaramian · 2018-01-30T00:01:19Z

Ah, good catch, I missed that bit of nuance. I'll handle that in this PR.

Edit: I'm going to leave <RepresentationIndex> alone here, until we have a better idea of how it fits into how things are structured in the m3u8-parser.

OshinKaramian · 2018-01-30T20:26:19Z

src/segment/segmentTemplate.js

+    };
+  }
+
+  if (attributes.initialization && typeof attributes.initialization === 'object') {


Based on what I picked up off the spec, <Initialization> doesn't get templated, but @initialization does, which is why there is a difference here.

mjneil · 2018-01-30T21:49:04Z

src/inheritAttributes.js

+  const template = segmentTemplate && getAttributes(segmentTemplate);
+
+  if (template && segmentInitialization) {
+    template.initialization = getAttributes(segmentInitialization);


I think it would be good if we can account for the string vs object here and have template.initialization always be an object.

if (template) { template.initialization = (segmentInitialization && getAttributes(segmentIntitialization) || { sourceURL: template.initialization || '' }; }

This way in segmentsFromTemplate you do not need the extra code path for the string

mjneil · 2018-01-30T22:04:53Z

src/segment/segmentTemplate.js

+  if (attributes.initialization && typeof attributes.initialization === 'object') {
+    mapSegment = urlTypeToSegment({
+      baseUrl: attributes.baseUrl,
+      source: attributes.initialization.sourceURL,


With the changes proposed in my other comment to handle the string to object conversion in getSegmentInformation, I think you can safely do

source: constructTemplateUrl(attributes.initialization.sourceURL, templateValues)

In the case that the <Initialization> node took precedence, there shouldn't be any template identifiers, so constructTemplateUrl here should just return the sourceURL unchanged

mjneil · 2018-01-30T22:17:30Z

src/segment/urlType.js

+    resolvedUri: resolveUrl(baseUrl || '', source)
+  };
+
+  if (source && range) {


I think range should be parsed even if source is not available. According to the spec for @sourceURL,

If not present, then any BaseURL element is mapped to the @sourceURL attribute and the range attribute shall be present.

The BaseURL mapping is already handled by the resolvedUri above, but this check would prevent parsing of the range that "shall be present"

Good catch. I made an assumption that both were required but the existence of baseUrl makes that a poor assumption.

mjneil · 2018-01-31T16:43:32Z

src/inheritAttributes.js

+  const segmentTimelineNode = segmentList || segmentTemplate;
+  const segmentTimeline =
+    segmentTimelineNode && findChildren(segmentTimelineNode, 'SegmentTimeline')[0];
+  const segmentInitializationNode = segmentList || segmentBase || segmentTemplate;


These variable names are a bit misleading. segmentTimelineNode and segmentInitializationNode are actually the parent nodes of those nodes

mjneil · 2018-01-31T16:48:08Z

src/inheritAttributes.js

-                     findChildren(segmentTimeline, 'S').map(s => getAttributes(s)),
-    list: segmentList && getAttributes(segmentList),
-    base: segmentBase && getAttributes(segmentBase)
+                    findChildren(segmentTimeline, 'S').map(s => getAttributes(s)),


I know that this line was added by me, including the indentation, but do you mind fixing the indentation here?

Wonder why the linter didn't catch this one, I think it gave me a warning or error for the indentation under list: and base:.

Ya I'm surprised I added it myself, and didn't catch it myself or with the linter. Its not even lined up with anything ha

mjneil

Note for future PR:

In 5.3.9.5.2 of the spec, this is specified

If neither Initialization element nor SegmentTemplate@initialization attribute are present for a Representation then each Media Segment within the Representation shall be self-initializing.

Right now when this scenario is encountered, this PR will still create a segment.map = { uri: '', resolvedUri: baseUrl } with baseUrl not being the location of an initialization segment.

In VHS, the existence of segment.map is used to determine if the segments are fmp4 or ts, and VHS will automatically request the initialization segment specified in segment.map without any checks, and appends the response to the source buffer.

We will have to think about how to handle this case, which can potentially require changes in both this project and VHS.

OshinKaramian and others added 9 commits January 19, 2018 16:22

Merge pull request #1 from videojs/master

fb9b91a

Merge in segment template changes

Real messy early commit

e31b500

Merge pull request #2 from videojs/master

8629106

Support for inheriting BaseURL and alternate BaseURLs (#17)

Sort of resolve merge

9e06c2c

Add segmentBase

6b89e5a

Add some tests, modify some logic to accomodate spec, shuffle some co…

0fa09fc

…de around

Add more tests and cover more cases, add comments

2a9f36a

uncomment some commented out tests, eslint fixes, package.json fix

1d5b64f

Add parantheses to make this clearer

e7469b6

forbesjo reviewed Jan 26, 2018

View reviewed changes

forbesjo mentioned this pull request Jan 26, 2018

UNSUPPORTED_SEGMENTATION_TYPE Error: UNSUPPORTED_SEGMENTATION_TYPE #5

Closed

forbesjo approved these changes Jan 29, 2018

View reviewed changes

Code review updates

2e4b4ec

mjneil reviewed Jan 29, 2018

View reviewed changes

mjneil suggested changes Jan 29, 2018

View reviewed changes

Add parsing for Initialization nodes

4539d9c

OshinKaramian commented Jan 30, 2018

View reviewed changes

mjneil reviewed Jan 30, 2018

View reviewed changes

Oshin Karamian added 2 commits January 30, 2018 18:27

Code review updates and add tests for urlType

9b1aad7

Fix lint errors

8bb1413

mjneil reviewed Jan 31, 2018

View reviewed changes

Update variable names to be cleareR

b9f69ad

mjneil reviewed Jan 31, 2018

View reviewed changes

Fix indentation

06cd527

mjneil approved these changes Jan 31, 2018

View reviewed changes

forbesjo merged commit 71b8976 into videojs:master Feb 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse <SegmentList> and <SegmentBase> #18

Parse <SegmentList> and <SegmentBase> #18

OshinKaramian commented Jan 26, 2018

forbesjo Jan 26, 2018

forbesjo Jan 26, 2018

mjneil Jan 29, 2018

mjneil Jan 29, 2018

OshinKaramian Jan 29, 2018

mjneil Jan 30, 2018

mjneil Jan 29, 2018

mjneil Jan 29, 2018

OshinKaramian Jan 30, 2018

mjneil Jan 30, 2018

OshinKaramian Jan 30, 2018

mjneil Jan 30, 2018

mjneil left a comment •

edited

Loading

OshinKaramian commented Jan 30, 2018 •

edited

Loading

OshinKaramian Jan 30, 2018

mjneil Jan 30, 2018 •

edited

Loading

mjneil Jan 30, 2018

mjneil Jan 30, 2018

OshinKaramian Jan 30, 2018

mjneil Jan 31, 2018

mjneil Jan 31, 2018

OshinKaramian Jan 31, 2018 •

edited

Loading

mjneil Jan 31, 2018

mjneil left a comment

		@@ -18,16 +13,13 @@ export const generateSegments = (segmentInfo, attributes) => {

		// TODO

Parse <SegmentList> and <SegmentBase> #18

Parse <SegmentList> and <SegmentBase> #18

Conversation

OshinKaramian commented Jan 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjneil left a comment • edited Loading

Choose a reason for hiding this comment

OshinKaramian commented Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

mjneil Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OshinKaramian Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjneil left a comment

Choose a reason for hiding this comment

mjneil left a comment •

edited

Loading

OshinKaramian commented Jan 30, 2018 •

edited

Loading

mjneil Jan 30, 2018 •

edited

Loading

OshinKaramian Jan 31, 2018 •

edited

Loading