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

Test/ff 83 #1004

Merged
merged 4 commits into from
Nov 24, 2020
Merged

Test/ff 83 #1004

merged 4 commits into from
Nov 24, 2020

Conversation

brandonocasey
Copy link
Contributor

@brandonocasey brandonocasey commented Nov 18, 2020

See browserstack results in #1003

test/test-helpers.js Outdated Show resolved Hide resolved
test/test-helpers.js Outdated Show resolved Hide resolved
@@ -44,6 +44,11 @@ jobs:
uses: actions/setup-node@v1
with:
node-version: '${{ steps.nvm.outputs.NVMRC }}'
# turn off the default setup-node problem watchers...
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns off "problem matchers" that add in linter warnings to unchanged files.

* the time to find the cue at
*
* @return {Object|null}
* the found cue or null
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove jsdoc changes, but I already did them, so we might as well keep them.

@@ -416,7 +416,7 @@ export default class SourceUpdater extends videojs.EventTarget {
// IE reports that it supports removeSourceBuffer, but often throws
// errors when attempting to use the function. So we report that it
// does not support removeSourceBuffer.
return !videojs.browser.IE_VERSION && window.MediaSource &&
return !videojs.browser.IE_VERSION && !videojs.browser.IS_FIREFOX && window.MediaSource &&
Copy link
Contributor Author

@brandonocasey brandonocasey Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is one of the fixes that we need. It seems like removeSourceBuffer is failing to work correctly in firefox 83.

@@ -26,6 +17,19 @@ const playFor = function(player, time, cb) {
}, 10);
};

if (player.paused()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually use the play promise. prevents timeouts for playback tests on firefox.


this.mediaSource = new window.MediaSource();

// need to attach the real media source to a video element for the media source to
// change to an open ready state
video.src = URL.createObjectURL(this.mediaSource);
this.video.src = URL.createObjectURL(this.mediaSource);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this helped or not, but we should be doing this cleanup.

assert.ok(true, 'triggered ended event');
done();
});
player.one('ended', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tests still exist here, some were just moved down to be a part of the block that skips tests on firefox.

src/bin-utils.js Outdated
@@ -17,6 +18,7 @@ const textRange = function(range, i) {
*
* @param {number} e The number
* @param {number} i the iterator
* @return {string} the hex formated number as a string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @return {string} the hex formated number as a string
* @return {string} the hex formatted number as a string

Comment on lines 23 to 24
// Catch/silence error when a pause interrupts a play request
// on browsers which return a promise
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth adding in the comment above here (and removing this, since we no longer silence the error)

});
});

QUnit[testFn]('Big Buck Bunny Demuxed av, audio only rendition same as group', function(assert) {
const done = assert.async();
if (!videojs.browser.IS_FIREFOX) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth adding a comment above here

@brandonocasey brandonocasey merged commit 00d9b1d into videojs:main Nov 24, 2020
evanfarina pushed a commit to evanfarina/http-streaming that referenced this pull request Nov 26, 2020
gkatsev pushed a commit that referenced this pull request Dec 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants