[FFmpeg-devel] [PATCH] libavcodec: implementation of DNxUncompressed decoder

[martin schitter]

Dear Andreas!

Thanks for taking a look.
I'm not familiar with ffmpeg development and it's really hard to figure 
out the more complex demands and hidden secrets of all the projects 
tooling and workflow requirements.

On 08.09.24 10:47, Andreas Rheinhardt wrote:
> [...] +++ b/libavcodec/dnxucdec.c
> 
>> +
>> +const AVCodecParser ff_dnxuc_parser = {
>> +    .codec_ids      = { AV_CODEC_ID_DNXUC },
>> +    .priv_data_size = sizeof(DNxUcParseContext),
>> +    .parser_parse   = dnxuc_parse,
>> +};
>> +
>> +const FFCodec ff_dnxuc_decoder = {
>> +    .p.name         = "dnxuc",
>> +    CODEC_LONG_NAME()"DNxUncompressed (SMPTE RDD 50)",
> 
> This shouldn't even compile.

I already fixed this obvious little glitch in a quick reply to michaels 
review yesterday.

btw: could you please give me an advice, how this kind of trivial 
corrections should be handled on this list?

Should I just post the individual correction as patch in a followup 
message or always post the numerated full patch set again or even a 
squashed variant of the changes for better readability in context?

>> +    .p.type         = AVMEDIA_TYPE_VIDEO,
>> +    .p.id             = AV_CODEC_ID_DNXUC,
>> +    .init           = dnxuc_decode_init,
>> +    FF_CODEC_DECODE_CB(dnxuc_decode_frame),
>> +    .p.capabilities = AV_CODEC_CAP_FRAME_THREADS,
>> +};
>> \ No newline at end of file
> 
> This should be fixed.

O.k. that's easy to solve...

btw: what particular linter or make argument should I use to get this 
more picky warnings and errors myself without access to fancy CI/CD test 
reports?

>> diff --git a/libavcodec/version.c b/libavcodec/version.c
>> index 27f9432..c3b576a 100644
>> --- a/libavcodec/version.c
>> +++ b/libavcodec/version.c
>> @@ -31,7 +31,7 @@ const char av_codec_ffversion[] = "FFmpeg version " FFMPEG_VERSION;
>>   
>>   unsigned avcodec_version(void)
>>   {
>> -    static_assert(AV_CODEC_ID_LEAD         ==   269 &&
>> +    static_assert(AV_CODEC_ID_LEAD         ==   270 &&
>>                     AV_CODEC_ID_PCM_SGA      == 65572 &&
>>                     AV_CODEC_ID_ADPCM_XMD    == 69683 &&
>>                     AV_CODEC_ID_CBD2_DPCM    == 81928 &&
> 
> This is by definition the wrong fix. Instead you should do as the assert
> message tells you: Don't insert your new ID in the middle of the list.

well -- that's indeed a real issue!

I needed few attempts to figure out how this somehow works at all, 
because I'm perhaps to stupid to understand/follow the relevant section 
in the documentation 
(https://ffmpeg.org/developer.html#New-codecs-or-formats-checklist):

If I add a new ID somewhere in the relevant section of codec_id.h the 
LEAD-element position will be shifted in any case. Isn't this the 
reason, why the minor version number in libavcodec/version.h must be 
bumped too?

Wherever possible I added changes for DNxUncompressed close to other 
DNx* entries in alphabetic order, but in codec_id.h and codec_desc.c I 
placed it at the end of the relevant section immediately before the 
LEAD-entry to minimize incompatibility.

Please simply show me how this particular issue can be solved in a more 
desired manner.

> Didn't look closely at the rest (but it already seems like there are
> many issues).

Please just help me figure out all relevant issues!
I'm really willing to improve my contribution.

thanks!
martin