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 a duration field to Frame #7

Closed
koute opened this issue Sep 3, 2022 · 5 comments
Closed

Add a duration field to Frame #7

koute opened this issue Sep 3, 2022 · 5 comments

Comments

@koute
Copy link

koute commented Sep 3, 2022

The Frame struct only contains a timestamp without a duration. This is problematic if you'd want to e.g. demux ASS subtitles since the raw subtitles are mangled and muxed into the .mkv without the timestamps.

For example, this is a raw line from an external .ass sub:

Dialogue: 0,0:00:12.36,0:00:12.40,Style,,0,0,0,,Insert text here

And if you'd mux that into an .mkv this is how it's going to look like:

14,0,Style,,0,0,0,,Insert text here

As you can see the timestamps are gone. The idea here is that you're supposed to recover them from the block timestamp + duration, as documented here.

For your reference, the symphonia_format_mkv crate does include the duration when demuxing.

@hasenbanck
Copy link
Owner

The duration is not a mandatory field in the EBML block (BlockDuration, 0x9B). Simple Blocks don't contain it at all.

So documentation says this about how to extract the duration from the EBML document:

The duration of the Block (based on TimestampScale). The BlockDuration Element can be useful at the end of a Track to define the duration of the last frame (as there is no subsequent Block available), or when there is a break in a track like for subtitle tracks. When not written and with no DefaultDuration, the value is assumed to be the difference between the timestamp of this Block and the timestamp of the next Block in "display" order (not coding order). BlockDuration **MUST** be set if the associated TrackEntry stores a DefaultDuration value.

This would need to be implemented by taking the default duration of the track (already exposed), the "BlockDuration" of a block (not exposed) or the time difference between the current and next frame into account (already exposed).

Not sure if this should be handled by the application or by the library though. PRs are welcome.

@gwen-lg
Copy link
Contributor

gwen-lg commented Feb 8, 2025

I also need to get the BlockDuration for extract subtitles.

I have take a look to how it can be implemented.
I have tested to only add a new method read_duration to be called just after call next_frame on subtitle track.
It's work in my test, but to me it looks more like a hack. (test: add read_duration method in MatroskaFile)

It seems to me that this would be more appropriate to handle this in next_frame. The method "next_frame" would read the entire BlockGroup when it encounters one.
But from what I've seen, it would require a significant changes in how "next_frame" works.
@hasenbanck : Are you open to major changes to fix this?

@hasenbanck
Copy link
Owner

@gwen-lg I'm open to improvement to the duration extraction. What kind of API do you have in mind?

gwen-lg added a commit to gwen-lg/matroska-demuxer that referenced this issue Feb 9, 2025
Subtitle frame generaly need a duration.
In this case, the Frame is placed in a Block grouped
in a GroupBlock with a BlockDuration.

This should fix issue hasenbanck#7 : [Add a duration field to Frame](hasenbanck#7)
gwen-lg added a commit to gwen-lg/matroska-demuxer that referenced this issue Feb 9, 2025
Subtitle frame generaly 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)
gwen-lg added a commit to gwen-lg/matroska-demuxer that referenced this issue Feb 9, 2025
Subtitle frame generaly 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)
gwen-lg added a commit to gwen-lg/matroska-demuxer that referenced this issue Feb 9, 2025
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)
gwen-lg added a commit to gwen-lg/matroska-demuxer that referenced this issue Feb 9, 2025
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)
@gwen-lg
Copy link
Contributor

gwen-lg commented Feb 9, 2025

@hasenbanck : finally I managed to add the feature while keeping the same API.
It's add some complexity next_frame method who are already not trivial, but it's seems to work.

You can review the proposal in the Darft PR : #16

gwen-lg added a commit to gwen-lg/matroska-demuxer that referenced this issue Feb 11, 2025
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)
gwen-lg added a commit to gwen-lg/matroska-demuxer that referenced this issue Feb 12, 2025
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 pushed a commit that referenced this issue Feb 12, 2025
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](#7)
@hasenbanck
Copy link
Owner

Fixed by 57c94b1

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

No branches or pull requests

3 participants