Update externals/xml2json.js

[defkrie]

Hi,
is it possible to update library externals/xml2json.js ?
Current version is 1.0.11. On github, you can see last version is 1.2.0 at https://github.com/abdmob/x2js/blob/master/xml2json.js.
thanks

[davemevans]

What would we gain from moving to the latest version over the one we have tested* ?

The minified version looks to be about 50% bigger (~6.8k vs 4.3k) which seems a good argument against updating.

[*] tested as in deployed without reported bugs for a number of years.

[TobbeEdgeware]

We have also made a small change to xml2json in order to be able to parse TTML properly without losing information. If an update to a later version is made, that change has to be carried over to avoid that TTML subtitling stops working.

[dsparacio]

@defkrie, Unless you have a good argument of why we should change what is proven, I think we not make this change and close this issue. Anyone have any objections?

[defkrie]

@AkamaiDASH In fact, I have trouble using the player dash.js on tv and and on box with limited memory and processors. By turning on the logs, I noticed that I was losing more than a minute to decode a manifest xml to json. On a conventional browser that operation takes less than a second.

@TobbeEdgeware You're right, I looked at the source code of the library xml2json and there have been changes in dash.js. By testing only the latest version of the library xml2json regardless of dashjs, I earn more than one third of the time to decode it.
I can't update the library to 1.2.0 which works in dashjs to be abble to parse TTML properly. Could you watch and try please? It would help me a lot

[davemevans]

Bear in mind that we have also added code to xml2json to do type conversion from datetime, duration and numeric which would also need adding in (though there does appear to be some datetime stuff in the latest version)

I ran up xml2json with some representative MPDs in Firefox on a 2015 Smart TV from a major manufacturer and saw no discernible difference in time taken between versions. For example http://rdmedia.bbc.co.uk/dash/ondemand/testcard/1/client_manifest-events.mpd took an average of 28ms to parse across a number of runs with both versions. The same manifest takes an average of 4ms on my desktop. Comparing with the OP's numbers, perhaps a poorly crafted manifest is the culprit?

If we are going to update, it would be worth removing all the JSON to XML stuff to try to offset the increased library size. We could also do this to the current version if we chose not to upgrade.

[sandersaares]

We needed to update this library for a private fork of dash.js that we did some time ago and I made an updated version of the library, with some bugfixes and general cleanup. I seem to recall there being something funky with the type conversion code we wanted to fix. You can find it here: https://github.com/Axinom/x2js

The latest xml2json available from the internet was pulled in and any custom changes migrated. The modifications tried to preserve existing functionality but compatibility with anything except our own implementation was not a goal and the API was changed somewhat during the code cleanup. It should still contain all the custom added features from dash.js but I cannot state that for certain.

I have heard of (unconfirmed) problems with the conversion speed from some industry peers. No idea if our fork does anything to fix this, though. Maybe give it a try @defkrie and see what happens if you put it into dash.js. Does the original PR you made fix your performance issues? You mentioned a difference of a third - is that substantial enough to matter?

Ah, it looks like the one you linked in the first post is from an even newer codebase of the original maintainer than our fork.

[defkrie]

To try i've replaced in old version of hasplayer, library X2JS with new one provided at the url https://github.com/Axinom/x2js.
I've updated constructor converter = new X2JS(matchers, "", true),
by
converter = new X2JS({attributeConverters : matchers, attributePrefix : "", ignoreRoot : true}),
but i have a manifest parsing error.
The manifest generated looks different. For example minBufferTime "PT1.5S" instead of minBufferTime: 1.5

[FranklinWaller]

We suffer from the same problem, is upgrading the library something that is on the roadmap? Since we can't support Edge as referenced by #1526

[davemevans]

We suffer from the same problem

Which problem? slow parsing (#1267 (comment)) or incorrect type conversion (#1267 (comment)), or something else?

Updating the library to either the version in the first comment or the Axinom version will not fix the problem seen in #1526 since we would need to re-add the code which appears to be causing the problem in Edge (the type conversion stuff).

[bwidtmann]

Hi all,

I have to roll up this issue again.

I tested the latest version of xml2json of abdmob and experienced a big increase in speed. Here are the results in Chrome on my very fast mac book:

abdmob latest: 80ms
dash.js 2.3.0 (WITHOUT any custom matchers): 1500ms

Especially on low CPU devices, you end up with a couple of seconds just parsing the manifest, which is very annoying.

I have to agree to @bbcrddave that it heavily depends on the size of your manifest. So with perfect crafted manifests you do not really gain significant benefits with upgrading. BUT in real world you do not have always the opportunity to keep your manifest as small as possible.

One of our new distribution platforms forces us using very big manifests because of poor mobile 3rd party player support. So we have to choose between delivering different manifests or optimizing the parsing on the client.

Although I noticed that it could end up with some major effort to update the library in dash.js, I really think this is a vital improvement to increase the dash.js platform support.

@FranklinWaller #1526 is fixed and merged in latest dev branch. So you should not have any problems with supporting Edge anymore.

[wilaw]

@bwidtmann - what is adbmob - can you provide more details? FYI- we have moved this bug out of backlog and assigned to 2.4

[bwidtmann]

@wilaw sorry, there was a misspelling. I mean abdmob - the version of xml2json provided here: https://github.com/abdmob/x2js/blob/master/xml2json.js

[davemevans]

I had a first attempt to integrate xml2json 1.2.0. See results at https://github.com/bbcrddave/dash.js/tree/1267. I took a quick look at the Axinom-maintained version but concluded it was not going to be something we could pull in as a dependency without changes to both that and our code, so just went with the existing library.

It does seem to perform better for large manifests. The output is not identical to 1.0.11, particularly around __text nodes, but it is fairly close and seems to work with the handful of manifests I have tested in the reference player. I have also enabled a few other new features which make the output more sane which helps bring down the objectiron time with some manifests.

I'd encourage those who want this upgrade to happen to grab this, test it and provide feedback - this is core functionality and we need to be sure the new version and its configuration it is 100% solid before we can consider merging.

EDIT: I've just noticed that this will break XLink resolution and TTML parsing since those rely on X2JS too. I'll take a look at those too. Fixed in latest commit.

[bwidtmann]

@bbcrddave awesome! manifest parsing has come down from 2.5s to 250ms. Especially on low performance set top boxes and Smart TVs we experience a significant gain in video startup time even with small manifests.

So far I did not see any issues with our content. I will cherry pick your commit and deliver it to our internal QA so we can get better feedback.

[davemevans]

@bwidtmann let us know the feedback from your QA people and I will create a PR if it's all good.

[davemevans]

@defkrie Is this something you are still interested in? Have you tested the fork in #1267 (comment)?

[bwidtmann]

@bbcrddave regression tests did not bring up any issues so it did not break anything at least with our content.