-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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(HLS): Skip EXT-X-PRELOAD-HINT without full byterange info #5294
Conversation
We need investigate another solutions, for example https://www.akamai.com/blog/performance/-using-ll-hls-with-byte-range-addressing-to-achieve-interoperabi |
Incremental code coverage: 100.00% |
if (tag.getAttributeValue('BYTERANGE-START') != null) { | ||
if (tag.getAttributeValue('BYTERANGE-LENGTH') != null) { | ||
partialSegmentTags.push(tag); | ||
} | ||
} else { | ||
partialSegmentTags.push(tag); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to go straight to the recommendation of https://www.akamai.com/blog/performance/-using-ll-hls-with-byte-range-addressing-to-achieve-interoperabi, do this instead in createSegmentReference_
:
// A preload hinted partial segment may have byterange length info.
const pByterangeLength = item.getAttributeValue('BYTERANGE-LENGTH');
if (pByterangeLength) {
pEndByte = pStartByte + Number(pByterangeLength) - 1;
}
// *** ADD THIS ***
else if (pStartByte) {
// If we have a non-zero start byte, but no end byte, follow the recommendation of
// https://tinyurl.com/hls-open-byte-range and set the end byte explicitly to a
// large integer.
pEndByte = Number.MAX_SAFE_INTEGER;
}
I haven't tested that against the content in #5089. If it doesn't work, we can just ignore the HINT segment instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have tried and thought about that, but the problem is that it overlaps with the following partial segments and very rare cases occur. That's why I created this solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in the future we need to rethink the way we work with byterange, but this is a easy-fix in the meantime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does MAX_SAFE_INTEGER on preload hint interact poorly with partial segments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a certain segment it has 2 partial segments and 1 preload. In the next update you have 3 partial segments and 1 preload. The preload from before the update has now been converted to a partial segment with all the byterange info, but the new preload doesn't have the info. If we call the first one with the maximum range, we are actually obtaining the corresponding partial and all the following ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. It seems that Will Law's article suggests getting and using that continuation on purpose, but Shaka Player isn't built for it at the moment.
@@ -158,7 +158,13 @@ shaka.hls.ManifestTextParser = class { | |||
partialSegmentTags.push(tag); | |||
} else if (tag.name == 'EXT-X-PRELOAD-HINT') { | |||
if (tag.getAttributeValue('TYPE') == 'PART') { | |||
partialSegmentTags.push(tag); | |||
if (tag.getAttributeValue('BYTERANGE-START') != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment here that BYTERANGE-START without BYTERANGE-LENGTH is being ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
🤖 I have created a release *beep* *boop* --- ## [4.2.11](v4.2.10...v4.2.11) (2023-06-20) ### Bug Fixes * **Demo:** Fix deployment of codem-isoboxer in the Demo ([#5257](#5257)) ([23d48d4](23d48d4)) * **Demo:** Fix error link width to avoid overlap with close button ([#5309](#5309)) ([a6f980d](a6f980d)) * Fix error when network status changes on src= playbacks ([#5305](#5305)) ([ce354ba](ce354ba)) * **HLS:** Avoid "Possible encoding problem detected!" when is a preload reference ([#5332](#5332)) ([763ae6a](763ae6a)) * **HLS:** Avoid HLS resync when there is a gap in the stream ([#5284](#5284)) ([256cf20](256cf20)) * **HLS:** Avoid variable substitution if no variables ([#5269](#5269)) ([b549b60](b549b60)) * **HLS:** Parse EXT-X-PART-INF as media playlist tag ([#5311](#5311)) ([d78c080](d78c080)) * **HLS:** Skip EXT-X-PRELOAD-HINT without full byterange info ([#5294](#5294)) ([e462711](e462711)) * media source object URL revocation ([#5214](#5214)) ([80ce378](80ce378)) * Ship to NPM without node version restrictions ([#5253](#5253)) ([41c1ace](41c1ace)), closes [#5243](#5243) * unnecessary parsing of in-band pssh when pssh is in the manifest ([#5198](#5198)) ([889cc68](889cc68)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- ## [4.3.7](v4.3.6...v4.3.7) (2023-06-21) ### Bug Fixes * CEA 608 captions not work with H.265 video streams ([#5252](#5252)) ([b08bb41](b08bb41)), closes [#5251](#5251) * **Demo:** Fix deployment of codem-isoboxer in the Demo ([#5257](#5257)) ([7e2903a](7e2903a)) * **demo:** Fix deployment of v4.3.x on appspot ([ccf5e2e](ccf5e2e)) * **Demo:** Fix error link width to avoid overlap with close button ([#5309](#5309)) ([f575dab](f575dab)) * Fix error when network status changes on src= playbacks ([#5305](#5305)) ([cf683f5](cf683f5)) * **HLS:** Avoid "Possible encoding problem detected!" when is a preload reference ([#5332](#5332)) ([9ce8cc0](9ce8cc0)) * **HLS:** Avoid HLS resync when there is a gap in the stream ([#5284](#5284)) ([679dbae](679dbae)) * **HLS:** Avoid variable substitution if no variables ([#5269](#5269)) ([49afa92](49afa92)) * **HLS:** Fix HLS seekRange for live streams ([#5263](#5263)) ([03df9cb](03df9cb)) * **HLS:** Fix seekRange for EVENT playlist not using EXT-X-PLAYLIST-TYPE ([#5220](#5220)) ([562831b](562831b)) * **HLS:** Parse EXT-X-PART-INF as media playlist tag ([#5311](#5311)) ([f6210ee](f6210ee)) * **HLS:** Skip EXT-X-PRELOAD-HINT without full byterange info ([#5294](#5294)) ([9e193e2](9e193e2)) * media source object URL revocation ([#5214](#5214)) ([1a89daa](1a89daa)) * Ship to NPM without node version restrictions ([#5253](#5253)) ([ca096a8](ca096a8)), closes [#5243](#5243) * unnecessary parsing of in-band pssh when pssh is in the manifest ([#5198](#5198)) ([8d6494d](8d6494d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Fixes #5089