feat: support reading existing chapters from MKV files - #126
Conversation
GitBib
left a comment
There was a problem hiding this comment.
blocker: this changes mux output for every file that already has chapters, it's not just a read API.
chapters_obj gets populated in __init__ (MKVFile.py:259), and command() at MKVFile.py:393 emits --chapters <tmp.xml> whenever chapters_obj is set. So chapters that mkvmerge used to copy natively now round-trip through Chapters, and whatever the struct doesn't model is dropped.
repro:
cat > chap.xml <<'X'
<?xml version="1.0"?>
<Chapters><EditionEntry><EditionUID>1111</EditionUID><ChapterAtom>
<ChapterUID>2001</ChapterUID>
<ChapterTimeStart>00:00:00.000000000</ChapterTimeStart>
<ChapterPhysicalEquiv>10</ChapterPhysicalEquiv>
<ChapterDisplay><ChapterString>Intro</ChapterString>
<ChapterLanguage>chi</ChapterLanguage>
<ChapLanguageIETF>zh-Hans</ChapLanguageIETF></ChapterDisplay>
</ChapterAtom></EditionEntry></Chapters>
X
mkvmerge -o src.mkv --chapters chap.xml tests/file_2.mkv
python -c "from pymkv import MKVFile; MKVFile('src.mkv').mux('out.mkv', silent=True)"
mkvextract chapters out.mkv| field | source | master | this PR |
|---|---|---|---|
ChapterPhysicalEquiv |
10 |
10 |
dropped |
ChapLanguageIETF |
zh-Hans |
zh-Hans |
zh |
Chapters covers EditionUID/hidden/default/ordered plus atom start/end/uid/hidden/enabled/display. Matroska chapter XML has more — ChapterSegmentUID, ChapterPhysicalEquiv, ChapterTrack, ChapterProcess, EditionDisplay. All of it disappears on remux now, silently.
reading chapters shouldn't touch the write path. suggestion: record whether chapters came from the source file or were set by the caller, and only emit --chapters in the second case. otherwise leave pass-through to mkvmerge.
two more:
- the integration test never exercises the feature. both fixtures have zero chapters:
$ mkvmerge -J tests/file.mkv | jq .chapters
[]
so test_init_populates_chapters_obj_from_existing_file always takes the else branch and asserts chapters_obj is None. it needs a file with chapters — build one in the test with mkvmerge --chapters.
- the
sp.runmocks intest_mkv_chapters_tags.pyare what hid both of the above. hand-written XML has no BOM, noChapLanguageIETF, and noChapterCountrythat mkvextract derives on its own. mkvtoolnix is installed in CI, so run against realmkvextractoutput.
the parser itself is fine — parse_chapters_xml ate real mkvextract output, BOM included, without complaint.
the macos failure on this PR is unrelated — the fixture host was serving a corrupt file. fixed in #127, rebase on master and CI should go green.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #126 +/- ##
==========================================
+ Coverage 99.12% 99.30% +0.17%
==========================================
Files 21 21
Lines 1487 1575 +88
==========================================
+ Hits 1474 1564 +90
+ Misses 13 11 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
19b9549 to
ddd11e8
Compare
ddd11e8 to
20fc06f
Compare
8474c1a to
f12c9dc
Compare
Add mkvextract-based parsing so MKVFile can expose chapters that
already exist in a source file, instead of only supporting chapter
creation.
Closes #125