Added [System.Flags] to enums that are flags. - #354
Conversation
…just in case they come up in the future.
There was a problem hiding this comment.
Pull request overview
This PR updates the unsafe generator and regenerated enum outputs so that enums whose members are all expressed as single-bit shifts (1 << n) are emitted as C# flags enums ([Flags]) and preserve the shift-form instead of collapsing to numeric literals.
Changes:
- Updated enum processing to preserve
= 1 << nexpressions in generated enum member values (with appropriate numeric literal suffixes). - Updated enum generation to emit
using System;and add[Flags]to enums where every member value contains a left-shift. - Regenerated
Enums.g.csoutputs to reflect[Flags]and1 << nformatting for the affected enums (e.g.,SwsFlags,AVTimecodeFlag).
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| FFmpeg.AutoGen/generated/Enums.g.cs | Regenerated enums now include using System;, [Flags] on identified flags enums, and 1 << n member values. |
| FFmpeg.AutoGen.CppSharpUnsafeGenerator/Processing/EnumerationProcessor.cs | Preserves = 1 << n expressions during enum item value conversion via a generated regex. |
| FFmpeg.AutoGen.CppSharpUnsafeGenerator/Generation/EnumsGenerator.cs | Emits using System; and adds [Flags] when all enum items contain <<. |
| FFmpeg.AutoGen.Abstractions/generated/Enums.g.cs | Same regeneration as main package enums: using System;, [Flags], and 1 << n values where applicable. |
# Conflicts: # FFmpeg.AutoGen.Abstractions/generated/Enums.g.cs # FFmpeg.AutoGen/generated/Enums.g.cs
The detection keyed on the header writing a member as "1 << n", which makes the result depend on FFmpeg's source formatting rather than on what the enum means. SWS_BACKEND_C is written "(1 << 1)" and the regex could not match the closing parenthesis; AV_CODEC_HW_CONFIG_METHOD_* and AV_SIDE_DATA_PARAM_CHANGE_* are written in hex. All are bitmasks by their own header documentation and all were missed, and a reformatting upstream could turn the attribute on or off with no change here. EnumerationDefinition now carries IsFlags, computed in the processor from the numeric values: at least two distinct single-bit members, at most a couple of named aggregates such as SWS_BACKEND_ALL, every aggregate covered by bits the enum introduces, sentinels like *_MAX_ENUM ignored, and a consecutive run rejected so that 0, 1, 2 stays a sequence. EnumsGenerator reads that flag instead of sniffing for "<<" in the emitted text. Marks four more: AvCodecHwConfigMethod, AVSideDataParamChangeFlags, AVSideDataProps and SwsBackend. Drops one: AvFrameCrop has a single member, and values alone cannot tell AV_FRAME_CROP_UNALIGNED from AV_HWFRAME_TRANSFER_DIRECTION_FROM/TO, which have the same shape and are a direction rather than a mask. Keeping the shift spelling is now a separate concern from detection, and it is verified: the literal is only kept when 1 << n actually reproduces the value the compiler computed, since C# masks shift counts and would otherwise emit a different constant. The suffix comes from the type name the enum is generated with rather than from a second table of primitives, which had no cases for LongLong and ULongLong - the two that really are C# long and ulong - and disagreed with TypeHelper about Long and ULong. EnumerationItem keeps a numeric Value and gains Literal. Value is what ASTProcessor publishes into WellKnownEnumItems, where it is spliced into macro bodies as text, so an unparenthesised "1 << 3" there would have changed the precedence of any macro referring to the member. Also: invariant culture when formatting values, since several locales render the negative sign as U+2212 and the generated file is C# source; a named regex group per CONTRIBUTING; and "using System;" only when something is actually marked. Co-authored-by: Daniel Sass <twindan@users.noreply.github.com>
|
Thanks for this, and sorry it sat for so long. Two things I want to say before the changes: you filed the issue, implemented it, and then came back and rebased onto 9.0.1.1 the same day it shipped. And the generated files in the branch reproduce byte-for-byte from your generator — I checked with I have pushed a commit onto your branch rather than sending you round another lap. Look it over and push back on anything you disagree with. What I changed and whyThe detection keyed on the header spelling a member as SWS_BACKEND_C = (1 << 1), // parenthesised - the regex is anchored to $
AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX = 0x01, // hex
AV_SIDE_DATA_PARAM_CHANGE_SAMPLE_RATE = 0x0004, // hex, and the header's own docs use it as a maskAll three are documented bitmasks. The sharper worry is that this can flip with no change on our side: if upstream ever reformats So Net effect, 5 enums marked before, 8 now:
Keeping the shift spellingI kept it — reading like the header is genuinely useful when you are porting C code across — but it is now independent of detection and verified, because C# masks shift counts. The suffix comes from the type name the enum is generated with, rather than a second Smaller things
One question for you
Worth knowing for scale, by the way: FFmpeg keeps almost all of its flags in macros, not enums — 149 |
No change to the generated output; the regeneration diff is empty. - ToValue had a ternary whose branches were identical, and it duplicated the signed/unsigned knowledge that ConvertValue already held. There is now one table that produces the number and one line that renders it, with ULong the single case that cannot go through a long - the value was converted twice per member, once for Value and once as the fallback for Literal. Zip hands the number to both - ShiftLiteral returns the literal straight out of the type switch rather than building a suffix and then testing it for null - IsConsecutive compares first and last of an already sorted, distinct set instead of projecting and counting - XML docs on private helpers dropped for the short comments the rest of Processing uses, and System.Collections.Generic is no longer needed Worth recording how the one behavioural slip in here was caught: folding the "a negative member disqualifies" rule into a x > 0 filter did not reject AVDiscard, it dropped AVDISCARD_NONE = -16 and let the remaining scale (8, 16, 24, 32, 48) read as a mask, so AVDiscard gained [Flags]. Regenerating and diffing showed it immediately. Co-authored-by: Daniel Sass <twindan@users.noreply.github.com>
Nothing demonstrated the attribute this pull request adds, while VideoFrameConverter was already passing (int)SwsFlags.SWS_FAST_BILINEAR - a single member with a cast, which reads the same with or without [Flags]. The scaler options now go through a named constant that combines a scaler with a flag, which is what the enum is for and how the C is written. The comment records the part the attribute cannot express: the low bits pick the scaler and only one may be active, the rest combine freely. Co-authored-by: Daniel Sass <twindan@users.noreply.github.com>
Description
Some enums that are conceptually flags (e.g., SwsFlags) do no have [System.Flags] on the enum definition. This code adds that definition for the relevant classes.
What I do is look at the enum definition in ffmpeg when parsing. If it is of the form "1 << n", then I preserve the definition instead of collapsing the value (so the generated file say "VALUE = 1 << 3" instead of "VALUE = 8"). If all items in an enum are of the form 1 << n, then I treat it as a [System.Flags]. This seems to be how ffmpeg uses enumerated values.
Type of Change
Related Issues
#353
Testing
Ran the output
dotnet build -c Releasesuccessfullydotnet test -c ReleasesuccessfullyChecklist
Additional Notes
Output is the same except for enums that now have [System.Flags] appended. I have included the new generated files in this PR. Ran against ffmpeg commit 9047fa1b.