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 sidx duration #4849

Merged
merged 5 commits into from
Aug 16, 2022
Merged

fix sidx duration #4849

merged 5 commits into from
Aug 16, 2022

Conversation

nklhtv
Copy link
Contributor

@nklhtv nklhtv commented Aug 15, 2022

This PR will...

Fix calculation of the duration of a segment which contains multiple sidx atoms (multiple fragments in a single m4s file).

Why is this Pull Request needed?

Calculation of the duration based on sidx has a bug.

Are there any points in the code the reviewer needs to double check?

parseSegmentIndex is refactored to be more cohesive and only parse sidx data.
moovEndOffset field has been removed from SidxInfo.
Calculation of the moov offset has been moved to handleSidxRequest.

Resolves issues:

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

Copy link
Collaborator

@robwalch robwalch left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines 547 to 548
const moovBox = findBox(data, ['moov'])[0];
const moovEndOffset = moovBox ? moovBox.length : null; // we need this in case we need to chop of garbage of the end of current data
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can these moovBox, moovEndOffset lines be moved into the if (frag.initSegment) { block below since they are not used otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Ive also deleted the comment because i think its obvious what are we trying to do now.

@nklhtv nklhtv requested a review from robwalch August 16, 2022 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants