[FFmpeg-devel] [PATCH v2 07/13] cbs: Implement common parts of cbs-based bitstream filters separately

[Mark Thompson]

On 19/01/2021 03:07, James Almer wrote:
> On 1/18/2021 7:44 PM, Mark Thompson wrote:
>> +#define BSF_ELEMENT_OPTIONS_PIR(name, help, field, unit_name) \
> 
> You could reuse name instead of also passing unit_name. The only bsf where it differs is h264_metadata with display_orientation vs disp_or, and changing the latter into the former should be fine (Is it even user facing?).

I don't think it can have any user effect, though technically they can inspect it.  So, sure, that's simpler.

>> +    { name, help, OFFSET(field), AV_OPT_TYPE_INT, \
>> +        { .i64 = BSF_ELEMENT_PASS }, \
>> +        BSF_ELEMENT_PASS, BSF_ELEMENT_REMOVE, FLAGS, unit_name }, \
> 
> This depends on FLAGS being defined before this macro is invoked. It's probably safer and/or more robust to pass the flags as an argument.

All BSFs with options use this idiom, so it felt safe ("grep -r AV_OPT_FLAG_BSF_PARAM libavcodec/").

I guess it is slightly surprising to have that extra implicit argument, though, so no objection to making it explicit.

>> +    { "pass",   NULL, 0, AV_OPT_TYPE_CONST, \
>> +        { .i64 = BSF_ELEMENT_PASS   }, .flags = FLAGS, .unit = unit_name }, \
>> +    { "insert", NULL, 0, AV_OPT_TYPE_CONST, \
>> +        { .i64 = BSF_ELEMENT_INSERT }, .flags = FLAGS, .unit = unit_name }, \
>> +    { "remove", NULL, 0, AV_OPT_TYPE_CONST, \
>> +        { .i64 = BSF_ELEMENT_REMOVE }, .flags = FLAGS, .unit = unit_name }
>> +
>> +#define BSF_ELEMENT_OPTIONS_PIRE(name, help, field, unit_name) \
>> +    { name, help, OFFSET(field), AV_OPT_TYPE_INT, \
>> +        { .i64 = BSF_ELEMENT_PASS }, \
>> +        BSF_ELEMENT_PASS, BSF_ELEMENT_EXTRACT, FLAGS, unit_name }, \
>> +    { "pass",   NULL, 0, AV_OPT_TYPE_CONST, \
>> +        { .i64 = BSF_ELEMENT_PASS   }, .flags = FLAGS, .unit = unit_name }, \
>> +    { "insert", NULL, 0, AV_OPT_TYPE_CONST, \
>> +        { .i64 = BSF_ELEMENT_INSERT }, .flags = FLAGS, .unit = unit_name }, \
>> +    { "remove", NULL, 0, AV_OPT_TYPE_CONST, \
>> +        { .i64 = BSF_ELEMENT_REMOVE }, .flags = FLAGS, .unit = unit_name }, \
>> +    { "extract", NULL, 0, AV_OPT_TYPE_CONST, \
>> +        { .i64 = BSF_ELEMENT_EXTRACT }, .flags = FLAGS, .unit = unit_name } \
Thanks,

- Mark