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 of some DASH with multiple periods #7519

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ncocaign
Copy link

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

Fixes #7516

…ect#7516)

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

Fixes shaka-project#7516
some-changes.patch Outdated Show resolved Hide resolved
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
@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?

@@ -114,6 +114,7 @@
+../../lib/util/public_promise.js
+../../lib/util/state_history.js
+../../lib/util/stats.js
+../../lib/util/segment_index.js
Copy link
Member

Choose a reason for hiding this comment

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

There is already lib/media/segment_index, 2 same file names in different spaces might be confusing

Comment on lines +124 to +129
/**
* provide CloseSegmentIndexRegister
* @param {shaka.util.CloseSegmentIndexRegister} register
* @exportDoc
*/
setCloseSegmentIndexRegister(register) {}
Copy link
Member

Choose a reason for hiding this comment

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

If we want to add new method to publicly exposed interface, it will need to be documented better.
Preferably though, we shouldn't add new method but try to look for other solutions (maybe callback in shaka.extern.ManifestParser.PlayerInterface?)

Comment on lines +1507 to +1509
* Handles deferred releases of old SegmentIndexes
* content type from a previous update.
* @export
Copy link
Member

Choose a reason for hiding this comment

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

It's not a part of public interface, don't export it

* Removes all SegmentReferences that end before the given time.
* @export
*/
evictAll() {
Copy link
Member

Choose a reason for hiding this comment

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

If it was exported, it should be defined in shaka.extern.SegmentIndex.

@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
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
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