Dash-Industry-Forum / dash.js

A reference client implementation for the playback of MPEG DASH via Javascript and compliant browsers.
http://reference.dashif.org/dash.js/nightly/samples/dash-if-reference-player/index.html
Other
5.09k stars 1.67k forks source link

Javascript stalls due to emsg id handling #561

Closed TobbeMobiTV closed 9 years ago

TobbeMobiTV commented 9 years ago

The browser stalls for big ids in emsg.

Events are added with its id as a index in an array in the addInbandEvents function in EventController.js.

    addInbandEvents = function(values) {
        var self = this;
        for(var i=0;i<values.length;i++) {
            var event = values[i];
            inbandEvents[event.id] = event;
            self.log("Add inband event with id "+event.id);
        }
    },

In the triggerEvents function, there is then a loop

        if(events) {
            for (var j = 0; j < events.length; j++) {
                var curr = events[j];

which runs from 0 to the events.length, but in this case it becomes id due to the bad indexing.

This can be tested by using the dash-live-source-simualator with SCTE-35 Splice Insert events http://vm2.dashif.org/livesim-dev/scte35_2/testpic_2s/Manifest.mpd

The emsg ids are huge, such as 716411340, since they are based on time of the events.

A suggested fix is to change the triggerEvents lines to:

        if(events) {
            for (var j in events) {
                var curr = events[j];

The same issue exists in removeEvents.

With these changed it worked to stream with the SCTE-35 events without any stalls.

dsilhavy commented 9 years ago

@TobbeMobiTV Thanks for pointing that out, it was poorly implemented by me. For some reason I expected the length to be one, regardless where you insert a single element. The reason I used the id as an index is because the same event might be signaled multiple times. That way it was easy to just check if the event already exist. Can you issue a pull request for your fix or do you want me to do that? Or maybe @AkamaiDASH or @KozhinM have also a suggestion on that? Best regards Daniel

TobbeMobiTV commented 9 years ago

I saw your motivation in a comment. The length property of arrays in Javascript is sort of pathological... I'll issue a pull request that you and others can review.

TobbeMobiTV commented 9 years ago

Pull-request has been accepted, so I close this issue.