feat(hls): Read EXT-X-PROGRAM-DATE-TIME #4034
Conversation
theodab
added
type: enhancement
lib/hls/hls_parser.js
Outdated
| @@ -120,6 +121,9 @@ shaka.hls.HlsParser = class { | |||
| */ | |||
| this.updatePlaylistDelay_ = 0; | |||
|
|
|||
| /** @private {?number} */ | |||
There was a problem hiding this comment.
I think this could use a description in the jsdoc comment.
theodab
force-pushed
the
pdtBranch
branch
2 times, most recently
from
3588b39
to
0f04028
Compare
lib/hls/hls_parser.js
Outdated
| if (forward) { | ||
| forwardAdd -= other.endTime - other.startTime; | ||
| } | ||
| if (!forward) { | ||
| backwardAdd += other.endTime - other.startTime; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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."
|
This fixes the build failure I'm seeing on this PR: #4063 It was something I introduced by accident yesterday. Sorry about that! |
|
You should be able to rebase onto the fix and see a full test run on this PR now. |
lib/hls/hls_parser.js
Outdated
| /** | ||
| * 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. |
There was a problem hiding this comment.
but DOESN'T count the first reference with a syncTime (since we approach it from behind).
lib/hls/hls_parser.js
Outdated
| /** | ||
| * 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. |
There was a problem hiding this comment.
but DOES count the first reference with a syncTime (since we approach it from ahead).
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.
|
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. |
github-actions
bot
added
the
status: archived
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.