From 658406249f10001ba4c2a0c60b2d49cc0fa08772 Mon Sep 17 00:00:00 2001 From: xMohnad Date: Sat, 25 Jul 2026 12:15:33 +0300 Subject: [PATCH 1/4] feat: support reading existing chapters from MKV files --- pymkv/MKVFile.py | 48 +++++++++ pymkv/chapters.py | 106 ++++++++++++++++++++ pymkv/models.py | 16 +++ tests/test_chapters_parsing.py | 168 ++++++++++++++++++++++++++++++++ tests/test_mkv_chapters_tags.py | 93 ++++++++++++++++++ tests/test_models.py | 10 ++ 6 files changed, 441 insertions(+) create mode 100644 tests/test_chapters_parsing.py diff --git a/pymkv/MKVFile.py b/pymkv/MKVFile.py index 31f20f5..1740cbe 100644 --- a/pymkv/MKVFile.py +++ b/pymkv/MKVFile.py @@ -62,6 +62,7 @@ import subprocess as sp import sys import tempfile +import xml.etree.ElementTree as ET from collections.abc import Callable, Iterable, Sequence from pathlib import Path from typing import Any, ClassVar, TypeVar, cast @@ -79,6 +80,7 @@ Chapters, EditionEntry, export_to_xml, + parse_chapters_xml, ) from pymkv.command_generators import ( AttachmentOptions, @@ -135,6 +137,10 @@ class MKVFile: The path where pymkv looks for the mkvmerge executable. pymkv relies on the mkvmerge executable to parse files. By default, it is assumed mkvmerge is in your shell's $PATH variable. If it is not, you need to set *mkvmerge_path* to the executable location. + mkvextract_path : str, optional + The path where pymkv looks for the mkvextract executable. By default, it is + assumed mkvextract is in your shell's $PATH variable. If it is not, you need to set + *mkvextract_path* to the executable location. Raises ------ @@ -149,8 +155,10 @@ def __init__( file_path: str | os.PathLike | None = None, title: str | None = None, mkvmerge_path: str | os.PathLike | Iterable[str] = "mkvmerge", + mkvextract_path: str | os.PathLike | Iterable[str] = "mkvextract", ) -> None: self.mkvmerge_path: tuple[str, ...] = prepare_mkvtoolnix_path(mkvmerge_path) + self.mkvextract_path: tuple[str, ...] = prepare_mkvtoolnix_path(mkvextract_path) self.title = title self._chapters_file: str | None = None self.chapters_obj: Chapters | None = None @@ -248,6 +256,10 @@ def __init__( new_attachment.source_file = file_path self.attachments.append(new_attachment) + chapter_entries = sum(c.num_entries for c in info_struct.chapters) + if chapter_entries > 0: + self.chapters_obj = self._read_chapters(file_path) + # split options self._split_options: list[str] = [] self._progress_handler = None @@ -528,6 +540,42 @@ def mux( return proc.returncode + def _read_chapters(self, file_path: str) -> Chapters | None: + """ + Extract and parse the chapters already present in `file_path`. + + Uses ``mkvextract chapters `` to obtain the chapter XML and parses it into a + :class:`~pymkv.chapters.Chapters` object. This is used internally when importing a + pre-existing MKV file that has chapters. Failures are logged and treated as "no chapters" + rather than raised, so a file with unreadable chapters can still be opened. + + Parameters + ---------- + file_path : str + The path of the MKV file to read chapters from. + + Returns + ------- + Chapters | None + The parsed chapters, or ``None`` if they could not be extracted or parsed. + """ + command = [*self.mkvextract_path, "chapters", file_path] + try: + result = sp.run(command, check=True, capture_output=True) # noqa: S603 + except (sp.CalledProcessError, FileNotFoundError) as e: + logging.warning("Could not extract chapters from '%s': %s", file_path, e) + return None + + xml_content = result.stdout.decode("utf-8") + if not xml_content.strip(): + return None + + try: + return parse_chapters_xml(xml_content) + except ET.ParseError as e: + logging.warning("Could not parse chapters XML from '%s': %s", file_path, e) + return None + def _write_chapters_xml(self) -> None: """ Write chapter objects to a temporary XML file. diff --git a/pymkv/chapters.py b/pymkv/chapters.py index 732268c..083bdcd 100644 --- a/pymkv/chapters.py +++ b/pymkv/chapters.py @@ -161,3 +161,109 @@ def export_to_xml(chapters: Chapters) -> str: xml_bytes = ET.tostring(root, encoding="utf-8", xml_declaration=True) return xml_bytes.decode("utf-8") + + +def _text_to_bool(value: str | None) -> bool | None: + """ + Convert a Matroska chapter XML flag ("0"/"1") to a bool, preserving ``None``. + """ + if value is None: + return None + return value.strip() == "1" + + +def _text_to_int(value: str | None) -> int | None: + """ + Convert element text to an int, preserving ``None`` and tolerating blank/invalid values. + """ + if value is None: + return None + value = value.strip() + if not value: + return None + try: + return int(value) + except ValueError: + return None + + +def _parse_chapter_display(element: ET.Element) -> dict[str, Any]: + """ + Parse a single ```` element into a dict matching :class:`ChapterDisplay`. + """ + return { + "ChapterString": element.findtext("ChapterString") or "", + "ChapterLanguage": element.findtext("ChapterLanguage") or "und", + "ChapterCountry": element.findtext("ChapterCountry"), + } + + +def _parse_chapter_atom(element: ET.Element) -> dict[str, Any]: + """ + Recursively parse a ```` element into a dict matching :class:`ChapterAtom`. + """ + return { + "ChapterTimeStart": element.findtext("ChapterTimeStart") or "", + "ChapterTimeEnd": element.findtext("ChapterTimeEnd"), + "ChapterUID": _text_to_int(element.findtext("ChapterUID")), + "ChapterFlagHidden": _text_to_bool(element.findtext("ChapterFlagHidden")), + "ChapterFlagEnabled": _text_to_bool(element.findtext("ChapterFlagEnabled")), + "ChapterDisplay": [_parse_chapter_display(d) for d in element.findall("ChapterDisplay")], + "ChapterAtom": [_parse_chapter_atom(a) for a in element.findall("ChapterAtom")], + } + + +def _parse_edition_entry(element: ET.Element) -> dict[str, Any]: + """ + Parse a single ```` element into a dict matching :class:`EditionEntry`. + """ + return { + "EditionUID": _text_to_int(element.findtext("EditionUID")), + "EditionFlagHidden": _text_to_bool(element.findtext("EditionFlagHidden")), + "EditionFlagDefault": _text_to_bool(element.findtext("EditionFlagDefault")), + "EditionFlagOrdered": _text_to_bool(element.findtext("EditionFlagOrdered")), + "ChapterAtom": [_parse_chapter_atom(a) for a in element.findall("ChapterAtom")], + } + + +def parse_chapters_xml(xml_content: str | bytes) -> Chapters: + """ + Parse a Matroska chapters XML document (such as the output of ``mkvextract chapters``) + into a :class:`Chapters` object. + + Parameters + ---------- + xml_content : str | bytes + The XML content to parse, as returned by ``mkvextract chapters ``. + + Returns + ------- + Chapters + The parsed chapters, with one :class:`EditionEntry` per ```` element. + + Raises + ------ + xml.etree.ElementTree.ParseError + If `xml_content` is not well-formed XML. + + Examples + -------- + >>> xml = ''' + ... + ... + ... + ... 00:00:00.000 + ... + ... Intro + ... eng + ... + ... + ... + ... ''' + >>> chapters = parse_chapters_xml(xml) + >>> chapters.editions[0].atoms[0].displays[0].string + 'Intro' + """ + root = ET.fromstring(xml_content) # noqa: S314 + data = {"EditionEntry": [_parse_edition_entry(e) for e in root.findall("EditionEntry")]} + return msgspec.convert(data, type=Chapters, strict=False) diff --git a/pymkv/models.py b/pymkv/models.py index 3d56799..3390d76 100644 --- a/pymkv/models.py +++ b/pymkv/models.py @@ -168,6 +168,19 @@ class AttachmentInfo(msgspec.Struct): properties: AttachmentProperties = msgspec.field(default_factory=AttachmentProperties) +class ChapterInfo(msgspec.Struct): + """ + Summary information about the chapters present in a file. + + Attributes + ---------- + num_entries : int + The number of chapter entries. + """ + + num_entries: int = 0 + + class MkvMergeOutput(msgspec.Struct): """ Root structure of `mkvmerge -J` output. @@ -184,6 +197,8 @@ class MkvMergeOutput(msgspec.Struct): List of track tags. attachments : list[AttachmentInfo] List of attachments. + chapters : list[ChapterInfo] + Summary of chapters present in the file. file_name : str | None The file name. """ @@ -194,3 +209,4 @@ class MkvMergeOutput(msgspec.Struct): global_tags: list[TagEntry] = [] track_tags: list[TagEntry] = [] attachments: list[AttachmentInfo] = [] + chapters: list[ChapterInfo] = [] diff --git a/tests/test_chapters_parsing.py b/tests/test_chapters_parsing.py new file mode 100644 index 0000000..4c3e1dc --- /dev/null +++ b/tests/test_chapters_parsing.py @@ -0,0 +1,168 @@ +import xml.etree.ElementTree as ET + +import pytest + +from pymkv.chapters import Chapters, export_to_xml, parse_chapters_xml + +SIMPLE_CHAPTERS_XML = """ + + + 1000000000 + 1 + 0 + + 2000000001 + 00:00:00.000000000 + 00:05:00.000000000 + 0 + 1 + + Intro + eng + + + + 2000000002 + 00:05:00.000000000 + + Chapter 2 + eng + + + + +""" + +NESTED_CHAPTERS_XML = """ + + + + 00:00:00.000000000 + + Parent + + + 00:01:00.000000000 + + Nested + us + + + + + +""" + +MULTI_EDITION_CHAPTERS_XML = """ + + + 1 + + 00:00:00.000000000 + + Edition 1 Chapter + + + + + 2 + 1 + + 00:00:00.000000000 + + Edition 2 Chapter + + + + +""" + +NO_EDITIONS_CHAPTERS_XML = '\n\n' + + +def test_parse_chapters_xml_returns_chapters_instance() -> None: + chapters = parse_chapters_xml(SIMPLE_CHAPTERS_XML) + assert isinstance(chapters, Chapters) + + +def test_parse_chapters_xml_simple_edition_and_atoms() -> None: + chapters = parse_chapters_xml(SIMPLE_CHAPTERS_XML) + + assert len(chapters.editions) == 1 + edition = chapters.editions[0] + assert edition.uid == 1000000000 # noqa: PLR2004 + assert edition.default is True + assert edition.hidden is False + + assert len(edition.atoms) == 2 # noqa: PLR2004 + first, second = edition.atoms + + assert first.uid == 2000000001 # noqa: PLR2004 + assert first.time_start == "00:00:00.000000000" + assert first.time_end == "00:05:00.000000000" + assert first.hidden is False + assert first.enabled is True + assert len(first.displays) == 1 + assert first.displays[0].string == "Intro" + assert first.displays[0].language == "eng" + + assert second.uid == 2000000002 # noqa: PLR2004 + assert second.time_end is None + assert second.displays[0].string == "Chapter 2" + + +def test_parse_chapters_xml_nested_atoms() -> None: + chapters = parse_chapters_xml(NESTED_CHAPTERS_XML) + + edition = chapters.editions[0] + parent = edition.atoms[0] + assert parent.displays[0].string == "Parent" + assert len(parent.atoms) == 1 + + nested = parent.atoms[0] + assert nested.displays[0].string == "Nested" + assert nested.displays[0].country == "us" + # Language falls back to the default when not specified in the XML. + assert nested.displays[0].language == "und" + + +def test_parse_chapters_xml_multiple_editions() -> None: + chapters = parse_chapters_xml(MULTI_EDITION_CHAPTERS_XML) + + assert len(chapters.editions) == 2 # noqa: PLR2004 + assert chapters.editions[0].uid == 1 + assert chapters.editions[1].uid == 2 # noqa: PLR2004 + assert chapters.editions[1].ordered is True + assert chapters.editions[0].atoms[0].displays[0].string == "Edition 1 Chapter" + assert chapters.editions[1].atoms[0].displays[0].string == "Edition 2 Chapter" + + +def test_parse_chapters_xml_no_editions() -> None: + chapters = parse_chapters_xml(NO_EDITIONS_CHAPTERS_XML) + assert chapters.editions == [] + + +def test_parse_chapters_xml_invalid_xml_raises() -> None: + with pytest.raises(ET.ParseError): + parse_chapters_xml("") + + +def test_parse_chapters_xml_accepts_bytes() -> None: + chapters = parse_chapters_xml(SIMPLE_CHAPTERS_XML.encode("utf-8")) + assert len(chapters.editions) == 1 + + +def test_parse_chapters_xml_roundtrip_with_export() -> None: + """A chapters object built by hand should survive an export -> parse round trip.""" + chapters = Chapters() + chapters.add_simple_chapter("00:00:00.000", "Intro", language="eng") + chapters.add_simple_chapter("00:10:00.000", "Part 2", language="eng") + + xml_content = export_to_xml(chapters) + reparsed = parse_chapters_xml(xml_content) + + assert len(reparsed.editions) == 1 + assert len(reparsed.editions[0].atoms) == 2 # noqa: PLR2004 + assert reparsed.editions[0].atoms[0].displays[0].string == "Intro" + assert reparsed.editions[0].atoms[0].time_start == "00:00:00.000" + assert reparsed.editions[0].atoms[1].displays[0].string == "Part 2" diff --git a/tests/test_mkv_chapters_tags.py b/tests/test_mkv_chapters_tags.py index 332c0c4..437ae1b 100644 --- a/tests/test_mkv_chapters_tags.py +++ b/tests/test_mkv_chapters_tags.py @@ -1,9 +1,12 @@ +import subprocess as sp from pathlib import Path from unittest.mock import Mock +import msgspec import pytest from pymkv import MKVFile +from pymkv.Verifications import get_file_info def test_chapter_language_getter_setter() -> None: @@ -222,6 +225,96 @@ def test_no_track_tags() -> None: assert track.no_track_tags is True +def test_chapters_obj_defaults_to_none() -> None: + mkv = MKVFile() + assert mkv.chapters_obj is None + + +def test_read_chapters_parses_mkvextract_output(monkeypatch: pytest.MonkeyPatch, dummy_mkv: Path) -> None: + mkv = MKVFile() + + xml_output = b""" + + + + 00:00:00.000000000 + + Intro + eng + + + + +""" + + def fake_run(command: list[str], check: bool, capture_output: bool) -> Mock: + assert command[-2] == "chapters" + assert command[-1] == str(dummy_mkv) + result = Mock() + result.stdout = xml_output + return result + + monkeypatch.setattr(sp, "run", fake_run) + + chapters = mkv._read_chapters(str(dummy_mkv)) # noqa: SLF001 + + assert chapters is not None + assert len(chapters.editions) == 1 + assert chapters.editions[0].atoms[0].displays[0].string == "Intro" + + +def test_read_chapters_returns_none_on_process_error(monkeypatch: pytest.MonkeyPatch, dummy_mkv: Path) -> None: + mkv = MKVFile() + + def fake_run(*args: object, **kwargs: object) -> Mock: + raise sp.CalledProcessError(returncode=2, cmd=["mkvextract"]) + + monkeypatch.setattr(sp, "run", fake_run) + + assert mkv._read_chapters(str(dummy_mkv)) is None # noqa: SLF001 + + +def test_read_chapters_returns_none_on_empty_output(monkeypatch: pytest.MonkeyPatch, dummy_mkv: Path) -> None: + mkv = MKVFile() + + def fake_run(*args: object, **kwargs: object) -> Mock: + result = Mock() + result.stdout = b"" + return result + + monkeypatch.setattr(sp, "run", fake_run) + + assert mkv._read_chapters(str(dummy_mkv)) is None # noqa: SLF001 + + +def test_read_chapters_returns_none_on_invalid_xml(monkeypatch: pytest.MonkeyPatch, dummy_mkv: Path) -> None: + mkv = MKVFile() + + def fake_run(*args: object, **kwargs: object) -> Mock: + result = Mock() + result.stdout = b"" + return result + + monkeypatch.setattr(sp, "run", fake_run) + + assert mkv._read_chapters(str(dummy_mkv)) is None # noqa: SLF001 + + +def test_init_populates_chapters_obj_from_existing_file(get_path_test_file: Path) -> None: + """Integration test: if the fixture file has chapters, MKVFile should expose them.""" + info = msgspec.to_builtins(get_file_info(get_path_test_file, "mkvmerge")) + chapter_entries = info.get("chapters", []) + has_chapters = any(entry.get("num_entries", 0) > 0 for entry in chapter_entries) + + mkv = MKVFile(str(get_path_test_file)) + + if has_chapters: + assert mkv.chapters_obj is not None + assert len(mkv.chapters_obj.editions) > 0 + else: + assert mkv.chapters_obj is None + + def test_no_attachments() -> None: mkv = MKVFile() diff --git a/tests/test_models.py b/tests/test_models.py index d448118..5186e07 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -3,6 +3,7 @@ from pymkv.models import ( AttachmentInfo, AttachmentProperties, + ChapterInfo, ContainerInfo, ContainerProperties, MkvMergeOutput, @@ -71,12 +72,18 @@ def test_attachment_properties_defaults() -> None: assert ap.mime_type is None +def test_chapter_info_defaults() -> None: + ci = ChapterInfo() + assert ci.num_entries == 0 + + def test_mkv_merge_output_defaults() -> None: output = MkvMergeOutput(container=ContainerInfo()) assert output.tracks == [] assert output.global_tags == [] assert output.track_tags == [] assert output.attachments == [] + assert output.chapters == [] assert output.file_name is None @@ -97,6 +104,7 @@ def test_mkv_merge_output_decode() -> None: "global_tags": [], "track_tags": [], "attachments": [], + "chapters": [{"num_entries": 5}], "file_name": "test.mkv" }""" result = msgspec.json.decode(json_bytes, type=MkvMergeOutput, strict=False) @@ -107,3 +115,5 @@ def test_mkv_merge_output_decode() -> None: assert result.tracks[0].codec == "h264" assert result.tracks[0].properties.default_track is True assert result.file_name == "test.mkv" + assert len(result.chapters) == 1 + assert result.chapters[0].num_entries == 5 # noqa: PLR2004 From d2ed79b2927e7085db78e30072eae87638c1c762 Mon Sep 17 00:00:00 2001 From: xMohnad Date: Sat, 25 Jul 2026 13:30:16 +0300 Subject: [PATCH 2/4] refactor: derive optional chapter field defaults from struct, not parser --- pymkv/chapters.py | 66 +++++++++++++++++++++++----------- tests/test_chapters_parsing.py | 2 +- 2 files changed, 46 insertions(+), 22 deletions(-) diff --git a/pymkv/chapters.py b/pymkv/chapters.py index 083bdcd..b038fb0 100644 --- a/pymkv/chapters.py +++ b/pymkv/chapters.py @@ -187,43 +187,67 @@ def _text_to_int(value: str | None) -> int | None: return None +def _findtext(element: ET.Element, tag: str) -> str | None: + """ + Return the stripped text of a child element, or ``None`` if it is absent or blank. + """ + text = element.findtext(tag) + if text is None: + return None + text = text.strip() + return text or None + + +def _strip_none(data: dict[str, Any]) -> dict[str, Any]: + """ + Drop keys whose value is ``None`` before handing the dict to :func:`msgspec.convert`. + """ + return {key: value for key, value in data.items() if value is not None} + + def _parse_chapter_display(element: ET.Element) -> dict[str, Any]: """ Parse a single ```` element into a dict matching :class:`ChapterDisplay`. """ - return { - "ChapterString": element.findtext("ChapterString") or "", - "ChapterLanguage": element.findtext("ChapterLanguage") or "und", - "ChapterCountry": element.findtext("ChapterCountry"), - } + return _strip_none( + { + "ChapterString": _findtext(element, "ChapterString") or "", + "ChapterLanguage": _findtext(element, "ChapterLanguage"), + "ChapterCountry": _findtext(element, "ChapterCountry"), + } + ) def _parse_chapter_atom(element: ET.Element) -> dict[str, Any]: """ Recursively parse a ```` element into a dict matching :class:`ChapterAtom`. """ - return { - "ChapterTimeStart": element.findtext("ChapterTimeStart") or "", - "ChapterTimeEnd": element.findtext("ChapterTimeEnd"), - "ChapterUID": _text_to_int(element.findtext("ChapterUID")), - "ChapterFlagHidden": _text_to_bool(element.findtext("ChapterFlagHidden")), - "ChapterFlagEnabled": _text_to_bool(element.findtext("ChapterFlagEnabled")), - "ChapterDisplay": [_parse_chapter_display(d) for d in element.findall("ChapterDisplay")], - "ChapterAtom": [_parse_chapter_atom(a) for a in element.findall("ChapterAtom")], - } + return _strip_none( + { + "ChapterTimeStart": _findtext(element, "ChapterTimeStart") or "", + "ChapterTimeEnd": _findtext(element, "ChapterTimeEnd"), + "ChapterUID": _text_to_int(element.findtext("ChapterUID")), + "ChapterFlagHidden": _text_to_bool(element.findtext("ChapterFlagHidden")), + "ChapterFlagEnabled": _text_to_bool(element.findtext("ChapterFlagEnabled")), + "ChapterDisplay": [_parse_chapter_display(d) for d in element.findall("ChapterDisplay")], + "ChapterAtom": [_parse_chapter_atom(a) for a in element.findall("ChapterAtom")], + } + ) def _parse_edition_entry(element: ET.Element) -> dict[str, Any]: """ Parse a single ```` element into a dict matching :class:`EditionEntry`. """ - return { - "EditionUID": _text_to_int(element.findtext("EditionUID")), - "EditionFlagHidden": _text_to_bool(element.findtext("EditionFlagHidden")), - "EditionFlagDefault": _text_to_bool(element.findtext("EditionFlagDefault")), - "EditionFlagOrdered": _text_to_bool(element.findtext("EditionFlagOrdered")), - "ChapterAtom": [_parse_chapter_atom(a) for a in element.findall("ChapterAtom")], - } + return _strip_none( + { + "EditionUID": _text_to_int(element.findtext("EditionUID")), + "EditionFlagHidden": _text_to_bool(element.findtext("EditionFlagHidden")), + "EditionFlagDefault": _text_to_bool(element.findtext("EditionFlagDefault")), + "EditionFlagOrdered": _text_to_bool(element.findtext("EditionFlagOrdered")), + "ChapterAtom": [_parse_chapter_atom(a) for a in element.findall("ChapterAtom")], + } + ) def parse_chapters_xml(xml_content: str | bytes) -> Chapters: diff --git a/tests/test_chapters_parsing.py b/tests/test_chapters_parsing.py index 4c3e1dc..45f03dc 100644 --- a/tests/test_chapters_parsing.py +++ b/tests/test_chapters_parsing.py @@ -123,7 +123,7 @@ def test_parse_chapters_xml_nested_atoms() -> None: assert nested.displays[0].string == "Nested" assert nested.displays[0].country == "us" # Language falls back to the default when not specified in the XML. - assert nested.displays[0].language == "und" + assert nested.displays[0].language == "eng" def test_parse_chapters_xml_multiple_editions() -> None: From 20fc06fc7b760e735bc72c9d92c1bac32bfca867 Mon Sep 17 00:00:00 2001 From: xMohnad Date: Sun, 2 Aug 2026 19:32:17 +0300 Subject: [PATCH 3/4] fix(chapters): don't rewrite source chapters on remux --- pymkv/MKVFile.py | 34 +++++++++++-- tests/conftest.py | 76 +++++++++++++++++++++++++++++ tests/test_mkv_chapters_tags.py | 84 +++++++++++++++++++++++++++++---- 3 files changed, 181 insertions(+), 13 deletions(-) diff --git a/pymkv/MKVFile.py b/pymkv/MKVFile.py index 5264301..feeecd0 100644 --- a/pymkv/MKVFile.py +++ b/pymkv/MKVFile.py @@ -161,7 +161,8 @@ def __init__( self.mkvextract_path: tuple[str, ...] = prepare_mkvtoolnix_path(mkvextract_path) self.title = title self._chapters_file: str | None = None - self.chapters_obj: Chapters | None = None + self._chapters_obj: Chapters | None = None + self._chapters_from_source: bool = False self._temp_chapters_file: str | None = None self._chapter_language: str | None = None self._global_tags_file: str | None = None @@ -258,7 +259,10 @@ def __init__( chapter_entries = sum(c.num_entries for c in info_struct.chapters) if chapter_entries > 0: - self.chapters_obj = self._read_chapters(file_path) + chapters_from_source = self._read_chapters(file_path) + if chapters_from_source is not None: + self._chapters_obj = chapters_from_source + self._chapters_from_source = True # split options self._split_options: list[str] = [] @@ -292,6 +296,28 @@ def __exit__(self, exc_type: object, exc_val: object, exc_tb: object) -> None: """Clean up temporary files on context manager exit.""" self.cleanup() + @property + def chapters_obj(self) -> Chapters | None: + """ + Get or set the :class:`~pymkv.chapters.Chapters` object attached to this file. + + Assigning to this property (``mkv.chapters_obj = Chapters()``) always marks the + chapters as caller-supplied: :meth:`command` will serialize them to a temporary XML + file and pass it to mkvmerge via ``--chapters``, overriding whatever chapters (if any) + the source file already had. + + Returns + ------- + Chapters | None + The chapters object, or ``None`` if no chapters are set. + """ + return self._chapters_obj + + @chapters_obj.setter + def chapters_obj(self, chapters: Chapters | None) -> None: + self._chapters_obj = chapters + self._chapters_from_source = False + @property def chapter_language(self) -> str | None: """ @@ -402,7 +428,7 @@ def command( self.output_path = str(Path(output_path).expanduser()) # Handle object-based chapters - if self.chapters_obj and not self._chapters_file: + if self.chapters_obj and not self._chapters_file and not self._chapters_from_source: self._write_chapters_xml() # Pre-assign file IDs @@ -628,6 +654,8 @@ def add_chapter(self, chapter: ChapterAtom | EditionEntry) -> None: """ if self.chapters_obj is None: self.chapters_obj = Chapters() + else: + self._chapters_from_source = False if isinstance(chapter, ChapterAtom): if not self.chapters_obj.editions: diff --git a/tests/conftest.py b/tests/conftest.py index 8066ae8..4fbcfcc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,4 +1,6 @@ import random +import shutil +import subprocess as sp from collections.abc import Callable, Generator from pathlib import Path from typing import Any @@ -9,6 +11,11 @@ from pymkv.models import ContainerInfo, MkvMergeOutput, TrackInfo +requires_mkvtoolnix = pytest.mark.skipif( + shutil.which("mkvmerge") is None or shutil.which("mkvextract") is None, + reason="mkvmerge/mkvextract not installed; this test requires the real binaries", +) + TESTS_ROOT = Path.cwd() @@ -96,6 +103,75 @@ def get_path_test_chapters_txt(get_base_path: Path) -> Path: return chapter_path +# Source chapters XML used to build the real chaptered fixture file below. Deliberately includes +# fields that pymkv.chapters.Chapters does not model (ChapterPhysicalEquiv, ChapLanguageIETF) so +# tests can assert those are preserved on remux instead of being silently dropped. +_CHAPTERS_SOURCE_XML = """ + + + 1111 + + 2001 + 00:00:00.000000000 + 10 + + Intro + chi + zh-Hans + + + + +""" + + +@pytest.fixture +def get_path_test_file_with_chapters( + get_path_test_file_two: Path, + tmp_path: Path, +) -> Path: + """ + Build a real MKV file with chapters (via ``mkvmerge --chapters``) + """ + chapters_xml = tmp_path / "source_chapters.xml" + chapters_xml.write_text(_CHAPTERS_SOURCE_XML, encoding="utf-8") + + output_path = tmp_path / "chaptered.mkv" + + mkvmerge = shutil.which("mkvmerge") + assert mkvmerge is not None + sp.run( # noqa: S603 + [ + mkvmerge, + "-o", + str(output_path), + "--chapters", + str(chapters_xml), + str(get_path_test_file_two), + ], + check=True, + capture_output=True, + ) + return output_path + + +@pytest.fixture +def real_mkvextract_chapters_xml(get_path_test_file_with_chapters: Path) -> str: + """ + The actual XML that ``mkvextract chapters`` produces for the fixture above, including + whatever BOM/derived fields (e.g. ``ChapterCountry``) mkvextract adds on its own. + """ + + mkvextract = shutil.which("mkvextract") + assert mkvextract is not None + result = sp.run( # noqa: S603 + [mkvextract, "chapters", str(get_path_test_file_with_chapters)], + check=True, + capture_output=True, + ) + return result.stdout.decode("utf-8") + + @pytest.fixture(autouse=True) def auto_profile(request: pytest.FixtureRequest) -> Generator[None, None, None]: profile_root = TESTS_ROOT / ".profiles" diff --git a/tests/test_mkv_chapters_tags.py b/tests/test_mkv_chapters_tags.py index 437ae1b..717fabb 100644 --- a/tests/test_mkv_chapters_tags.py +++ b/tests/test_mkv_chapters_tags.py @@ -6,7 +6,9 @@ import pytest from pymkv import MKVFile +from pymkv.chapters import ChapterAtom, ChapterDisplay, Chapters from pymkv.Verifications import get_file_info +from tests.conftest import requires_mkvtoolnix def test_chapter_language_getter_setter() -> None: @@ -231,6 +233,7 @@ def test_chapters_obj_defaults_to_none() -> None: def test_read_chapters_parses_mkvextract_output(monkeypatch: pytest.MonkeyPatch, dummy_mkv: Path) -> None: + """Unit test for the plumbing in `_read_chapters` (command shape, stdout decoding).""" mkv = MKVFile() xml_output = b""" @@ -263,6 +266,26 @@ def fake_run(command: list[str], check: bool, capture_output: bool) -> Mock: assert chapters.editions[0].atoms[0].displays[0].string == "Intro" +@requires_mkvtoolnix +def test_read_chapters_matches_real_mkvextract_output( + get_path_test_file_with_chapters: Path, + real_mkvextract_chapters_xml: str, +) -> None: + """`_read_chapters` should handle actual `mkvextract chapters` output, BOM and all.""" + assert real_mkvextract_chapters_xml.strip(), "expected mkvextract to emit chapter XML" + + mkv = MKVFile() + chapters = mkv._read_chapters(str(get_path_test_file_with_chapters)) # noqa: SLF001 + + assert chapters is not None + assert len(chapters.editions) == 1 + assert chapters.editions[0].uid == 1111 # noqa: PLR2004 + atom = chapters.editions[0].atoms[0] + assert atom.uid == 2001 # noqa: PLR2004 + assert atom.displays[0].string == "Intro" + assert atom.displays[0].language == "chi" + + def test_read_chapters_returns_none_on_process_error(monkeypatch: pytest.MonkeyPatch, dummy_mkv: Path) -> None: mkv = MKVFile() @@ -300,19 +323,60 @@ def fake_run(*args: object, **kwargs: object) -> Mock: assert mkv._read_chapters(str(dummy_mkv)) is None # noqa: SLF001 -def test_init_populates_chapters_obj_from_existing_file(get_path_test_file: Path) -> None: - """Integration test: if the fixture file has chapters, MKVFile should expose them.""" - info = msgspec.to_builtins(get_file_info(get_path_test_file, "mkvmerge")) +@requires_mkvtoolnix +def test_init_populates_chapters_obj_from_existing_file(get_path_test_file_with_chapters: Path) -> None: + """Integration test: opening a file that actually has chapters should expose them.""" + info = msgspec.to_builtins(get_file_info(get_path_test_file_with_chapters, "mkvmerge")) chapter_entries = info.get("chapters", []) - has_chapters = any(entry.get("num_entries", 0) > 0 for entry in chapter_entries) + assert any(entry.get("num_entries", 0) > 0 for entry in chapter_entries), ( + "fixture file should have chapters -- test is not exercising the feature" + ) + + mkv = MKVFile(str(get_path_test_file_with_chapters)) + + assert mkv.chapters_obj is not None + assert len(mkv.chapters_obj.editions) > 0 + assert mkv.chapters_obj.editions[0].atoms[0].displays[0].string == "Intro" + + +@requires_mkvtoolnix +def test_command_does_not_rewrite_chapters_read_from_source(get_path_test_file_with_chapters: Path) -> None: + """Chapters populated from an existing file must not be round-tripped through `Chapters`.""" + mkv = MKVFile(str(get_path_test_file_with_chapters)) + assert mkv.chapters_obj is not None # sanity check: the fixture does have chapters + + command = mkv.command("output.mkv", subprocess=True) + + assert "--chapters" not in command + assert mkv._chapters_file is None # noqa: SLF001 + + +@requires_mkvtoolnix +def test_command_rewrites_chapters_after_explicit_edit(get_path_test_file_with_chapters: Path) -> None: + """Once the caller explicitly edits chapters that came from the source file, they should be + written out and passed to mkvmerge via `--chapters`, since they no longer match the source. + """ + mkv = MKVFile(str(get_path_test_file_with_chapters)) + assert mkv.chapters_obj is not None + + mkv.add_chapter(ChapterAtom(time_start="00:10:00.000", displays=[ChapterDisplay(string="New Chapter")])) + + command = mkv.command("output.mkv", subprocess=True) + + assert "--chapters" in command + assert mkv._chapters_file is not None # noqa: SLF001 + + +def test_command_rewrites_chapters_reassigned_by_caller() -> None: + """Reassigning `chapters_obj` directly is also treated as a caller edit.""" + mkv = MKVFile() + mkv.chapters_obj = Chapters() + mkv.chapters_obj.add_simple_chapter("00:00:00.000", "Intro") - mkv = MKVFile(str(get_path_test_file)) + command = mkv.command("output.mkv", subprocess=True) - if has_chapters: - assert mkv.chapters_obj is not None - assert len(mkv.chapters_obj.editions) > 0 - else: - assert mkv.chapters_obj is None + assert "--chapters" in command + assert mkv._chapters_file is not None # noqa: SLF001 def test_no_attachments() -> None: From f12c9dcf95e2851466a3d0e325de9ccfeb7ca0d4 Mon Sep 17 00:00:00 2001 From: xMohnad Date: Sun, 2 Aug 2026 20:35:12 +0300 Subject: [PATCH 4/4] test(chapters): cover blank/invalid ChapterUID parsing --- tests/test_chapters_parsing.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/tests/test_chapters_parsing.py b/tests/test_chapters_parsing.py index 45f03dc..7e77526 100644 --- a/tests/test_chapters_parsing.py +++ b/tests/test_chapters_parsing.py @@ -11,7 +11,7 @@ 1 0 - 2000000001 + 00:00:00.000000000 00:05:00.000000000 0 @@ -29,6 +29,13 @@ eng + + not-a-number + 00:15:00.000000000 + + Invalid UID + + """ @@ -94,10 +101,10 @@ def test_parse_chapters_xml_simple_edition_and_atoms() -> None: assert edition.default is True assert edition.hidden is False - assert len(edition.atoms) == 2 # noqa: PLR2004 - first, second = edition.atoms + assert len(edition.atoms) == 3 # noqa: PLR2004 + first, second, invalid_uid = edition.atoms - assert first.uid == 2000000001 # noqa: PLR2004 + assert first.uid is None # blank text assert first.time_start == "00:00:00.000000000" assert first.time_end == "00:05:00.000000000" assert first.hidden is False @@ -110,6 +117,9 @@ def test_parse_chapters_xml_simple_edition_and_atoms() -> None: assert second.time_end is None assert second.displays[0].string == "Chapter 2" + assert invalid_uid.uid is None # non-numeric + assert invalid_uid.displays[0].string == "Invalid UID" + def test_parse_chapters_xml_nested_atoms() -> None: chapters = parse_chapters_xml(NESTED_CHAPTERS_XML)