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

IE11 internal error in stream-controller.js #2337

Closed
stevendesu opened this issue Aug 16, 2019 · 12 comments
Closed

IE11 internal error in stream-controller.js #2337

stevendesu opened this issue Aug 16, 2019 · 12 comments
Labels

Comments

@stevendesu
Copy link

stevendesu commented Aug 16, 2019

Update

I've tracked the issue and left all of my original debugging notes here just in case they're of value to anyone. Ultimately the problem is: Internet Explorer does not support Number.isFinite (which is used in a few places in the HLS.js source code). A polyfill for this function is necessary for IE 11 support.

A simple polyfill like Number.isFinite = isFinite is insufficient because Number.isFinite and isFinite treat null differently.

Original post:

I noticed a video of mine was not loading in IE11 so I traced the issue through the debugger and came upon line 265 of stream-controller.js in the method _fetchPayloadOrEos:

        logger.log(`Loading ${frag.sn} of [${levelDetails.startSN} ,${levelDetails.endSN}],level ${level}, currentTime:${pos.toFixed(3)},bufferEnd:${bufferEnd.toFixed(3)}`);

In Internet Explorer 11, when I reach this line, bufferEnd is null causing an error to be thrown by the log and _loadFragment to never run. The variable bufferEnd is populated from bufferInfo.end, which is passed in as a parameter from _doTickIdle, which builds the variable like so:

    const bufferInfo = BufferHelper.bufferInfo(this.mediaBuffer ? this.mediaBuffer : media, pos, config.maxBufferHole),

Tracing this it looks like bufferEnd is only set (in buffer-helper.js) if media.buffered has a non-zero count of items. My guess is that in normal browsers this is likely true (perhaps it defaults to a single "buffered" object from 0 seconds to 0 seconds) but Internet Explorer may behave differently. Further research is necessary to narrow this down.

@stevendesu
Copy link
Author

Upon updating my node_modules/hls.js/src/controllers/stream-controller.js to add the following (for testing purposes):

if (bufferEnd === null) bufferEnd = 0;

I noticed that pos was also null. Adding:

if (pos === null) pos = 0;

allowed my video to load an play as expected. I should also note that it's possible the issue lies in the way that I'm loading HLS.js -- directly linking the source instead of using the compiled output. Perhaps you have a build step which amends this line so it works as expected in Internet Explorer.

@stevendesu
Copy link
Author

Further investigation:

I noticed that bufferEnd and pos were getting set to null because the call to BufferHelper.bufferInfo was passing null as the second parameter. This second parameter is set in stream-controller.js around line 150 via:

    // if we have not yet loaded any fragment, start loading from start position
    let pos;
    if (this.loadedmetadata) {
      pos = media.currentTime;
    } else {
      pos = this.nextLoadPosition;
    }

In the case of Internet Explorer when I reached this line I noticed that media.currentTime was properly set to 0 but this.nextLoadPosition was null. The fact that this.loadedmetadata was null also struck me as a bit odd, because it correlated to another issue I noticed in IE that I haven't investigated yet: my media is never throwing a "loadedmetadata" event (which means that my autoplay code isn't working, as I wait for "loadedmetadata")

Looking into the HLS.js variable, I noticed that loadedmetadata gets set to true in the following function:

  _checkBuffer () {
    const { media } = this;
    if (!media || media.readyState === 0) {
      // Exit early if we don't have media or if the media hasn't bufferd anything yet (readyState 0)
      return;
    }

    const mediaBuffer = this.mediaBuffer ? this.mediaBuffer : media;
    const buffered = mediaBuffer.buffered;

    if (!this.loadedmetadata && buffered.length) {
      this.loadedmetadata = true;
      this._seekToStartPos();
    } else if (this.immediateSwitch) {
      this.immediateLevelSwitchEnd();
    } else {
      this.gapController.poll(this.lastCurrentTime, buffered);
    }
  }

In the case of Internet Explorer, this function was exiting early because media.readyState was 0. Perhaps the readyState variable works differently in Internet Explorer than it does in other browsers? Investigation is on-going.

@stevendesu
Copy link
Author

Looks like readyState is behaving as intended (without HLS.js the browser doesn't know how to load an M3U8, so it makes sense it should remain in readyState 0 until HLS.js has put some data in the buffer) but the issue seem to lie in the value of this.nextLoadPosition.

On line 78 this gets properly set here:

      this.nextLoadPosition = this.startPosition = this.lastCurrentTime = startPosition;

However later this gets improperly set on line 805 here:

      this.nextLoadPosition = this.startPosition;

Somehow between these two lines getting run the value of this.startPosition was updated from -1 to null. I'm looking into how this happened right now.

@stevendesu
Copy link
Author

Continuing my investigation it looks like the major difference actually slipped in through a polyfill I had created:

// HLS.js uses "Number.isFinite", whereas IE only supports "isFinite"
if (!Number.isFinite && isFinite)
{
	Number.isFinite = isFinite;
}

These two functions (Number.isFinite and isFinite) are almost identical except for one small quirk:

console.log(Number.isFinite(null)); // false
console.log(isFinite(null)); // true

I updated my polyfill like so:

// HLS.js uses "Number.isFinite", whereas IE only supports "isFinite"
if (!Number.isFinite && isFinite)
{
	Number.isFinite = function(x)
	{
		if (x === null) { return false; }
		return isFinite(x);
	};
}

So after a couple of hours of digging it looks like the root cause was:

Issue with IE 11

Internet Explorer 11 doesn't support Number.isFinite, which is utilized in multiple places throughout the HLS.js source code. A very simple polyfill for this method will fix it.

@itsjamie
Copy link
Collaborator

itsjamie commented Aug 18, 2019

Number.isFinite is polyfilled here.

export const isFiniteNumber = Number.isFinite || function (value) {
return typeof value === 'number' && isFinite(value);
};

I should also note that it's possible the issue lies in the way that I'm loading HLS.js -- directly linking the source instead of using the compiled output. Perhaps you have a build step which amends this line so it works as expected in Internet Explorer.

There is a build-step that goes through and replaces the usage with alternative usage.
See:

hls.js/webpack.config.js

Lines 76 to 81 in f723c06

visitor: {
CallExpression: function (espath) {
if (espath.get('callee').matchesPattern('Number.isFinite')) {
espath.node.callee = importHelper.addNamed(espath, 'isFiniteNumber', path.resolve('src/polyfills/number-isFinite'));
}
}

Rather than checking null it explicitly checks that the passed value is a Number type. Which functionally would achieve the return false.

If that fixes your issue, I think using the compiled output would solve your issue. Hope this helps!

@stevendesu
Copy link
Author

Perhaps instead of a build step (which obfuscates the solution somewhat and more tightly couples your code with your build environment) it would be better to import the custom Number.isFinite polyfill anywhere you use it? Something like:

const numberIsFinite = require("../polyfills/number-isFinite");

// ...

if (numberIsFinite(pos))
{
    // ...
}

This would allow you to remove the visitor helper from your webpack config.

@itsjamie
Copy link
Collaborator

Perhaps, but the visitor helper prevents us from accidentally allowing IE11 incompatible code from being in the built bundle which was an issue.

@itsjamie
Copy link
Collaborator

Curious, why are you directly using source versus the built bundle?

Also, if it's a choice, can you switch to the built bundle?

@stevendesu
Copy link
Author

I’m building from source to reduce bundle size as described here:

#2316

Instead of a build step to replace Number.isFinite with your polyfill, why not add a unit test to avoid accidentally calling the non-IE-compatible version?

@itsjamie
Copy link
Collaborator

I see!

That makes sense. I'd be happy to accept adding additional build parameters and defining them via CLI params for the build step so you can build your custom package with a lighter bundle size. I think that's a much simpler stepping stone to the eventual goal of the plugin system.


I don't think it's likely we would switch away from the current technique for handling the polyfill. I believe it was put into place quite recently so that future code that is written won't accidentally break IE compat.

Since it sounds like you already have a working solution for the custom build do you think you could submit a pull request with your additional defines?

Thank you!

@stevendesu
Copy link
Author

I could, but it would take quite a long time. I’m not very familiar with the HLS.js source yet so I’d have to do a lot of digging and reading to figure out everywhere the conditionals were needed.

@itsjamie
Copy link
Collaborator

No problem. If you find the time to do so, would be happy to accept!

littlespex pushed a commit to cbsinteractive/hls.js that referenced this issue Dec 9, 2022
Now that video-dev#2337 has been implemented, the error code HLS_INTERNAL_SKIP_STREAM
is no longer used anywhere in the code.
This retires that error, and also cleans up the code that previously
was responsible for handling that error being fired.

Pre-work for video-dev#1936
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants