Skip to content

fix: add_track(str) no longer reuses the source file's mkvmerge info - #129

Merged
GitBib merged 2 commits into
masterfrom
fix/add-track-stale-info
Aug 2, 2026
Merged

fix: add_track(str) no longer reuses the source file's mkvmerge info#129
GitBib merged 2 commits into
masterfrom
fix/add-track-stale-info

Conversation

@GitBib

@GitBib GitBib commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Fixes #123.

The issue reports wrong metadata. It is worse than that — the added track is silently dropped from the muxed output.

mkv = MKVFile("tests/file_2.mkv")
mkv.add_track("s.srt")
mkv.mux("out.mkv")     # returns 0, no warning

generated command, trimmed to the added file:

--no-audio --video-tracks 0 --no-subtitles s.srt

The track is built with existing_info=self._info_json, which is the mkvmerge -J output of the file MKVFile was constructed from, not of s.srt. MKVTrack reads metadata from that JSON instead of probing the file, and track_id defaults to 0, so the SRT reports itself as track 0 of the source — video. TrackOptions then asks the SRT for a video track and explicitly suppresses subtitles. An SRT has no video track, so nothing is taken from it.

tracks in output
before video, audio
after video, audio, subtitles

The same line also suppressed verification: MKVTrack.file_path skips verify_supported when _info_json is set, so any path was accepted through this branch.

Nothing is lost by dropping it. MKVFile.__init__ builds its tracks with existing_info=info_struct directly and passes MKVTrack objects to add_track, so the str branch is never used while loading a file — the reuse only ever applied to caller-supplied paths, where it was wrong by construction.

Behaviour change worth noting: add_track(str) on an MKVFile built from a file now raises ValueError for an unsupported path instead of accepting it silently. Code that relied on the old silence was already producing a broken mux, but it will now fail loudly.

Cost: add_track(str) now runs mkvmerge -J on the added file, about 33 ms here. MKVTrack(path) already pays exactly this, so add_track(str) just costs the same as the equivalent explicit construction.

Both new tests fail on master and pass with the fix — the second one covers the verification bypass, which is DID NOT RAISE before the change.


Second commit: probe each track file once instead of twice.

Profiling the fix showed MKVTrack running mkvmerge -J twice per construction — verify_supported() in the file_path setter fetches the info and returns only a bool, then the track_id setter fetches the same JSON again. Same root cause as above: _info_json doubles as both the probe cache and the "caller vouched, skip verification" flag.

Split by _info_path, the path the cached info describes. The setter now probes once and keeps the result, existing_info is trusted for the first assignment only, and pointing the track elsewhere drops the cache.

This also fixes stale metadata on reassignment, which was a second latent bug of the same family:

t = MKVTrack("sub.srt")      # subtitles
t.file_path = "video.mkv"
t.track_type                 # 'subtitles' before, 'video' now

Verification of the new path was skipped too, for the same reason.

20 iterations of MKVFile + add_track + command: 1.56s → 1.10s, add_track itself halved from 1.01s to 0.53s.

verify_supported is replaced by info.container.supported, which is literally what that function returns; checking_file_path had already validated the path, so only the mkvmerge lookup remains and that one is cached.

Test churn worth flagging: 20 patches of verify_supported in the MKVTrack namespace are gone. They targeted a function that is no longer called, so leaving them would mean tests passing even with verification broken. test_track_file_path_setter_verification_failure now uses a real empty file, which mkvmerge reports as unsupported, instead of mocking the check away.

Both new reassignment tests fail without this commit.

MKVFile.add_track passed self._info_json - the mkvmerge -J output of the
file the MKVFile was built from - as existing_info for a track created
from an unrelated path. MKVTrack then read metadata out of that JSON
instead of probing the file, and since track_id defaults to 0 the new
track reported track 0 of the original file:

    mkv = MKVFile("file.mkv")
    mkv.add_track("subtitle.srt")
    mkv.tracks[-1].track_type   # 'video' instead of 'subtitles'

Same line also suppressed verification: MKVTrack.file_path skips
verify_supported when _info_json is set, so any path was accepted.

Fixes #123
@codecov

codecov Bot commented Aug 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.92%. Comparing base (3165152) to head (d8deb2d).

Files with missing lines Patch % Lines
pymkv/MKVTrack.py 85.71% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #129      +/-   ##
==========================================
- Coverage   99.11%   98.92%   -0.20%     
==========================================
  Files          21       21              
  Lines        1477     1487      +10     
==========================================
+ Hits         1464     1471       +7     
- Misses         13       16       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

MKVTrack ran mkvmerge -J twice per construction. The file_path setter
called verify_supported(), which fetches the info and returns only a
bool, and the track_id setter then fetched the same JSON again because
_info_json was still empty.

The cause is that _info_json served two roles at once: cache of the
probe, and flag for "caller supplied the info, skip verification". That
same conflation produced #123.

Split them with _info_path, the path the cached info describes:

- the file_path setter probes once and keeps the output, so track_id has
  nothing left to fetch
- pointing a track at a different file drops the cache, so the track
  always describes the file it currently points at
- existing_info is trusted for the first assignment and stops being
  trusted once the path changes
- verify_supported is replaced by info.container.supported, which is
  exactly what that function returns. checking_file_path already
  validated the path, so only the mkvmerge lookup remains, and it is
  cached

This also fixes stale metadata on reassignment. Before, pointing a track
at another file kept the previous file's type and codec, and skipped
verification of the new path:

    t = MKVTrack("sub.srt")     # subtitles
    t.file_path = "video.mkv"
    t.track_type                # 'subtitles' — wrong

20 iterations of MKVFile + add_track + command: 1.56s -> 1.10s, with
add_track itself halved from 1.01s to 0.53s.

Tests dropped 20 patches of verify_supported in the MKVTrack namespace;
they targeted a function that is no longer called and would have passed
even with verification broken. test_track_file_path_setter_verification_failure
now uses a real empty file, which mkvmerge reports as unsupported.
@GitBib
GitBib merged commit 5574958 into master Aug 2, 2026
42 of 45 checks passed
@GitBib
GitBib deleted the fix/add-track-stale-info branch August 2, 2026 13:35
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.

MKVFile.add_track(str) uses stale existing_info from the source file, producing wrong track metadata

1 participant