Skip to content

Add support for parsing chapters from MKV files#3144

Open
tymmesyde wants to merge 6 commits intoandroidx:mainfrom
tymmesyde:feat/mkv-chapters
Open

Add support for parsing chapters from MKV files#3144
tymmesyde wants to merge 6 commits intoandroidx:mainfrom
tymmesyde:feat/mkv-chapters

Conversation

@tymmesyde
Copy link
Copy Markdown

Hi,
this include changes to support parsing MKV chapters using the recent changes to introduce metadata Chapter interface
This is already implemented for MP4: Nero & QuickTime
Used this doc as reference: https://www.matroska.org/technical/elements.html

Added sample_with_chapters.mkv for mkvSample_withChapters test
Generated with:

;FFMETADATA1
[CHAPTER]
TIMEBASE=1/1000
START=0
END=1999
title=Chapter 1

[CHAPTER]
TIMEBASE=1/1000
START=2000
END=4999
title=Chapter 2
ffmpeg \                                                                                       
  -f lavfi -i testsrc=duration=5:size=320x240:rate=30 \
  -f lavfi -i sine=frequency=440:duration=5 \
  -i chapters.txt \
  -map 0:v -map 1:a -map_chapters 2 \
  -c:v libx264 -c:a aac \
  sample_with_chapters.mkv

Copy link
Copy Markdown
Collaborator

@icbaker icbaker left a comment

Choose a reason for hiding this comment

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

I can't work out how to delete the stale comment made against an old copy of the code, sorry for the duplication.

private static final int ID_CHAPTER_TIME_START = 0x91;
private static final int ID_CHAPTER_TIME_END = 0x92;
private static final int ID_CHAPTER_TRACK = 0x8F;
private static final int ID_CHAPTER_TRACK_UID = 0x89;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I didn't find this documented in https://datatracker.ietf.org/doc/rfc9559/ - where is this defined?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I don't see it there, i followed this spec: https://www.matroska.org/technical/elements.html
So i guess it's not a RFC standard? Should i remove related logic?

Copy link
Copy Markdown
Author

@tymmesyde tymmesyde Mar 28, 2026

Choose a reason for hiding this comment

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

It is specified in the sources tho: https://github.com/ietf-wg-cellar/matroska-specification/blob/master/ebml_matroska.xml#L1521, probably was not deployed to the webiste

getCurrentTrack(id).number = (int) value;
break;
case ID_TRACK_UID:
getCurrentTrack(id).uid = (int) value;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is documented in https://datatracker.ietf.org/doc/rfc9559/ with range: not 0 (1-18446744073709551615) which I think means:

  1. We should use long here (since 18446744073709551615 = 2^64 - 1 - so actually even Java signed long kinda isn't large enough if you consider signedness - but better than int)
  2. We should use 0 as the 'unset' value, not Format.NO_VALUE which is -1 in Java signed int math, which is18446744073709551615 as a 64-bit unsigned int (i.e. a technically valid value).

if (!chapter.flagHidden
&& (chapter.trackUid == Format.NO_VALUE || chapter.trackUid == uid)) {
long startTimeMs =
chapter.timeStartNs != Format.NO_VALUE
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

it is defaulted to C.TIME_UNSET but you're comparing it with Format.NO_VALUE

? TimeUnit.NANOSECONDS.toMillis(chapter.timeStartNs)
: 0L;
long endTimeMs =
chapter.timeEndNs != Format.NO_VALUE
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

again, C.TIME_UNSET should be used.

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.

3 participants