On Wed Sep 28 21:13:10 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list:
On 9/27/22 03:32, Rémi Bernon wrote: > +/* List all supported raw video formats, with their metadata. > + * > + * Each element should be X(wg, guid, depth, type), with: > + * wg: suffix of the GStreamer constant, and enum wg_video_format value. > + * guid: suffix of the corresponding MEDIASUBTYPE / MFVideoFormat guid. > + * depth: the number of bits of each video sample. > + * type: whether the video format is RGB or YUV. > + * > + */ > +#define FOR_EACH_WG_VIDEO_FORMAT(F) \ > + F(BGRA, ARGB32, 32, RGB) \ > + F(BGRx, RGB32, 32, RGB) \ > + F(BGR, RGB24, 24, RGB) \ > + F(RGB15, RGB555, 16, RGB) \ > + F(RGB16, RGB565, 16, RGB) \ > + F(AYUV, AYUV, 32, YUV) \ > + F(I420, I420, 12, YUV) \ > + F(NV12, NV12, 12, YUV) \ > + F(UYVY, UYVY, 16, YUV) \ > + F(YUY2, YUY2, 16, YUV) \ > + F(YV12, YV12, 12, YUV) \ > + F(YVYU, YVYU, 16, YUV) \ > + > +enum wg_video_format > +{ > + WG_VIDEO_FORMAT_UNKNOWN = 0, > +#define X(x, g, d, t) WG_VIDEO_FORMAT_ ## x, > + FOR_EACH_WG_VIDEO_FORMAT(X) > +#undef X > +}; I don't hate this, but I'm also less than thrilled about it. One of the nice things about spelling things out explicitly is that it's easy for someone unfamiliar with the code to grep for constants to see how they're converted. This also puts some burden on the reader to mentally preprocess the code first. It's probably not a big deal, since it's not difficult to understand what it's doing, but I'm not really sure it's worthwhile in this case.
In my opinion the advantages of generating all these arrays from the same macro outweight the drawbacks, especially when the mappings are spread out like that in several files, converting the wg_format to multiple formats back and forth.
When there's a switch on the enum the compiler will catch missing values but with the arrays it's very easy to miss one, and hard to validate that all the values are handled, and in a consistent way with the other mappings elsewhere.
To allow grepping the names, instead of just keeping the prefix it could use the full wg name, and keep the gst suffix, guid, etc... Though imho it gets a bit too verbose and not very necessary (like I agree you can't grep for the enum values but it's always easy to get to the enum declaration and lookup how it generates the value, or grep for the macro usage).