[ABI BREAK] v4++ MarginB/T #651
Open
Conversation
This file contains 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
|
In MPC the
|
Masaiki
reviewed
Dec 27, 2022
This does not actually implement any v4++ features yet, but prepares for commits which do. As far as I can tell, no tool ever wrote or parsed Format lines for v4++ files, so in the interest of better compatibility, libass' custom format line extension is disabled when parsing v4++. With no canonical Format line to go by, this means the internal pseudo-format line and the new field names used for parsing v4++ are a fabrication of this commit. Albeit, the naming is corroborated by both VSFilter's internal names, existing Margin* naming and the naming used in asa's AS5 wiki page[1]. Notably, the new field names will leak to custom format line users who will now be able to use them outside of v4++, but this seems mostly harmless as we won't need to change the fabricated field names. [1]: https://web.archive.org/web/20071030224805/http://asa.diac24.net/AS5
But no implementation yet. Implementations for the new Margin properties and v4++'s \kt tag will be added in follow-up commits. For RelativeTo no implementation is immediately planned, but given xy-VSFilter also dropped support for RelativeTo, this doesn't seem pressing. A field is already added and initalised to VSFilter's default anyway, to spare us another ABI break if a guliverkli(2)-like implementation is ever added. The top margin keeps the MarginV name to not break public API as the Event and Style properties are exposed in ass_types.h. If we want to preserve full API compatibility even with purely API-created ASS_Styles, we'll need to put version conditionals into the rendering, not only the parsing, part. If we only cared about preserving API for _reading_ the fields: An alternative would be to alias MarginV to MarginT via anonymous union, to preserve API while only using the clearer T/B names internally. However, anonymous unions are not part of ISO C99, and while requiring ISO C11 or the widespread earlier compiler extension enabling this might be fine for *building* libass, requiring as part of our public API seems more problematic so this just stick with the MarginV name only. (An anonymous union would also help somewhat with easing a potential future API-transition from MarginV to MarginT by allowing API-users to already use the new name now.) Even with API preserved, this still breaks ABI since arrays of styles and events are exposed in the API and their element size changed by adding new fields to those structs.
MarginB Style and Event are ignored when not in v4++ mode, but as style overrides all Margin* fields act independent of version with MarginV always setting both fields and MarinT/B only ever one. In order to not break API, some complexity had to be introduced: To keep API compatibility with styles created via API, we need to decide at rendering time between using the styles MarginV or MarginB as the bottom margin based on track version. Since such styles never go through our parsing logic and MarginB didn't exist until now, they'll might have MarginB set to 0 but expect MarginV to apply to both. To keep API-compatibility with a mix of API-created styles and user overrides, it is necessary to track per style if it was affected by a MarginT or MarginB override. If either was overriden, we'll use the split margin values at render time even for non-v4++ files. Access to MarginB needs to be tracked separate from MarginT, otherwise incompatibilities can still arise. Consider an override applied to an API-created style which originally didn't set MarginB: If we didn't know whether MarginB was already set by an override, overrinding only MarginT would either suddenly set MarginB to zero or destroy the value set by a previous override. After a potential future API break, renaming the MarginV struct member to MarginT and requiring API-created styles to set both independent of version, all this complexitiy can be dropped. TODO: Before merging the override bitmap logic should best be checked carefully again as it wasn't examined or tested carefully yet.
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.
Not for immediate merge.
This breaks ABI but is by itself not too useful, so it is postponed until it can be confalted with another ABI or API break or perhaps until at least the more useful
RelativeTois also implemented.The first actual commit of this is “Recognise v4++ files”, prior ones are from #649 and once this is merged I'll rebase this to get rid of them.
In its current state this implements:
MarginB/Twith added complexity to ensure its only an ABI and not also an API break.If this gets conflated with an API break, this complexity can be removed and
MarginVstruct members renamed toMarginT.Missing for full v4++ support is
RelativeTo, we currently assume this field allows to basically enableuse_marginsper Style. However, xy* dropped support for it completly, and renderers using the external VSFilter interface cannot meaningfully act on it. Thus tests to confirm our understanding must use guliverkli(2) MPC with its internal subtitle renderer and may additionally also test MPC-HC'S ISR which did not just retain but expandRelativeTo-related logic.As a side note: if we end up breaking API anyway, we can also drop
treat_fontname_as_pattern, which is currently still used in MPlayer (and in MPlayer's internal libass copy the field actually still has an effect). Ideally MPlayer would receive patches before the field is removed from libass.Just in case the branch disappears, here are the commits as format-patches including a patch for
libass-tests: v4++_api-break_patches.tar.gz