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 frame duration management #16

Merged
merged 2 commits into from
Feb 12, 2025
Merged

Conversation

gwen-lg
Copy link
Contributor

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

Draft:

This add complexity to the method next_frame which already had a certain complexity.
And have its a "big change", but not easy to split in smaller atomic commit.

Subtitle frame generally need a duration.
In this case, the Frame is placed in a Block grouped in a GroupBlock with a BlockDuration. Without break the laced frames management.

This should fix issue #7 : Add a duration field to Frame

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.

A test would also be nice to have.

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.

Looks good so far. Only a small naming issue left.

src/error.rs Outdated
@@ -117,6 +119,12 @@ impl std::fmt::Display for DemuxError {
DemuxError::PositiveValueIsNotPositive => {
write!(f, "a value that should be positive is not positive")
}
DemuxError::FrameAlreadyHaveDuration(previous_duration) => {
Copy link
Owner

Choose a reason for hiding this comment

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

DemuxError::FrameAlreadyHasDuration

Subtitle frame generally need a duration.
In this case, the Frame is placed in a Block grouped in a GroupBlock with a BlockDuration.
Without break the laced frames management.

This should fix issue hasenbanck#7 : [Add a duration field to Frame](hasenbanck#7)
@hasenbanck
Copy link
Owner

PR now looks fine for merge.

@gwen-lg
Copy link
Contributor Author

gwen-lg commented Feb 12, 2025

Thank you, do you plan to release a new version of matroska-demuxer after merge ?

@gwen-lg gwen-lg marked this pull request as ready for review February 12, 2025 17:18
@hasenbanck hasenbanck merged commit 67e7b66 into hasenbanck:master Feb 12, 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