Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move STREAM_TEARDOWN_COMPLETE to MediaPlayerEvents #2038

Merged

Conversation

AxelDelmas
Copy link

Fix #2005

I could also rename this event to something more explicit like PLAYER_RESET if necessary

@dsparacio dsparacio merged commit 52470bd into Dash-Industry-Forum:development Jun 22, 2017
@nicosang
Copy link
Contributor

nicosang commented Jul 6, 2017

Hi @AxelDelmas

there is a little problem with this PR when we use the different modules of dash.js (mediaplayer, mss, protection and reporting) instead of dash.all. Specially in the reporting module, the MetricsCollectionController class is registered on the STREAM_TEARDOWN_COMPLETE event which was initially defined in the core events. This module doesn't include MediaPlayerEvents.....my question is why do you need to move this event from Core to MediaPlayerEvents? By the way, in mediaPlayer module, MediaPlayerEvents are added to core events...

Nicolas

@dsparacio
Copy link
Contributor

@nicosang it should not matter all events are aggregated into EVENTS.js . You should only be calling Events.js in internal code. Regardless where const is defined, in Core or MediPlayer or any other event class for that matter.

Events.EVENT_NAME should get you access to ALL events.

@davemevans
Copy link
Contributor

Events.EVENT_NAME should get you access to ALL events.

Unfortunately that isn't always true.

It depends on when the code referencing Events.EVENT_NAME is run. In the case @nicosang points out (ie where there is a "plugin" attached as separate js file - see example at bottom), the aggregation may not have run before the event is referenced which leads to an error being thrown. There is no way for plugins to hold off initialisation until this point, but the issue was masked by having the event in Core.

This could be fixed in a variety of ways, but really there is probably a need to sort out the plugin system in a more generic way.

e.g:

    <script src="../../dist/dash.mediaplayer.debug.js"></script>
    <script src="../../dist/dash.reporting.debug.js"></script>

@AxelDelmas AxelDelmas deleted the stream-teardown-complete branch September 13, 2017 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants