yanwsh / videojs-panorama

a plugin for videojs run a full 360 degree panorama video.
http://yanwsh.github.io/videojs-panorama/
Other
485 stars 160 forks source link

"Cannot read property 'paused' or 'classList' of null" when disposing and the removing video element #76

Open slementin opened 7 years ago

slementin commented 7 years ago

Hello!

When disposing of a panorama video and then removing the associated element in my ionic 2 application I am getting two exceptions in the console:

Cannot read property 'paused' of null

And

Cannot read property 'classList' of null

I have read about similar exceptions on the video-js github (this one for example: https://github.com/videojs/video.js/issues/1505). In those issues the problem seemed to be that all the property listeners aren't being removed properly and are still trying to access the video after dispose.

I am also using regular video-js videos in the same application and those don't seem to have this problem. The exceptions are only being thrown if the video has been played. Also, the "classList" one only gets thrown sometimes.

This is the component that takes care of the all the videos in the application:

video-player.zip

Let me know if you need any more information!

Thanks

kuehn-sba commented 7 years ago

+1 i ran into same issues...videojs is working fine, but the exactly described problems occurs by using videojs-panorama.

it seems cancelAnimationFrame is not called for this.animate on release.

Setup:

noticed in chrome, firefox console (mac)...

yanwsh commented 7 years ago

Looks like it's friendly to single page application, will work on a fix. Nice catch, Thank you.

kuehn-sba commented 7 years ago

small fix for videojs-panorama.v5.js (added dispose + cancelAnimationFrame + detachControlEvents function inside BaseCanvas):

L464
            this.player().on("play", function () {
                this.time = new Date().getTime();
            cancelAnimationFrame(this.requestAnimationId); // necessary ! 
                this.animate();
            }.bind(this));
L643
        el: function el() {
            return this.el_;
        },

       // it would be fine to detach any event listening in dispose too:
        detachControlEvents: function detachControlEvents() {
            this.off('mousemove', this.handleMouseMove);
            this.off('touchmove', this.handleTouchMove);
            this.off('mousedown', this.handleMouseDown);
            this.off('touchstart', this.handleTouchStart);
            this.off('mouseup', this.handleMouseUp);
            this.off('touchend', this.handleTouchEnd);
            if (this.settings.scrollable) {
                this.off('mousewheel', this.handleMouseWheel);
                this.off('MozMousePixelScroll', this.handleMouseWheel);
            }
            this.off('mouseenter', this.handleMouseEnter);
            this.off('mouseleave', this.handleMouseLease);
        },

    dispose: function dispose() {
        this.detachControlEvents();
        cancelAnimationFrame(this.requestAnimationId);
    }
kuehn-sba commented 7 years ago

@yanwsh: pls, can you fix overall memory leaks? I am missing overall plugins dispose-functions in connection with THREE remove / dispose and nulling of 'new'ed THREE elements.

regardless of this, it seems WebGL context:

WARNING: Too many active WebGL contexts. Oldest context will be lost.
THREE.WebGLRenderer @ three.js:24593
init @ BaseCanvas.js:32
init @ Canvas.js:54
addChild @ video.js:1130
onPlayerReady @ plugin.js:186
(anonymous) @ plugin_v5.js:32
(anonymous) @ video.js:1569
(anonymous) @ video.js:1568
bound @ video.js:22721

can only be removed by:

this.renderer.forceContextLoss();
this.renderer.context = null;
this.renderer.domElement = null;
this.renderer.dispose();
this.renderer = null;
yanwsh commented 7 years ago

Hello, this should be fixed on the latest version, please run npm update and make sure it fixed. Let me know if you have any questions. Thank you

kuehn-sba commented 7 years ago

thx, but you have not take over that release code, like e.g. shown below?:

baseCanvase dispose:

        dispose: function dispose() {
            this.detachControlEvents();  //  This has already been implemented by you on dipsose handling
            cancelAnimationFrame(this.requestAnimationId);

            this.texture.dispose();
            this.texture = null;

            this.renderer.forceContextLoss();
            this.renderer.context = null;
            this.renderer.domElement = null;
            this.renderer.dispose();
            this.renderer = null;

            this.el_.classList.remove('vjs-video-canvas');
            this.el_.remove();
            this.el_ = null;
        }

Canvas dispose:

        dispose: function() {
            parent.dispose.call(this);

            this.scene.remove(this.mesh);
            this.geometry.dispose();
            this.geometry = null;

            this.mesh = null;

            this.cameraL = null;
            this.cameraR = null;
            this.camera.target = null;
            this.camera = null;
            this.scene = null;
        }

ThreeDCanvas dispose:

        dispose: function() {
            parent.dispose.call(this);

            this.scene.remove(this.meshL);
            this.scene.remove(this.meshR);

            this.meshL = null;
            this.meshR = null;
            this.cameraL.target = null;
            this.cameraR.target = null;
            this.cameraL = null;
            this.cameraR = null;

            this.geometryL.dispose();
            this.geometryR.dispose();
            this.geometryL = null;
            this.geometryR = null;

            this.scene = null;
        }