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

feat: add ludt box support #262

Merged
merged 4 commits into from
Sep 5, 2023
Merged

feat: add ludt box support #262

merged 4 commits into from
Sep 5, 2023

Conversation

tanghaowillow
Copy link
Contributor

Add ludt, alou and tlou support.
Fixes #240

The only test file I could find was from MPEGGroup/isobmff#38.

@tobbee
Copy link
Collaborator

tobbee commented Aug 31, 2023

@tanghaowillow Thanks for the PR. I'm a bit tight on time right now, but I will try to go through it in the next few days.

@tobbee tobbee self-requested a review September 3, 2023 19:45
Copy link
Collaborator

@tobbee tobbee left a comment

Choose a reason for hiding this comment

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

Great work!

The main issue I see is that non-zero loudness_info_type values are not detected and would lead to broken parsing. Returning an error for non-zero values may be a good enough for now, but just parsing the extra byte and storing the two values would be a cleaner solution.

The test file you found is good to have, but is not used in any test as far as I can see. Maybe just extracting the ludt box data and decode it could be a good test.

Please check my other comments as well.

In any case, really nice to receive such a good PR.

mp4/lou.go Outdated Show resolved Hide resolved
mp4/lou.go Show resolved Hide resolved
mp4/lou.go Show resolved Hide resolved
mp4/lou.go Outdated Show resolved Hide resolved
mp4/lou.go Outdated Show resolved Hide resolved
mp4/testdata/ludt.mp4 Outdated Show resolved Hide resolved
@tanghaowillow
Copy link
Contributor Author

@tobbee Thanks for your advice. I have modified the code according to the comments.

Copy link
Collaborator

@tobbee tobbee left a comment

Choose a reason for hiding this comment

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

Looks good to me with the changes including the box test and the error for a non-supported value.

Thanks a lot for your contribution!

@tobbee
Copy link
Collaborator

tobbee commented Sep 5, 2023

@tanghaowillow
Found a minor thing in the automatic tests:
golangci-lint complains about an error value from AddChild not being used in the ludt.go file. I suggest that you remove the error value since it is never triggered.

@tobbee tobbee merged commit a68289c into Eyevinn:master Sep 5, 2023
7 checks passed
@tobbee
Copy link
Collaborator

tobbee commented Sep 5, 2023

@tanghaowillow Your PR has now been merged. Good work!

Welcome back if you want to contribute with other code or file an issue!

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.

Audio stream loudness box missing
2 participants