Multiple calls to EventController.start without EventController.reset/.clear causes uncaught TypeError

[davemevans]

EventController has an interval which is created when .start() is called, regardless of whether an interval already exists. If .start() is called multiple times without a .clear() or .reset(), the existing interval is not cleared and therefore runs forever as its handle is lost.

When the EventController is reset, its state is lost but the existing interval still attempts to access that state (playbackController specifically), causing an uncaught TypeError.

This can be reproduced easily by seeking during playback when autoplay is enabled, or reloading the manifest once the presentation has been completed.

[dsparacio]

@bbcrddave attempted to reproduce it but was not able to. Do you have an mpd that produces this issue?

Does this happen in 1.5.1 and /or 1.6.0 ?

[davemevans]

Can reproduce using several manifests, try: http://dash.edgesuite.net/dash264/TestCases/1a/netflix/exMPD_BIP_TC1.mpd

In the reference player: start playing, seek around a few times, click load to reload manifest.

The problem existed in a similar way in 1.5.1 (many calls to start without a clear/reset), but the required state (videoModel) was a singleton injected by dijon so would always be available.

[dsilhavy]

@bbcrddave @AkamaiDASH I was not able to reproduce the uncaught TypeError in the current dev branch using the manifest Dave mentioned. I tried seeking and reloading the manifest after the presentation has finished, but works fine for me. Has this been fixed in the meantime?

[davemevans]

Looks like 436ccc9 has changed the behaviour. Now there are zero calls to EventController.start as far as I can see, so it never starts! Do you see this @dsilhavy?

[dsilhavy]

@bbcrddave You are right it looks like the EventController is not started at all. I tried to implement a solution for both problems. Maybe you can tell me if you agree:
fraunhoferfokus@4efab9f

Since the EventController is a singleton I think we should trigger the interval only once when the playback is started and remove it when the playback has finished. Still wondering if I missed something regarding multiperiod issues.

[davemevans]

I don't think this will work quite right in the multi-period case: initialize is called each time a new stream is activated which will keep adding event handlers? eventBus.on should either be called in a one-off setup method when the instance is created, or there should be a corresponding eventBus.off. What do you think?

This has reminded me that we really could do with implementing some method of synchronisation with the media timeline here, since currently events will continue to be fired despite the media time not advancing if the player is paused for example. That's for a different day though 😉

[davemevans]

Thinking about this some more, any remaining events should be cleared at the end of each period, so perhaps the controller should be reset between periods.

In the short term we should probably get this fixed in the simplest way - something like what you suggest - then look to fix this properly in a later version.

[dsilhavy]

Thanks Dave. I agree for now we should focus on a simple fix. I will try to take a more detailed look at this in the next few days if you haven´t done already?

[dsparacio]

@dsilhavy I wanted to see if you have a chance to look at this after pulling HEAD of dev.

@bbcrddave You are right it looks like the EventController is not started at all. I tried to implement a solution for both problems. Maybe you can tell me if you agree:

I think this comment is resolved after I changed the autoplay logic. But maybe if autoplay is off the event controller will not start without the change you have @fraunhoferfokus4efab9f

[dsilhavy]

@AkamaiDASH @bbcrddave
Hi guys,

• EventController should start even with autoplay set to false, because the startAutoPlay() function of the StreamController is triggered anyways.
• I added a flag in the EventController which should prevent from starting multiple times (for instance after MPD refresh). Also the EventController is reset and started again every time we switch from one period to another (StreamController.switchStream).
• EventController will now throw custom events. An app can use the schemeIdUri to register for such an event.
• I refactored the ad insertion sample sites. Since multiperiod support is still a bit buggy I cant really test it though.

I will issue a pull request tomorrow since this is the deadline. But I was wondering if you have any feedback on the EventController stuff. You can find it here:
https://github.com/fraunhoferfokus/dash.js/blob/61030bb8fbd09831890303b51f8562a2052479a8/src/streaming/controllers/EventController.js
Thank you

[dsparacio]

Thanks for the update Let me check out the diff on your fork and ill add comment if I have any. You overview seems like the right way....

[dsparacio]

@dsilhavy Regarding -

EventController should start even with autoplay set to false, because the startAutoPlay() function of the StreamController is triggered anyways.

This is the desired behavior correct? I think it stopped after 436ccc9 and started working this way again after this change. e75e65e

Your other changes seem correct.

[dsilhavy]

@AkamaiDASH I think this is ok for now everything should work as expected

[davemevans]

LGTM 👍

[dsparacio]

closing flixed with PR #1120