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

Add management of multiple SeekHead Element #15

Merged
merged 2 commits into from
Feb 7, 2025

Conversation

gwen-lg
Copy link
Contributor

@gwen-lg gwen-lg commented Feb 6, 2025

Some Matroska files have a multiple SeakHead Elements, like a first one who only indicate the position of an SeekHead Element at the end of the file.

Copy link
Owner

@hasenbanck hasenbanck left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. I think a test for this change would be helpful to understand it's implementation better.

@hasenbanck
Copy link
Owner

PR looks fine so far. Can you please add some sort of test for this?

@gwen-lg
Copy link
Contributor Author

gwen-lg commented Feb 7, 2025

You think of a mkv file with multiple seekhead ?
I'm not sure if I could create a small test file with this particularity.

@hasenbanck
Copy link
Owner

I thought this change was needed to support multiple SeekHead elements. So we should be able to either create the binary stream that implements such multiple heads, or create a test file to do so. Implementing this feature without a test case is not a good idea.

Some Matroska files have a multiple SeakHead Elements, like a first one
who only indicate the position of an SeekHead Element at the end
of the file.
the multi_seekhead.mkv file have a first seekhead at start of segment who
have the information of a second seekhead segement at end of the file.
@gwen-lg
Copy link
Contributor Author

gwen-lg commented Feb 7, 2025

I'm able to create a file with two seekhead.
The first one (at start of the file) contain only the information (position) of the second one, who are at the end of the file.
It's not as small as the file simple.mkv, but its size seems reasonable to me (570K).

@hasenbanck
Copy link
Owner

Yes, the test file and test itself looks fine. Thanks!

@hasenbanck hasenbanck merged commit b4cc2a4 into hasenbanck:master Feb 7, 2025
1 check 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