-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Consistent codec-switching test failure on Edge #6458
Comments
Consistent failures are seen in GitHub CI. Running tests in the lab with |
A full test run in the lab passed. The first example of this failure I can spot in the GitHub Actions history is this run testing #6387: There might be an older one, but I haven't found it. The PR isn't obviously related to the failure. |
A slightly older run of the same PR also shows this failure: The overall run was cancelled, but the logs for Edge show the same failure before the cancelation. |
The same failure shows up in some nightly test runs, but not all. Last night it passed after 2 retries: https://github.com/shaka-project/shaka-player/actions/runs/8751808406 The night before it failed every time: Before the pass, it failed 3 nights in a row. There's another long streak of failures before that. I may have been unlucky to have the test pass this morning in my manual run. I will try more repeated runs with a filter, more repeated runs without a filter, and runs with all Windows browsers running at once to slow down the system (as happens in our nightly runs). |
Repeated test runs produced additional failures that may be unrelated flakiness. With
With
|
Running with I see now that this test failure is actually an internal assertion failure in StreamingEngine. The reason it fails on Edge specifically in our test runs is that we use The error 7003 from above seems related. In an uncompiled run, error 7003 appears some of the time (4/20), but only after the assertion fails. |
I discovered that compiled mode masked another problem with these tests. They used the wrong config path So these tests did not test the actual strategies they intended to. |
With the configs fixed, I get assertion failures on three browsers on Windows during the RELOAD test. This shows that the RELOAD test was actually using the SMOOTH strategy before the fix. |
After enabling stack traces for Shaka error objects in the test run, I see the 7003 error (OBJECT_DESTROYED) derives from MediaSourceEngine, where destroyer_.ensureNotDestroyed() is called from an event listener instead of an async function. This results in an uncaught exception if MSE is destroyed before |
The tests were not testing what they were supposed to because their configs were invalid and being ignored. Related to #6458
I found some code in StreamingEngine around reloading MediaSource that appears to be to blame for breaking state tracking in StreamingEngine. Testing a fix now. |
The tests for MediaSourceEngine codec switching were written to ignore types and suppress access controls. The were unreadable, too, with very little whitespace, confusing one-letter variable names, and difficult-to-follow event mocking. This made it more difficult to debug test failures in PR shaka-project#6460. This rewrites the tests in a more readable manner with compiler enforcement of types in the tests. Two helper functions are used to isolate the necessary access-control suppressions. This exposed a bug in the tests, in which one test case (preserve SourceBuffer attributes) only passed because the original version failed to await on an async process. I am not sure that the functionality in that test exists at that level. For now, the test is disabled. I'll follow up with removal after more investigation. Related to shaka-project#6458, shaka-project#6460
The tests for MediaSourceEngine codec switching were written to ignore types and suppress access controls. The were unreadable, too, with very little whitespace, confusing one-letter variable names, and difficult-to-follow event mocking. This made it more difficult to debug test failures in PR shaka-project#6460. This rewrites the tests in a more readable manner with compiler enforcement of types in the tests. Two helper functions are used to isolate the necessary access-control suppressions. This exposed a bug in the tests, in which one test case (preserve SourceBuffer attributes) only passed because the original version failed to await on an async process. I am not sure that the functionality in that test exists at that level. For now, the test is disabled. I'll follow up with removal after more investigation. Related to shaka-project#6458, shaka-project#6460
The tests for MediaSourceEngine codec switching were written to ignore types and suppress access controls. The were unreadable, too, with very little whitespace, confusing one-letter variable names, and difficult-to-follow event mocking. This made it more difficult to debug test failures in PR #6460. This rewrites the tests in a more readable manner with compiler enforcement of types in the tests. Two helper functions are used to isolate the necessary access-control suppressions. This exposed a bug in the tests, in which one test case (preserve SourceBuffer attributes) only passed because the original version failed to await on an async process. I am not sure that the functionality in that test exists at that level. For now, the test is disabled. I'll follow up with removal after more investigation. Related to #6458, #6460
MediaSourceEngine does not, in fact, preserve SourceBuffer properties when we reset it to switch codecs. This is handled by StreamingEngine instead, through a follow-up call to setStreamProperties. The test only ever passed as originally written because there was a missing `await` on an async reset process. This removes the bogus test. Related to shaka-project#6458, shaka-project#6462
The tests for MediaSourceEngine codec switching were written to ignore types and suppress access controls. The were unreadable, too, with very little whitespace, confusing one-letter variable names, and difficult-to-follow event mocking. This rewrites the tests in a more readable manner with compiler enforcement of types in the tests. Two helper functions are used to isolate the necessary access-control suppressions. This exposed a bug in the tests, in which one test case (preserve SourceBuffer attributes) only passed because the original version failed to await on an async process. I am not sure that the functionality in that test exists at that level. For now, the test is disabled. I'll follow up with removal after more investigation. Related to shaka-project#6458
When reloading MediaSourceEngine via StreamingEngine, some logic executed before the reload cancels operations and aligns StreamingEngine state with MediaSourceEngine. However, additional logic was executed after setStreamProperties, which caused a race condition that broke StreamingEngine state by manipulating it out of the usual sequence and outside the usual methods, leading to exceptions and failed assertions. This removes that unnecessary logic and leaves subtle state management to the functions that are meant to do it. Closes shaka-project#6458
When reloading MediaSourceEngine via StreamingEngine, some logic executed before the reload cancels operations and aligns StreamingEngine state with MediaSourceEngine. However, additional logic was executed after setStreamProperties, which caused a race condition that broke StreamingEngine state by manipulating it out of the usual sequence and outside the usual methods, leading to exceptions and failed assertions. This removes that unnecessary logic and leaves subtle state management to the functions that are meant to do it. Closes shaka-project#6458
MediaSourceEngine does not, in fact, preserve SourceBuffer properties when we reset it to switch codecs. This is handled by StreamingEngine instead, through a follow-up call to setStreamProperties. The test only ever passed as originally written because there was a missing `await` on an async reset process. This removes the bogus test. Related to #6458, #6462
When reloading MediaSourceEngine via StreamingEngine, some logic executed before the reload cancels operations and aligns StreamingEngine state with MediaSourceEngine. However, additional logic was executed after setStreamProperties, which caused a race condition that broke StreamingEngine state by manipulating it out of the usual sequence and outside the usual methods, leading to exceptions and failed assertions. This removes that unnecessary logic and leaves subtle state management to the functions that are meant to do it. Closes #6458
The tests were not testing what they were supposed to because their configs were invalid and being ignored. Related to #6458
The tests for MediaSourceEngine codec switching were written to ignore types and suppress access controls. The were unreadable, too, with very little whitespace, confusing one-letter variable names, and difficult-to-follow event mocking. This made it more difficult to debug test failures in PR #6460. This rewrites the tests in a more readable manner with compiler enforcement of types in the tests. Two helper functions are used to isolate the necessary access-control suppressions. This exposed a bug in the tests, in which one test case (preserve SourceBuffer attributes) only passed because the original version failed to await on an async process. I am not sure that the functionality in that test exists at that level. For now, the test is disabled. I'll follow up with removal after more investigation. Related to #6458, #6460
MediaSourceEngine does not, in fact, preserve SourceBuffer properties when we reset it to switch codecs. This is handled by StreamingEngine instead, through a follow-up call to setStreamProperties. The test only ever passed as originally written because there was a missing `await` on an async reset process. This removes the bogus test. Related to #6458, #6462
When reloading MediaSourceEngine via StreamingEngine, some logic executed before the reload cancels operations and aligns StreamingEngine state with MediaSourceEngine. However, additional logic was executed after setStreamProperties, which caused a race condition that broke StreamingEngine state by manipulating it out of the usual sequence and outside the usual methods, leading to exceptions and failed assertions. This removes that unnecessary logic and leaves subtle state management to the functions that are meant to do it. Closes #6458
The tests were not testing what they were supposed to because their configs were invalid and being ignored. Related to #6458
The tests for MediaSourceEngine codec switching were written to ignore types and suppress access controls. The were unreadable, too, with very little whitespace, confusing one-letter variable names, and difficult-to-follow event mocking. This made it more difficult to debug test failures in PR #6460. This rewrites the tests in a more readable manner with compiler enforcement of types in the tests. Two helper functions are used to isolate the necessary access-control suppressions. This exposed a bug in the tests, in which one test case (preserve SourceBuffer attributes) only passed because the original version failed to await on an async process. I am not sure that the functionality in that test exists at that level. For now, the test is disabled. I'll follow up with removal after more investigation. Related to #6458, #6460
MediaSourceEngine does not, in fact, preserve SourceBuffer properties when we reset it to switch codecs. This is handled by StreamingEngine instead, through a follow-up call to setStreamProperties. The test only ever passed as originally written because there was a missing `await` on an async reset process. This removes the bogus test. Related to #6458, #6462
When reloading MediaSourceEngine via StreamingEngine, some logic executed before the reload cancels operations and aligns StreamingEngine state with MediaSourceEngine. However, additional logic was executed after setStreamProperties, which caused a race condition that broke StreamingEngine state by manipulating it out of the usual sequence and outside the usual methods, leading to exceptions and failed assertions. This removes that unnecessary logic and leaves subtle state management to the functions that are meant to do it. Closes #6458 Backported to v4.6.x
Have you read the FAQ and checked for duplicate open issues?
Yes
If the problem is related to FairPlay, have you read the tutorial?
N/A
What version of Shaka Player are you using?
main
Can you reproduce the issue with our latest release version?
N/A
Can you reproduce the issue with the latest code from
main
?yes
Are you using the demo app or your own custom app?
N/A
If custom app, can you reproduce the issue using our demo app?
N/A
What browser and OS are you using?
Microsoft Edge 123 on Windows
For embedded devices (smart TVs, etc.), what model and firmware version are you using?
N/A
What are the manifest and license server URIs?
N/A
What configuration are you using? What is the output of
player.getConfiguration()
?N/A
What did you do?
Run standard CI tests on a PR.
What did you expect to happen?
All tests pass on all platforms
What actually happened?
Test runs on PRs have been failing lately, with a fairly consistent failure in codec-switching tests. For example:
(From https://github.com/shaka-project/shaka-player/actions/runs/8740237552/job/23990010975)
The text was updated successfully, but these errors were encountered: