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

fix(player): techGet is undefined #8256

Merged
merged 1 commit into from
May 31, 2023

Conversation

amtins
Copy link
Contributor

@amtins amtins commented Apr 30, 2023

Description

This PR fixes #8255 the call to techGet_.

Specific Changes proposed

  • rename techGet to techGet_
  • add test case

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@mister-ben
Copy link
Contributor

I'm not sure why this causes the test failure...

@mister-ben mister-ben added the needs: LGTM Needs an additional approval label May 12, 2023
@amtins
Copy link
Contributor Author

amtins commented May 12, 2023

@mister-ben I don't know why it causes the tests to fail, especially since locally I didn't have any issue. I was tempted to delete the tests 😅, but it seemed important to add the test coverage.

I wonder if it's a timer issue, a clock.tick missing, but it still seems strange.

misteroneill
misteroneill previously approved these changes May 12, 2023
Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

Kind of amazing this hasn't been encountered yet.

@misteroneill misteroneill dismissed their stale review May 12, 2023 16:30

Approved before I noticed its test was failing... change looks good, though.

@amtins
Copy link
Contributor Author

amtins commented May 14, 2023

@mister-ben, @misteroneill thank you for taking the time to review this PR.

The test added in this PR seems to produce a timeout in videojs-integration.test.js by side effect. Moving the test case from sourceset.test.js to player.test.js, seems to fix the problem. I'm not sure what is causing the timeout in videojs-integration.test.js and changing the assert.timeout doesn't help.

Error message

Chrome Headless 112.0.5615.165 (Linux x86_64) videojs-integration create a real player and dispose FAILED
	Test took longer than 30000ms; test timed out.

So there are two options, delete the test case if it is not useful or move it to player.test.js. However, from my point of view, if this test really makes sense, it would be better in sourceset.test.js.

Finally, while writing the unit test for this fix I came across a behavior that I'm not quite sure I understand. It's all in the handleTechSourceset_ and updateSourceCaches_ method.

Demonstration

player.src('https://d2zihajmogu5jn.cloudfront.net/bipbop-advanced/bipbop_16x9_variant.m3u8');

player.cache_.src;
// "https://d2zihajmogu5jn.cloudfront.net/bipbop-advanced/bipbop_16x9_variant.m3u8"
player.cache_.source;
// { src: "https://d2zihajmogu5jn.cloudfront.net/bipbop-advanced/bipbop_16x9_variant.m3u8", type: "application/x-mpegURL" }
player.cache_.sources;
// [{ src: "https://d2zihajmogu5jn.cloudfront.net/bipbop-advanced/bipbop_16x9_variant.m3u8", type: "application/x-mpegURL" }]

player.load();

player.cache_.src;
// "blob:http://localhost:9999/a1dc9574-404a-47a2-9d00-1ff7b0949abb"
player.cache_.source
// { src: "blob:http://localhost:9999/a1dc9574-404a-47a2-9d00-1ff7b0949abb", type: "" }
player.cache_.sources
//[{ src: "blob:http://localhost:9999/a1dc9574-404a-47a2-9d00-1ff7b0949abb", type: "" }]

I couldn't tell if this is the expected behavior, however it creates a playback issue on Dash and HLS content, see PR #8274

@video-archivist-bot
Copy link

Hey! We've detected some video files in a comment on this issue. If you'd like to permanently archive these videos and tie them to this project, a maintainer of the project can reply to this issue with the following commands:

@gkatsev
Copy link
Member

gkatsev commented May 14, 2023

Yeah, calling load() with VHS enabled is known to be broken right now. I've long thought that we should forward the call to the tech and then the source handler, but your PR seems like a good workaround.

@amtins amtins force-pushed the fix/player-techget-undefined branch from cf39dee to c25cbdd Compare May 18, 2023 10:10
@codecov
Copy link

codecov bot commented May 18, 2023

Codecov Report

Merging #8256 (c25cbdd) into main (1491d71) will increase coverage by 82.40%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##           main    #8256       +/-   ##
=========================================
+ Coverage      0   82.40%   +82.40%     
=========================================
  Files         0      112      +112     
  Lines         0     7483     +7483     
  Branches      0     1804     +1804     
=========================================
+ Hits          0     6166     +6166     
- Misses        0     1317     +1317     
Impacted Files Coverage Δ
src/js/player.js 90.47% <100.00%> (ø)

... and 111 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@amtins
Copy link
Contributor Author

amtins commented May 18, 2023

@mister-ben, @misteroneill I fixed the unit test, I forgot to put a clock.restore 😅

I wonder if it's a timer issue, a clock.tick missing, but it still seems strange.

Copy link
Contributor

@adrums86 adrums86 left a comment

Choose a reason for hiding this comment

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

:shipit: !

@misteroneill misteroneill merged commit 5151bc5 into videojs:main May 31, 2023
11 checks passed
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: LGTM Needs an additional approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in player, techGet is undefined and should be techGet_ in handleTechSourceset_ method
6 participants