Skip to content

Bugfix: Handle seekhead for cues on mkv files. #2268

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.

Sign up for GitHub

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

Open
KingLucius wants to merge 10 commits into
base: main
Choose a base branch
from
+4,274 −4
Open

Bugfix: Handle seekhead for cues on mkv files. #2268

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ffca82f
Bugfix: Handle seekhead for cues on mkv files.
KingLucius Mar 25, 2025
3a79ac2
Bugfix: Handle seekhead for cues on mkv files.
KingLucius Mar 25, 2025
04c8b3d
Add extractor test dump files
icbaker Mar 26, 2025
f5a6ed0
Experiment with adding a third SeekHead
icbaker Mar 26, 2025
a77bd28
Fix: Allow any number of seek heads in MKV
KingLucius Mar 28, 2025
73a79cb
Merge branch 'mkvSeekhead' of https://github.com/KingLucius/media int…
KingLucius Mar 28, 2025
3525b89
Update MatroskaExtractor.java
KingLucius Mar 28, 2025
705b31d
Update sample_three_seekheads dump files
icbaker Mar 28, 2025
5ee7bd2
Update dump files that were missed in the previous commit
icbaker Mar 28, 2025
46f72f3
Update MatroskaExtractor.java
KingLucius Mar 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion libraries/extractor/src/main/java/androidx/media3/extractor/mkv/MatroskaExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -749,6 +749,13 @@ protected void startMasterElement(int id, long contentPosition, long contentSize
throw ParserException.createForMalformedContainer(
"Multiple Segment elements not supported", /* cause= */ null);
}

// If we have to reparse due to an IO exception we also have to clear the seek head data
Copy link
Collaborator

@icbaker icbaker Apr 29, 2025

Choose a reason for hiding this comment

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

It's not obvious to me how you know that you've encountered an I/O exception if you land here

Is this the clearing logic I suggested maybe belongs in MatroskaExtractor.seek() in #2268 (comment)?

visitedSeekHeads.clear();
pendingSeekHeads.clear();
seekPositionAfterSeekingForHead = C.INDEX_UNSET;
seekForSeekContent = false;

segmentContentPosition = contentPosition;
segmentContentSize = contentSize;
break;
Expand Down Expand Up @@ -846,7 +853,10 @@ protected void endMasterElement(int id) throws ParserException {
} else if (seekEntryId == ID_SEEK_HEAD) {
// We have a set here to prevent inf recursion, only if this seek head is non
// visited we add it. VLC limits this to 10, but this should work equally as well.
if (visitedSeekHeads.add(seekEntryPosition)) {
//
// Note that we also need to check that we do not jump before or to the segment we are on
// as we do not want to clear our visitedSeekHeads
if (visitedSeekHeads.add(seekEntryPosition) && seekEntryPosition > segmentContentPosition) {
Copy link

@fire-light42 fire-light42 Apr 1, 2025

Choose a reason for hiding this comment

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

seekEntryPosition > segmentContentPosition is needed to not seek above the segment to avoid clearing this visistedlist, however should this also be added to cues, or do we trust cues to be valid?

Copy link
Author

@KingLucius KingLucius Apr 8, 2025

Choose a reason for hiding this comment

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

@icbaker

Copy link
Collaborator

@icbaker icbaker Apr 29, 2025

Choose a reason for hiding this comment

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

IIUC it's not valid for the SeekPosition of SeekHead element inside segment A to point outside segment A into segment B?

In that case, it seems reasonable to trust that both the SeekHead and Cues elements are valid - with undefined behaviour (but probably an obscure parsing error) if they are not.

pendingSeekHeads.add(seekEntryPosition);
}
} else if (seekEntryId == ID_CUES) {
Expand Down