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: stop adding unnecessary null terminators to strings #172

Merged
merged 4 commits into from
Feb 19, 2024

Conversation

voxeljorge
Copy link
Contributor

When using this project with AWS Kinesis Video, Kinesis Video fails to parse the CodecID field of an MKV TrackEntry due to the fact that this package guarantees that strings will always have null terminators.

According to the EBML spec, null termination is allowed but not recommended except in the case where a field is being overwritten in place, such that the subsequent data in a string value can be ignored without fully overwriting it (in case the field is very large for example).

https://www.rfc-editor.org/rfc/rfc8794.html#terminating-elements

While the behavior in this package is technically correct, for better compatibility it is advisable not to null terminate strings unless absolutely necessary.

@at-wat
Copy link
Owner

at-wat commented Aug 9, 2022

Looks like some tests aren't updated

@voxeljorge
Copy link
Contributor Author

I'm happy to go through and update more tests, do you have any feedback on the change itself though?

@at-wat
Copy link
Owner

at-wat commented Aug 12, 2022

I think the change itself is reasonable

@voxeljorge voxeljorge force-pushed the no-string-null-terminators branch from 81de5ef to 85eb603 Compare October 4, 2023 13:00
@voxeljorge
Copy link
Contributor Author

@at-wat Sorry for the long delay here but I think I've fixed the broken test, at least go test ./... passes now.

@at-wat at-wat self-requested a review October 5, 2023 02:09
@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9791271) 99.25% compared to head (98735d0) 99.25%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #172   +/-   ##
=======================================
  Coverage   99.25%   99.25%           
=======================================
  Files          24       24           
  Lines        1886     1886           
=======================================
  Hits         1872     1872           
  Misses          9        9           
  Partials        5        5           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

marshal_test.go Outdated Show resolved Hide resolved
@voxeljorge voxeljorge requested a review from at-wat February 11, 2024 17:17
value.go Outdated Show resolved Hide resolved
Copy link
Owner

@at-wat at-wat left a comment

Choose a reason for hiding this comment

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

LGTM

@at-wat at-wat merged commit 14de3a6 into at-wat:master Feb 19, 2024
15 checks passed
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