XML parser #3412

bbert · 2020-09-23T13:38:50Z

The goal of this PR is to provide a new XML parser in order to speed up parsing time.
Parsing time may be criticial for long contents with SegmentTimelines.

What has been achieved in this PR:

integration of tXml parser: https://github.com/TobiasNickel/tXml
modification of the tXml parser in order to apply simplification process (https://github.com/TobiasNickel/tXml#xmlsimplify-txml_dom_object) on-the-fly during parsing, while also applying matchers on the attributes values and managing children that shall be stored as arrays
remove the StringMatcher since it is not useful anymore as every attribute is parsed by default as a string (BTW simply removing this matcher could reduce up to 30-40% the original parsing processing time)
update of the NumericMatcher in order to avoid convert into numbers attributes that shall be in string format but that can be filled as a numeric value ("id" attribute for example)
remove all references to '_asArray' notation (no more duplication of child nodes as a single object and as an array of object)

Here are some numbers for the XML parsing processing time (in ms) for a 240mn content having one AdaptationSet with a SegmentTimeline without repeat pattern:

	Chrome desktop (core i7 2x2.7GHz 16Go)	Chromecast 3rd Gen	Chromecast Ultra
dash.js	110	2 500	1 800
dash.js + tXml	30	500	350

…ree and manage node as arrays

…ssue with ES6 syntax

…nodes as array process) on-the-fly

…d remove _asArray nodes)

bbert · 2021-02-10T09:54:33Z

@technogeek00 can you please check this PR on your side regarding all the changes from your PR #3451?
Can you check if does not break anything? Also can you check the unit tests since some of them do not success anymore? But I don't know how to fix them.

…d remove _asArray nodes)

technogeek00 · 2021-02-16T17:42:09Z

@bbert I can take a look, will take me a few days to get to it.

bbert · 2021-03-26T08:03:05Z

@technogeek00 Did you have time to check this PR and unit tests?

technogeek00 · 2021-04-08T01:22:22Z

Reviewing tonight

technogeek00

First I apologize for the delay in my review, but hopefully this is insightful feedback.

I pulled this branch and attempted to run the player and the tests locally, both appeared broken as indicated, but upon further investigation there seems to be deeper problems with the library proposed. The points I would highlight for discussion:

The ordering of sibling elements of different tag names is not persisted, this is critical to patching operations as the add/remove/replace operations operate on the consecutively modified document
There appears to be little to no proper support for xml namespacing, the prefixes are directly preserved, but these are arbitrary in a document and need to be properly handled

As a note on the first point, this is technically only critical for the patch document, the non-modified format of the tXml library does appear to support parsing and simplification independently. If that version was taken directly the DashParser.js file could apply simplification to the MPD element to keep footprints small while the Patch could be left untouched to persist the ordering information for usage in application.

While I agree the parsing and internal structuring of the current library should be replaced with a more performant option, this particular library seems very purpose built for a specific use case and may not be fully conformant. If this library was used, I'd recommend reverting back to the original version, re-porting the attribute matcher framework, and then utilize the simplified version for mpds and lossless simplified version for patches instead of manually modifying the original parse process.

Outside the library we may need to take a close look at document usage cases to ensure single vs array element usage is done consistently and accurately with the updated parsed structure. I highlighted a couple bugs in this review, but I'd worry about areas that did not reference the _asArray and still always expected it or others that always assumed standalone elements.

technogeek00 · 2021-04-08T04:25:43Z

externals/tXml.js

+function tXml(S, options, attrMatchers, arrayChildNames) {
+    "use strict";
+    options = options || {};
+    attrMatchers = attrMatchers || {};


type mismatch, attrMatchers used as array but defaulted to object.

technogeek00 · 2021-04-08T04:27:53Z

externals/tXml.js

+
+ * @return {tNode[]}
+ */
+function tXml(S, options, attrMatchers, arrayChildNames) {


Given the differences between this file and the main tXML attrMatches and arrayChildNames appear to be extensions, it may be better to integrate them through the libraries existing conventions of the options object.

The attribute matchers are a nice addition, but generally why introduce a sometimes array type child function like arrayChildNames instead of using the full tXML file and passing options.simplify: true to have the all children are arrays simplified form without file modification?

attrMatches and attrMatches can indeed be passed in the options object.
So do you suggest to have systematically all children being stored as arrays? This has the disadvantage to access some child nodes as an array while DASH spec indicates for some nodes you can have only (for example SegmentBase)

technogeek00 · 2021-04-08T04:55:15Z

src/dash/parser/DashParser.js

+    DashConstants.SUPPLEMENTAL_PROPERTY,
+    DashConstants.METRICS,
+    DashConstants.REPORTING
+];


Missing elements preventing patching from working:

PatchLocation

replace

add

remove

Example patch documents can be found in the DASH Schema repo:

example_patch_base.mpd

example_patch_update.mpd

Two other random notes discovered in my personal testing:

The parsing library does not handle namespaced elements properly which means future inclusion will need to build this out in the library. I highlight this because the Annex I query parameter work may soon be included from the DASH-IF Ad Insertion work which requires namespacing. Note that with XML namespaces the namespace identifier in documents are arbitrary and part of the node definition. Example namespaced node also in the DASH Schema repo example_I1.mpd

The parsing library does not correctly unescape XML characters, i.e. & => & does not occur rendering URLs invalid.

Missed these ones. I did fix

technogeek00 · 2021-04-08T05:16:19Z

src/dash/models/DashManifestModel.js

@@ -1043,7 +1043,7 @@ function DashManifestModel() {
        if (manifest && manifest.hasOwnProperty(Constants.LOCATION)) {
            // for now, do not support multiple Locations -
            // just set Location to the first Location.
-            manifest.Location = manifest.Location_asArray[0];
+            manifest.Location = manifest.Location[0];


This is an unsafe re-assignment, it will cause the Location to be truncated on subsequent calls. With Location always an array this line should be omitted and the return should be shifted to return manifest.Location[0]

Good point, did replace asArray without paying attention to this.
I fixed it

technogeek00 · 2021-04-08T05:16:26Z

src/dash/models/DashManifestModel.js

@@ -1055,7 +1055,7 @@ function DashManifestModel() {
    function getPatchLocation(manifest) {
        if (manifest && manifest.hasOwnProperty(DashConstants.PATCH_LOCATION)) {
            // only include support for single patch location currently
-            manifest.PatchLocation = manifest.PatchLocation_asArray[0];
+            manifest.PatchLocation = manifest.PatchLocation[0];


Same as above

technogeek00 · 2021-04-08T05:20:27Z

src/dash/models/DashManifestModel.js

        }

        mimeTypeRegEx = (type !== Constants.TEXT) ? new RegExp(type) : new RegExp('(vtt|ttml)');

-        if (adaptation.Representation_asArray && adaptation.Representation_asArray.length && adaptation.Representation_asArray.length > 0) {
-            let essentialProperties = getEssentialPropertiesForRepresentation(adaptation.Representation_asArray[0]);
+        if (adaptation.Representation && adaptation.Representation.length && adaptation.Representation.length > 0) {


A theme I've observed across the review, there are a number of places that the dash.js code was guarding against the possibility of the element not being an array. With the forced array representation only an existence check is needed:

if(adaptation.Representation && adaptation.Representation.length > 0) {

Agree, I did update these checks

technogeek00 · 2021-04-08T05:39:02Z

src/dash/vo/SimpleXPath.js

@@ -106,7 +106,7 @@ class SimpleXPath {

            // stop one early if this is the last element and an attribute
            if (level !== this.path.length - 1 || !name.startsWith('@')) {
-                let children = parent[name + '_asArray'] || [];
+                let children = parent[name] || [];


This change assumes that all children are now arrays, but only a limited set are based on the parsing logic. While some SimpleXPath tests are failing because the test expects need to be updated to use array notation: Period.AdaptationSet => Period[0].AdaptationSet[0].

The remainder still fail because of nodes that are not arrays, such as SegmentTemplate and SegmentTimeline. For those objects, this line will take the object direction and it will have fail the length check as it is a strict equality to zero causing the node search to fail.

Maybe we can fix it, for example by considering the list of node names that should be as array, as done in the parser.
But I would need some test streams to check. Do you have some?

technogeek00 · 2021-04-08T05:42:14Z

test/unit/helpers/PatchHelper.js

@@ -26,7 +25,7 @@ function staticSegmentTimeline() {
 function staticSegmentTemplate() {
    let timeline = staticSegmentTimeline();
    return {
-        SegmentTimeline: timeline, // purposely omit the _asArray to ensure single node case captured
+        SegmentTimeline: timeline,


no problem on this exact line, but need to highlight in this file

Note that the PatchHelper is explicitly generating the patches with well formed and ordered __children arrays because ordering of elements is significant for patch operations, they must be supplied sequentially not based on node type. Actual code reference: https://github.com/Dash-Industry-Forum/dash.js/blob/development/test/unit/helpers/PatchHelper.js#L87

The simplified output of the updated parsing library does not appear capable of representing this information.

Re-apply modifications to tXml parser library in order to: - add child node to their parent as arrays or object according to node names - process attributes on the fly - set node text value in "_text" property

…at can contain a number value (such as "id" attribute)

lkinasiewicz · 2023-04-28T08:44:07Z

src/dash/DashAdapter.js

-                    // if we did have a positional reference we need to purge from array set and restore X2JS proper semantics
-                    if (relativePosition != -1) {
-                        let targetArray = target[name + '_asArray'];
+                    if (typeof target[name] === Object) {


Suggested change

if (typeof target[name] === Object) {

if (typeof target[name] === 'object') {

typeof returns a string.

Done in PR #4180

bbert · 2023-05-05T08:01:07Z

Closed, see #4180

bbert added 13 commits September 21, 2020 17:03

update package-lock.json

f4d956a

add txml parser dependency

d52688c

DashParser: use txml parser and add post-processing to simplify xml t…

7d626f4

…ree and manage node as arrays

remove '_asArray' notation

b807a62

DashParser: fix processAttr()

322038f

add tXml in externals and remove from dependencies, to avoid uglify i…

462d78a

…ssue with ES6 syntax

XlinkController: use parser (tXml) from DashParser

56861ba

DashParser: handle parsing failure

174ba8b

MssParser: complete _asArray removal

5214418

update unit tests

331a295

update DashConstants

bdbcbcd

Modify tXml to apply simplification proces (including attributes and …

2b963e6

…nodes as array process) on-the-fly

Remove StringMatcher

053eafc

bbert added Feature Enhancement Work in Progress - Don't Merge labels Sep 23, 2020

dsilhavy added this to the 3.2.1 milestone Dec 10, 2020

dsilhavy modified the milestones: 3.2.1, 3.3.0 Jan 18, 2021

bbert added 2 commits February 9, 2021 17:38

Merge branch 'development' into xml-parser

f4830da

update according to PR Dash-Industry-Forum#3451 (update xml parser an…

eb87794

…d remove _asArray nodes)

bbert added 2 commits February 10, 2021 15:03

update according to PR Dash-Industry-Forum#3451 (update xml parser an…

1432481

…d remove _asArray nodes)

update according to PR Dash-Industry-Forum#3451 (update xml parser an…

666d1fa

…d remove _asArray nodes)

Merge branch 'development' into xml-parser

3ca4a3a

dsilhavy modified the milestones: 3.2.2, 4.0.0 Mar 29, 2021

technogeek00 reviewed Apr 8, 2021

View reviewed changes

dsilhavy added this to the 4.1.0 milestone Jun 27, 2021

dsilhavy removed this from the 4.1.0 milestone Sep 15, 2021

bbert added 17 commits March 10, 2022 16:26

Merge branch 'development' into xml-parser

8343cdf

Merge branch 'development' into xml-parser

0792283

Merge remote-tracking branch 'origin/development' into xml-parser

4ad3d6a

xml parser: complete asArray removal

5926140

xml parser: fix samples

47db0a6

xml parser: fix regression after merge

88f8369

Merge branch 'development' into xml-parser

bc18c42

Update tXml parser to v4.0.1

2181999

Modify tXml parser

f517946

Re-apply modifications to tXml parser library in order to: - add child node to their parent as arrays or object according to node names - process attributes on the fly - set node text value in "_text" property

Update DashParser according to new version of tXml

76fd9bc

UTCTiming to be stored as an array

b213172

NumericMatcher: do not apply on attributes specified as string but th…

d5058e7

…at can contain a number value (such as "id" attribute)

Merge branch 'development' into xml-parser

1b735c7

Remove StringMatcher

db9b4f3

Merge remote-tracking branch 'origin/development' into xml-parser

602a6ae

Merge remote-tracking branch 'origin/development' into xml-parser

70dd5b7

Fix merge for LangMatcher

f4e5a8b

dsilhavy added this to the 4.6.0 milestone Oct 10, 2022

bbert and others added 2 commits October 20, 2022 11:09

Merge remote-tracking branch 'origin/development' into xml-parser

d14cb7d

Merge branch 'development' into xml-parser

ef3389e

dsilhavy modified the milestones: 4.5.2, 4.6.0 Dec 8, 2022

dsilhavy modified the milestones: 4.6.0, 5.0.0 Jan 26, 2023

lkinasiewicz reviewed Apr 28, 2023

View reviewed changes

bbert mentioned this pull request May 5, 2023

XML parser #4180

Merged

bbert closed this May 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XML parser #3412

XML parser #3412

bbert commented Sep 23, 2020 •

edited

Loading

bbert commented Feb 10, 2021

technogeek00 commented Feb 16, 2021

bbert commented Mar 26, 2021

technogeek00 commented Apr 8, 2021

technogeek00 left a comment

technogeek00 Apr 8, 2021

bbert Apr 16, 2021

technogeek00 Apr 8, 2021

bbert Apr 16, 2021

technogeek00 Apr 8, 2021

bbert Apr 16, 2021

technogeek00 Apr 8, 2021

bbert Apr 16, 2021

technogeek00 Apr 8, 2021

bbert Apr 16, 2021

technogeek00 Apr 8, 2021

bbert Apr 16, 2021

technogeek00 Apr 8, 2021

bbert Apr 16, 2021 •

edited

Loading

technogeek00 Apr 8, 2021

lkinasiewicz Apr 28, 2023

bbert May 5, 2023

bbert commented May 5, 2023

	if (typeof target[name] === Object) {
	if (typeof target[name] === 'object') {

XML parser #3412

XML parser #3412

Conversation

bbert commented Sep 23, 2020 • edited Loading

bbert commented Feb 10, 2021

technogeek00 commented Feb 16, 2021

bbert commented Mar 26, 2021

technogeek00 commented Apr 8, 2021

technogeek00 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbert Apr 16, 2021 • 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

bbert commented May 5, 2023

bbert commented Sep 23, 2020 •

edited

Loading

bbert Apr 16, 2021 •

edited

Loading