search

contently / videojs-annotation-comments

A plugin for video.js to add support for timeline moment/range comments and annotations
https://contently.github.io/videojs-annotation-comments/
Other
171 stars 50 forks source link

Weird issue whenever I change the src of the video #40

Closed tarekgabarin closed 5 years ago

tarekgabarin commented 5 years ago

Hello,

My company is currently testing to see whether this plugin suits our needs. We are seeing if it could be used for a tool in which the user can select between different video clips and annotate each one. However, whenever we change the src attribute of the video, this visual bug comes up...

screen shot 2019-01-03 at 4 09 49 pm

Every time I change the src attribute of the video, a duplicate annotation button is added to the video player. So if I change the src attribute of the video player three times, three notification buttons show up on the controls.

We are integrating it with AngularJS. And we are doing it all within a single page. Is there anyway we can resolve this issue?

jackpope commented 5 years ago

Hi @tarekgabarin, thanks for checking out the plugin! 🎥 💬

I poked around and it appears that changing the src fires the loadeddata on the videojs instance. We have an event listener here: https://github.com/contently/videojs-annotation-comments/blob/master/src/js/annotation_comments.js#L51 This was added to prevent some edge cases where the video meta data wasn't available in time for the plugin to initialize.

This can be changed to use player.one instead of player.on which will remove the listener after it is called the first time. I tried this and I'm able to set the src multiple times without the button appearing again. I don't expect any side effects with this change, as all of this logic is designed for the plugin construction.

While the listener should probably be updated, I'm curious as to why you'd like to swap the src instead of constructing a new videojs instance for each clip. The plugin is designed to allow multiple instances to live on the same page. We've also been pretty diligent about writing cascading teardowns to the ui and listeners so you can remove players in place without lingering functionality or resource usage. In our app, we allow users to upload multiple videos at a time and when each one is done, a videojs instance with our plugin registered appears, ready for annotation. If you want to change the clip being animated, I would suggest tearing down the player and adding a new one with the selected clip. Remember to call dispose() on both the player and the plugin instances for proper teardowns.

Let me know if you have any questions. I'm happy to clarify here or add documentation if you think it's warranted.

jackpope commented 5 years ago

@tarekgabarin any thoughts on this?

jackpope commented 5 years ago

@tarekgabarin Let me know if you're still working on this or have any other questions. I'm going to close the issue now but happy to reopen if needed.