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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jun 14, 2023
Merged

fix(HLS): Skip EXT-X-PRELOAD-HINT without full byterange info #5294

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9b255e0
fix(HLS): Skip EXT-X-PRELOAD-HINT without full byterange info
avelad Jun 13, 2023
df250bb
Add comment
avelad Jun 14, 2023
82c7122
Revert "Add comment"
avelad Jun 14, 2023
91afca6
Add comment
avelad Jun 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion lib/hls/manifest_text_parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

if (tag.getAttributeValue('BYTERANGE-LENGTH') != null) {
partialSegmentTags.push(tag);
}
} else {
partialSegmentTags.push(tag);
}
Comment on lines +163 to +169
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

} else if (tag.getAttributeValue('TYPE') == 'MAP') {
// Rename the Preload Hint tag to be a Map tag.
tag.setName('EXT-X-MAP');
Expand Down