Store EBML element defaults in the schema and resolve zero-length elements - #86
Merged
Conversation
ashleyz
requested changes
Aug 24, 2026
Declare each element's spec default in the descriptor tables and copy it into storage during parsing, so absent elements read back as their default instead of via ad-hoc constants scattered across the accessor call sites. The read flag becomes a tri-state so a value taken from the file is distinguishable from a default. Defaults a static table can't express (DocType, dynamic DisplayWidth/Height, BitDepth) stay at their call sites.
For fields with a schema default, ne_get_uint/ne_get_float always write the destination, so the "value = N" presets in the parameter accessors were dead stores, and misleading where the real default was non-zero (e.g. 2 for MatrixCoefficients). Replace them with a guard on the return value, assigning only on success; params is zeroed up front so the impossible unread path degrades to zero. Presets encoding a real fallback (BitDepth 16, dynamic DisplayWidth/Height, the mastering NaN sentinels) are kept.
Since the upstream zero-length change (1c9936b) a zero-length numeric encoding reads as 0, but an empty element with a declared default must resolve to that default, so a zero-length TimestampScale wrongly read as 0 and broke duration and block-duration scaling. In ne_read_simple, keep the value already installed at init (the default, or zero when none is declared) instead of reading a zero-length element. Add zerolen_default.webm, which the suite missed because bug2045549.webm only covers a zero-length cluster Timestamp on the direct read path.
zerolen_default.webm only exercises a zero-length element with a declared uint default (TimestampScale), so the float path had no coverage. Add zerolen_audio.webm, holding a zero-length SamplingFrequency, which resolves to the declared default of 8000.0, and a zero-length BitDepth, which reads back as 0 rather than the 16 reported for an absent BitDepth. The next commit fixes the BitDepth case and updates the expectation.
kinetiknz
force-pushed
the
schema-defaults
branch
from
August 25, 2026 02:49
f98ec68 to
5f31daf
Compare
ashleyz
reviewed
Aug 25, 2026
DisplayWidth, DisplayHeight and BitDepth carry no default in the element descriptors: the first two derive theirs from PixelWidth and PixelHeight, and BitDepth has no declared default at all, with 16 supplied as nestegg API policy. A zero-length instance of any of them was therefore read as a value of 0, while an absent one picks up the accessor's default, so an empty DisplayWidth reported a display width of 0 instead of the pixel width. RFC 8794 Section 6.1 defines the value of an Empty Element only for elements with a declared default (the default) and without one (zero); these have neither, and RFC 9559 declares all three range="not 0", so zero cannot be what the file meant. Mark them DESC_FLAG_ACCESSOR_DEFAULT and record a zero-length instance as read but valueless, so the accessor resolves it exactly as it resolves an absent element. libavformat's matroskadec reaches the same result by resolving an empty element to its syntax table default, where DisplayWidth and DisplayHeight hold the sentinel it uses for absent. Read but valueless is a state of its own, EBML_READ_EMPTY, because the two users of the read state want different answers for it. The parser skips repeated instances of an element it has already read, so an empty instance has to count as read or a later instance would override it, breaking the first instance wins rule that applies everywhere else. The accessors, on the other hand, must see it as absent to supply the default. Neither can use EBML_READ_DEFAULT, which is established before parsing and so has to leave the element open to being read. Add zerolen_display.webm and zerolen_repeat.webm, the latter covering a DisplayWidth repeated after an empty instance and before one, and update the zerolen_audio.webm expectation.
BlockAdditions are parsed outside the element descriptors, and that manual parse overwrote the preset default with the zero ne_read_uint returns for a zero-length element, which the following add_id == 0 check then rejected: a zero-length BlockAddID failed the whole packet read rather than resolving to 1, the default RFC 9559 Section 5.1.3.5.2.3 declares for it. Read the value only when the element is not empty. libavformat's matroskadec defaults the same element to 1. Add zerolen_blockaddid.webm.
kinetiknz
force-pushed
the
schema-defaults
branch
from
August 25, 2026 21:08
5f31daf to
8b1cb03
Compare
Collaborator
Author
|
Thanks for the review! |
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
2 participants
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.
Centralizes per-element default values in the descriptor tables so absent elements read back as their spec default, replacing the ad-hoc default constants that were scattered across the accessor call sites (and dropping the presets those left dead).
Builds on the merged zero-length change (1c9936b): per RFC 8794 §6.1 an empty element with a declared default must resolve to that default rather than 0, which fixes cases like a zero-length TimestampScale that previously read as 0 and broke duration and block-duration scaling.
Behavior is unchanged for well-formed files (the regress suite is byte-identical), and zerolen_default.webm is added to cover the empty-defaulted-element case the existing suite missed.