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(DASH): Fix playback after DASH period eviction #7519

Merged
merged 8 commits into from
Nov 13, 2024

Conversation

ncocaign
Copy link
Contributor

@ncocaign ncocaign commented Oct 29, 2024

With this change, closeSegmentIndex() of all streams of a removed period are defered in StreamingEngine.onUpdate_()

Fixes #7516

BEGIN_COMMIT_OVERRIDE
ignore: Reverted
END_COMMIT_OVERRIDE

some-changes.patch Outdated Show resolved Hide resolved
@ncocaign ncocaign requested a review from avelad October 30, 2024 17:00
@shaka-bot
Copy link
Collaborator

Incremental code coverage: 98.08%

@ncocaign ncocaign changed the title fix(DASH): Fix playback of some DASH with multiple period (#7516) fix(DASH): Fix playback of some DASH with multiple periods (#7516) Oct 31, 2024
@avelad avelad added type: bug Something isn't working correctly priority: P2 Smaller impact or easy workaround component: DASH The issue involves the MPEG DASH manifest format labels Oct 31, 2024
@avelad avelad added this to the v4.12 milestone Oct 31, 2024
@avelad avelad changed the title fix(DASH): Fix playback of some DASH with multiple periods (#7516) fix(DASH): Fix playback of some DASH with multiple periods Nov 4, 2024
Copy link
Member

@tykus160 tykus160 left a comment

Choose a reason for hiding this comment

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

Could you add tests?

build/types/core Outdated Show resolved Hide resolved
externs/shaka/manifest_parser.js Outdated Show resolved Hide resolved
lib/dash/dash_parser.js Outdated Show resolved Hide resolved
lib/media/segment_index.js Outdated Show resolved Hide resolved
@avelad avelad added the status: waiting on response Waiting on a response from the reporter(s) of the issue label Nov 6, 2024
@ncocaign
Copy link
Contributor Author

thanks for the feedback, I'm making the various modifications.
I would like to tell your opinions on whether having defered the closing of the segmentIndexes at the level of the StreamingEngine suits you.
Especially in order to not break the management of the indexes in shaka.media.SegmentIndex the evicted segment counter has been updated with evictAll().

@shaka-bot shaka-bot removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Nov 12, 2024
@avelad avelad modified the milestones: v4.12, v4.13 Nov 13, 2024
Copy link
Member

@avelad avelad left a comment

Choose a reason for hiding this comment

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

Please rebase your PR against the main branch and fix the conflicts. Thanks!

externs/shaka/manifest_parser.js Outdated Show resolved Hide resolved
lib/media/segment_index.js Outdated Show resolved Hide resolved
lib/media/streaming_engine.js Outdated Show resolved Hide resolved
lib/media/streaming_engine.js Outdated Show resolved Hide resolved
lib/media/streaming_engine.js Outdated Show resolved Hide resolved
lib/media/streaming_engine.js Outdated Show resolved Hide resolved
lib/util/periods.js Outdated Show resolved Hide resolved
@ncocaign
Copy link
Contributor Author

Could you add tests?

Difficult for me to add test as the issues arrive in a very specific condition (switch + segmentUpdate + period eviction in manifest update).

…ect#7516)

With this change, closeSegmentIndex() of all streams of a removed period are defered in StreamingEngine.onUpdate_()

Fixes shaka-project#7516
Provide an interface between StreamEngine and ManifestParser to defer the call of closeStreamIndex durring the update of segments instead the update of the manifest.

Fix eslint / jsdoc
extend shaka.extern.ManifestParser.PlayerInterface to provide the function closeSegmentIndex.
SegmentIndexex are closed only if not part of an active stream.
restore empty lines initialy removed by eslint --fix
remove logExtPrefix_
@joeyparrish joeyparrish changed the title fix(DASH): Fix playback of some DASH with multiple periods fix(DASH): Fix playback after DASH period eviction Nov 13, 2024
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

Seems okay to me. @tykus160? @avelad?

@tykus160 tykus160 merged commit 5eff038 into shaka-project:main Nov 13, 2024
18 checks passed
avelad added a commit to avelad/shaka-player that referenced this pull request Nov 15, 2024
avelad pushed a commit that referenced this pull request Nov 15, 2024
With this change, closeSegmentIndex() of all streams of a removed period
are defered in StreamingEngine.onUpdate_()

Fixes #7516
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: DASH The issue involves the MPEG DASH manifest format priority: P2 Smaller impact or easy workaround type: bug Something isn't working correctly
Projects
None yet
5 participants