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: Fix handling of non-interleaved multi track FMP4 files #1214

Merged
merged 2 commits into from
Aug 21, 2023

Conversation

duggaraju
Copy link
Contributor

@duggaraju duggaraju commented Jun 1, 2023

Do not assume that each fragment contains all tracks.
Use track id instead of index to pick the correct timestamp.

Fixes #1213

@joeyparrish joeyparrish changed the title Fix handling of non-interleaved multi track FMP4 files fix: Fix handling of non-interleaved multi track FMP4 files Jul 5, 2023
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

@duggaraju, could you add a unit test of some kind for this?

@joeyparrish joeyparrish added the status: waiting on response Waiting on a response from the reporter(s) of the issue label Jul 5, 2023
@joeyparrish
Copy link
Member

The fix looks good otherwise. Thanks for contributing!

@duggaraju
Copy link
Contributor Author

Thnaks. Let me take a shot at the unit tests and update the PR.

@duggaraju
Copy link
Contributor Author

Added a simple unit tests.

@github-actions github-actions bot removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Jul 24, 2023
@joeyparrish
Copy link
Member

It seems that the tests crash with an out-of-bounds index. Please fix the tests!

Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

Please fix crashing in the tests.

Donot assume that each fragment contains all tracks.
Use track id instead of index to pick the correct timestamp.
@duggaraju
Copy link
Contributor Author

@joeyparrish let me know if the changes look ok.

@cosmin
Copy link
Contributor

cosmin commented Aug 1, 2023

assuming the tests pass this looks good to me

@cosmin cosmin requested a review from joeyparrish August 5, 2023 05:07
@duggaraju
Copy link
Contributor Author

@joeyparrish any concerns?

@joeyparrish
Copy link
Member

Looks good to me

@cosmin cosmin merged commit dcf3225 into shaka-project:main Aug 21, 2023
12 of 21 checks passed
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Oct 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shaka packager doesn't handle non-interleaved multi track fragmented MP4 files correctly.
3 participants