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

feat(hls): Read EXT-X-PROGRAM-DATE-TIME #4034

Merged
merged 1 commit into from Mar 24, 2022

Conversation

theodab
Copy link
Collaborator

@theodab theodab commented Mar 17, 2022

This makes the HLS parser read the EXT-X-PROGRAM-DATE-TIME value
on manifests, and use it to make sure that segments are inserted at
the correct place in the timeline, when in sequence mode.

@theodab theodab added type: enhancement New feature or request component: HLS The issue involves Apple's HLS manifest format labels Mar 17, 2022
demo/config.js Outdated Show resolved Hide resolved
@@ -120,6 +121,9 @@ shaka.hls.HlsParser = class {
*/
this.updatePlaylistDelay_ = 0;

/** @private {?number} */
Copy link
Member

Choose a reason for hiding this comment

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

I think this could use a description in the jsdoc comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/media/media_source_engine.js Outdated Show resolved Hide resolved
@theodab theodab force-pushed the pdtBranch branch 2 times, most recently from 3588b39 to 0f04028 Compare March 22, 2022 06:10
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/media/streaming_engine.js Show resolved Hide resolved
lib/util/xml_utils.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Show resolved Hide resolved
lib/hls/hls_parser.js Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
externs/shaka/player.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
Comment on lines 2062 to 2067
if (forward) {
forwardAdd -= other.endTime - other.startTime;
}
if (!forward) {
backwardAdd += other.endTime - other.startTime;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think you're off-by-one here somewhere.

If reference i has no sync time, and i+1 does, the difference between the sync times should be the duration of i. If reference i-1 is the first match, the difference between the sync times should be the duration of i-1.

Since the loop starts at j=0, reference == other in the first pass. Say the durations of the segments going forward (including j=0) are d0, d1, d2, ... and the durations of the segments going backward (including j=0) are e0, e1, e2, ..., then:

matching j correct offset
0 impossible
+1 d0
+2 d0+d1
+3 d0+d1+d2
+4 d0+d1+d2+d3
-1 e1
-2 e1+e2
-3 e1+e2+e3
-4 e1+e2+e3+e4

The backward offsets should never include the current segment duration (j=0), but in this algorithm, they do. Your answer would still be correct any time the segments have a consistent duration, though.

It took me a very long time to analyze this code, though. I think this needs to be rewritten in a simpler way if possible, rather than adding another clause or exception.

I think we should also make sure we add test cases with varying segment lengths to catch issues like this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I reorganized this some. Do you think it is more readable this way? Or should I come up with an entirely different algorithm?

if (other.syncTime != null) {
return other.syncTime + forwardAdd;
}
forwardAdd -= other.endTime - other.startTime;
Copy link
Member

Choose a reason for hiding this comment

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

The asymmetry between forward and backward is necessary, but a reader might think you had done that by mistake. I suggest you add a comment here such as "looking forward, sum all durations, including the original reference at i, excluding the other with a syncTime."

Similarly, below I would add a comment like "looking backward, sum all durations, including the other with a syncTime, but excluding the original reference at i."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added comments.

@joeyparrish
Copy link
Member

This fixes the build failure I'm seeing on this PR: #4063

It was something I introduced by accident yesterday. Sorry about that!

@joeyparrish
Copy link
Member

You should be able to rebase onto the fix and see a full test run on this PR now.

/**
* Look forwards one reference at a time, summing all durations as we
* go, until we find a reference with a syncTime to use as a basis.
* This DOES count the original reference.
Copy link
Member

Choose a reason for hiding this comment

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

but DOESN'T count the first reference with a syncTime (since we approach it from behind).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

/**
* Look backwards one reference at a time, summing all durations as we
* go, until we find a reference with a syncTime to use as a basis.
* This does NOT count the original reference.
Copy link
Member

Choose a reason for hiding this comment

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

but DOES count the first reference with a syncTime (since we approach it from ahead).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

This makes the HLS parser read the EXT-X-PROGRAM-DATE-TIME value
on manifests, and use it to make sure that segments are inserted at
the correct place in the timeline, when in sequence mode.
@theodab
Copy link
Collaborator Author

theodab commented Mar 24, 2022

Hm. A few failures, but that macOS Safari failure doesn't seem to be from this PR, and the others are all failures to boot. Probably safe to merge.

@theodab theodab merged commit 89409ce into shaka-project:main Mar 24, 2022
@theodab theodab deleted the pdtBranch branch March 24, 2022 22:58
@avelad avelad added this to the v4.0 milestone May 4, 2022
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: HLS The issue involves Apple's HLS manifest format status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants