Fix: remove player props on dispose to stop middleware #229

brandonocasey · 2018-09-13T18:13:30Z

After the tech/vhs is disposed the middleware can still end up running for the next source because player.vhs still exists and that is what the middleware looks for. See: https://github.com/videojs/http-streaming/blob/master/src/middleware-set-current-time.js

welcome · 2018-09-13T18:13:39Z

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

Run npm run lint -- --errors locally to catch formatting errors earlier.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

brandonocasey · 2018-09-18T18:04:06Z

test/test-helpers.js

-    swfId: player.tech_.el().id
-  });
-  clock.tick(1);
+  if (player.tech_.hls) {


This helper assumed that hls would always be on tech, which this pr gets rid of.

Won't it only get rid of it on dispose though?

Yeah it should only get rid of it on dispose, but that will happen between sources as well.

forbesjo · 2018-09-21T17:18:54Z

test/karma.conf.js

@@ -25,10 +25,10 @@ module.exports = function(config) {
      'node_modules/video.js/dist/video-js.css',
      'dist-test/videojs-http-streaming.test.js'
    ],
-    browserConsoleLogOptions: {
+    /*browserConsoleLogOptions: {


commented code

gesinger · 2018-09-21T17:17:18Z

test/karma.conf.js

@@ -25,10 +25,10 @@ module.exports = function(config) {
      'node_modules/video.js/dist/video-js.css',
      'dist-test/videojs-http-streaming.test.js'
    ],
-    browserConsoleLogOptions: {
+    /*browserConsoleLogOptions: {


Was this left in?

gesinger · 2018-09-21T17:18:21Z

test/test-helpers.js

-    swfId: player.tech_.el().id
-  });
-  clock.tick(1);
+  if (player.tech_.hls) {


Won't it only get rid of it on dispose though?

brandonocasey · 2018-10-25T18:26:32Z

This has been on the back burner for awhile, but I picked it pack up and got the tests working! Would be good if you can take another look when you get a chance @gesinger

brandonocasey · 2018-10-25T18:26:58Z

src/mse/html-media-source.js

@@ -252,6 +256,9 @@ export default class HtmlMediaSource extends videojs.EventTarget {
      // event handlers left to unbind anyway
      if (this.player_.el_) {
        this.player_.off('mediachange', this.onPlayerMediachange_);
+      }
+
+      if (this.player_.tech_ && this.player_.tech_.el_) {


in some cases the techs element would already be gone at this point.

brandonocasey · 2018-10-25T18:27:51Z

src/master-playlist-controller.js

@@ -969,6 +969,9 @@ export class MasterPlaylistController extends videojs.EventTarget {
    this.mainSegmentLoader_.dispose();

    ['AUDIO', 'SUBTITLES'].forEach((type) => {
+      if (!this.mediaTypes_[type]) {


This wasn't causing the test failures, but an error was printing for it in a lot of the tests.

That's odd. Do you know what the error was? It seems like both of those media types should exist if an instance of MasterPlaylistController exists.

A syntax error about media types[type] not existing.

Did we ever discover what might have been causing it? I'm always a bit hesitant to add code to fix a test error.

Perhaps this https://github.com/videojs/http-streaming/blob/master/test/videojs-http-streaming.test.js#L2884 ? I'll investigate and update the test if that's the problem

Confirmed, updating

forbesjo · 2018-11-30T18:13:23Z

src/videojs-http-streaming.js

+
+    if (this.player_) {
+      this.player_.vhs = null;
+      this.player_.dash = null;


There's also player.hls

gesinger · 2018-12-06T21:39:22Z

src/videojs-http-streaming.js

@@ -325,7 +325,8 @@ class HlsHandler extends Component {
            videojs.log.warn('player.hls is deprecated. Use player.tech().hls instead.');
            tech.trigger({ type: 'usage', name: 'hls-player-access' });
            return this;
-          }
+          },
+          configurable: true


What's this property for?

By default configurable is false, means you can't change the value once set.

change including delete.

gesinger · 2018-12-06T21:39:52Z

src/master-playlist-controller.js

@@ -969,6 +969,9 @@ export class MasterPlaylistController extends videojs.EventTarget {
    this.mainSegmentLoader_.dispose();

    ['AUDIO', 'SUBTITLES'].forEach((type) => {
+      if (!this.mediaTypes_[type]) {


Did we ever discover what might have been causing it? I'm always a bit hesitant to add code to fix a test error.

welcome · 2018-12-06T22:07:09Z

Congrats on merging your first pull request! 🎉🎉🎉

brandonocasey force-pushed the fix/remove-player-props branch 2 times, most recently from 1531750 to 49aeab8 Compare September 18, 2018 17:58

brandonocasey commented Sep 18, 2018

View reviewed changes

forbesjo reviewed Sep 21, 2018

View reviewed changes

gesinger reviewed Sep 21, 2018

View reviewed changes

gesinger previously approved these changes Oct 22, 2018

View reviewed changes

brandonocasey dismissed gesinger’s stale review via 682300c October 25, 2018 17:42

brandonocasey force-pushed the fix/remove-player-props branch 2 times, most recently from cb2578d to c22b291 Compare October 25, 2018 18:18

brandonocasey commented Oct 25, 2018

View reviewed changes

forbesjo mentioned this pull request Nov 30, 2018

hls property not removed from tech on disposal #58

Closed

forbesjo reviewed Nov 30, 2018

View reviewed changes

forbesjo added the needs updates label Nov 30, 2018

brandonocasey and others added 8 commits December 6, 2018 15:53

fix: remove player props on dispose

cacc268

fix: check for tech_ before removing event

b6e99a4

fix: tests

d8db404

fix tests

83024e0

woops

818efca

attempt to fix the tests

3f0fb38

null out hls

be7ba58

delete properties on dispose

b4826b7

forbesjo force-pushed the fix/remove-player-props branch from b646550 to b4826b7 Compare December 6, 2018 21:36

forbesjo removed the needs updates label Dec 6, 2018

gesinger reviewed Dec 6, 2018

View reviewed changes

update tests to avoid property access error

757b015

gesinger approved these changes Dec 6, 2018

View reviewed changes

forbesjo merged commit cd13f9f into master Dec 6, 2018

forbesjo deleted the fix/remove-player-props branch December 6, 2018 22:07

yokuze mentioned this pull request Jan 1, 2019

Seeking video location is not working silvermine/videojs-chromecast#32

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: remove player props on dispose to stop middleware #229

Fix: remove player props on dispose to stop middleware #229

brandonocasey commented Sep 13, 2018

welcome bot commented Sep 13, 2018

brandonocasey Sep 18, 2018

gesinger Sep 21, 2018

brandonocasey Oct 12, 2018

forbesjo Sep 21, 2018

gesinger Sep 21, 2018

gesinger Sep 21, 2018

brandonocasey commented Oct 25, 2018

brandonocasey Oct 25, 2018

brandonocasey Oct 25, 2018

gesinger Nov 2, 2018

brandonocasey Dec 6, 2018

gesinger Dec 6, 2018

forbesjo Dec 6, 2018

forbesjo Dec 6, 2018

forbesjo Nov 30, 2018

gesinger Dec 6, 2018

gkatsev Dec 6, 2018

gkatsev Dec 6, 2018

gesinger Dec 6, 2018

welcome bot commented Dec 6, 2018

Fix: remove player props on dispose to stop middleware #229

Fix: remove player props on dispose to stop middleware #229

Conversation

brandonocasey commented Sep 13, 2018

welcome bot commented Sep 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey commented Oct 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

welcome bot commented Dec 6, 2018