Repository navigation
Replace Haali with libmatroska - #692
Draft
CoffeeFlux wants to merge 28 commits into
Draft
CoffeeFlux wants to merge 28 commits into
CoffeeFlux wants to merge 28 commits into
Conversation
CoffeeFlux
marked this pull request as ready for review
August 27, 2026 22:20
agi::matroska::Demuxer was implemented on top of a reimplementation of the Haali MatroskaParser C API, which in turn wrapped libebml and libmatroska. Fold the parts that are actually used directly into the demuxer and delete the C layer along with its licence notice. Behavior changes: - Attachment payloads are only located while opening a file rather than read into memory, which libebml's SCOPE_PARTIAL_DATA did not prevent. - Tracks with encryption or an unsupported content encoding are reported as unsupported instead of making the whole file fail to open. - Subtitle tracks are no longer limited to the first 32 tracks. - Reader failures while opening are reported as IoError. - Duration() is nullopt when the file doesn't specify one. - A cluster which fails to parse while computing the duration from the end of the file is skipped rather than failing to open the file. - Default per-packet limits are 16 MiB to match 5a53f19. The fixture test now dumps through the public Demuxer interface as part of the gtest suite instead of a separate executable using the C API. Its output on the existing fixtures is unchanged from the previous implementation other than unknown durations, and a fixture covering header stripping, block durations and encrypted tracks is added. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- The demuxer's cancellation callback captured the metadata progress dialog's sink, which is destroyed when that dialog finishes and was then used while reading subtitles. Route it to whichever dialog is running. - Errors while reading track information were only logged to the progress dialog, after which the empty demuxer was dereferenced. Rethrow them. - Restore the 64 MiB limit on total subtitle data from 5a53f19. - Report malformed ASS packets instead of letting bad_lexical_cast escape. - Restore the file's original licence header. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CoffeeFlux
force-pushed
the
libmatroska-replacement
branch
2 times, most recently
from
October 7, 2026 18:13
7672447 to
c0c2969
Compare
- Honor ContentEncodingScope: compression which doesn't cover frame contents is no longer applied to frames, and a zlib-compressed CodecPrivate is decompressed. - Skip track entries whose UID was already seen, as Tracks may be repeated in live streams, rather than failing on the duplicate number. - Make AttachmentId an index and expose the UID separately, so that attachments with missing or duplicate UIDs are no longer dropped, and skip attachments without data as the Haali parser did. - Fix signed overflow when a block's relative timestamp is added to a cluster timestamp of INT64_MAX. - Poll for cancellation per element while parsing a cluster, as clusters can be arbitrarily large. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Make block timestamps relative to the first block in the file, as the Haali parser did. Video providers normalize frame timestamps to start at zero, so without this subtitles in files which don't start at zero were imported out of sync. - Apply the deprecated TrackTimecodeScale to block timestamps and durations, as the Haali parser did. - Skip elements such as Void between the EBML header and the Segment. libebml's FindNextID returns whatever element comes next, so these were being treated as the Segment, and anything at all was being accepted as the EBML header. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Opening a file walked every top-level element with libebml, indexing all clusters before returning. MatroskaWrapper::HasSubtitles does this on the UI thread whenever video is opened, which means reads from throughout the file. Like the Haali parser, stop at the first cluster and follow the SeekHead for metadata stored after the clusters, only walking the entire file if the tracks still haven't been found. Clusters are now indexed as packets are read, and the fallback duration is computed the first time Duration() is called rather than while opening the file. This walks the segment with the raw EBML reader and only uses libebml to parse Info and Tracks. That fixes a Void before a CRC-32-prefixed Info or Tracks element leaving libebml's nesting level negative, which made it stop reading the element's children. The existing video-only fixture, written by ffmpeg, has this layout and was losing its duration. Also: - Compute block timestamps relative to the first block exactly, so only the relative timestamp needs to fit in 64 bits. - Find the end of unknown-size clusters, as live muxers write. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The fixture test compares describe() output with the .behavior files byte-for-byte, which failed when Git converted them to CRLF on checkout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The demuxer now reports timestamps as stored in the file rather than relative to the first block, and exposes the earliest audio or video timestamp as StartTime(). This is what FFmpeg (and so MPC-HC/LAV) reports as the start time, and what mpv uses for Matroska in practice, and both rebase playback so that it is time zero. MatroskaWrapper subtracts it from imported subtitles. This matches the Haali parser for nearly all files, as their first block is normally audio or video, while fixing two cases where that differed: - Files with only subtitles are no longer shifted to start at zero (TypesettingTools#91), as they are never played by themselves. - A subtitle line before the first audio or video block no longer moves every other line earlier. Lines which start before the start of the file are clamped to start at zero, and lines which end before it are dropped. Aegisub's own video timeline makes the first video frame time zero instead (TypesettingTools#21), so in files where video starts after audio, imported lines appear late by that delay, as they did before. They are exported with their original timing, and this will be consistent if the video timeline is changed to use the start of the file. The deprecated TrackTimecodeScale is now applied to the stored timestamp as the specification describes, rather than to the offset from the first block. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- A cluster cut off by the end of the file was rejected entirely, which failed to open the file if it was the first cluster and otherwise dropped its subtitles. Read the blocks in it which are complete. - Walk the whole file when Info, not only Tracks, wasn't found before the first cluster or via a SeekHead, as otherwise timestamps are scaled with the default TimestampScale. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Read clusters a block at a time rather than collecting every frame in a cluster before returning the first. Clusters have no size limit and lacing turns each block into up to 256 frames, so a crafted file could use an unbounded amount of memory before any packet limit was checked. - Choose libebml based on which libmatroska is used. With an older system libebml and no system libmatroska, the bundled libmatroska requires a newer libebml than the one already found, which failed to configure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
RFC 9559 section 11.2 gives a block's timestamp as (Cluster\Timestamp + block timestamp * TrackTimestampScale) * TimestampScale, so the track scale doesn't apply to the cluster's timestamp. The previous commit wrongly scaled the sum. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CoffeeFlux
marked this pull request as draft
October 7, 2026 22:53
Rather than choosing libebml based on which libmatroska was found, raise the minimums to the bundled versions so a system libebml is always new enough for the bundled libmatroska. Older distributions such as Ubuntu 22.04 now use the bundled copies of both. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
It was kept separate so the matroska-behavior executable could link it without everything else, but that has since been folded into the gtest suite. This also restores the executable's original dependencies line, which had lost a space of indentation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces the bundled Haali
MatroskaParser.cwith a C++ demuxer,agi::matroska::Demuxer(src/matroska.{h,cpp}), whichMatroskaWrappernow uses for subtitle import andHasSubtitles.Design
InfoandTracks. Everything else (the top-level walk,SeekHead,Attachments, clusters and blocks) uses a small bounded EBML reader, so only the bytes that are needed get read.SeekHeadfor metadata stored after the clusters, like the Haali parser did. It only walks the whole file ifInfoorTracksstill haven't been found.HasSubtitlesruns on the UI thread whenever video is opened, so this matters on slow storage.IoError,InvalidDataError,LimitError, ...), size limits, and cancellation.Behavior changes from master
Subtitle timing. The demuxer reports timestamps as stored in the file.
StartTime()is the earliest audio or video timestamp: what FFmpeg (and so LAV/MPC-HC) reports as the start time, and what mpv rebases to for Matroska in practice. Import makes that time zero.Other changes
ContentEncodingScopeis honored, including a zlib-compressedCodecPrivate.Trackselements are tolerated.TrackTimecodeScaleis applied as RFC 9559 §11.2 specifies.Import UI fixes
bad_lexical_cast.Testing
tests/matroska/fixturesholds 14 small files. Most are generated bygenerate.py, which builds the edge cases directly as EBML. A gtest dumps everything the demuxer exposes for each file and compares it with the hand-written.behaviorfile next to it.MatroskaParser.c. The remaining differences are the intended ones listed above.HasSubtitleson a cold cache takes about 1 ms, compared with about 250 ms when walking every cluster.Not tested: importing through the GUI (progress dialogs, cancelling, the track chooser).
Known limitations
Info/Tracksdeclares, so a crafted file can make it allocate up to about 2 GB. This is libebml's behavior and is left as is.🤖 Generated with Claude Code