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

Fixes to mp4 parsing #411

Closed
wants to merge 5 commits into from
Closed

Conversation

mlrsmith
Copy link

@mlrsmith mlrsmith commented Feb 5, 2025

Two fixes (separate commits):

  • Handle trailing four-byte terminator in VisualSampleEntryBox
  • Handle quicktime-style AudioSampleEntryBox variants (and add 'lcpm' handling)

@mlrsmith mlrsmith force-pushed the msmith/assorted-fixes branch from d6ee449 to 42df38a Compare February 6, 2025 17:54
@mlrsmith
Copy link
Author

mlrsmith commented Feb 6, 2025

Oops - this was intended as a PR to an internal fork for now, accidentally set the wrong branch base.

Nonetheless, I think these are valuable fixes - let me know if you'd prefer if I split them out to separate PRs.

These all fix failures to parse real-world MP4s (and in the case of the last commit, a crash).

@tobbee
Copy link
Collaborator

tobbee commented Feb 7, 2025

@mlrsmith Thanks for using the mp4ff library. I'm open to adding support for less common cases as long as the performance is not degraded much for normal usage.

That said, there is an increased maintenance effort with every new tweak, so it is important that there is good testing for them. The way this project is setup now, a GitHub action prohibits merging of a PR into the master branch if the test coverage decreases. This means that all new code must have a test coverage of at least 81% as of now.

If want to provide tests, please make sure that any test files are small, so that the library doesn't grow too much.

@mlrsmith
Copy link
Author

mlrsmith commented Feb 8, 2025

Thanks! I'll try to find some time to build test cases that aren't private customer data, and pull the fixes out into separate PRs.

For now, closing this.

@mlrsmith mlrsmith closed this Feb 8, 2025
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.

2 participants