[FFmpeg-devel] [PATCH] libavformat/mxfdec.c: Recognize and Ignore MXF fill boxes

[Nicolas Gaullier]

>De : ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> De la part de martin schitter
>Envoyé : jeudi 12 septembre 2024 13:54
>On 12.09.24 13:14, Nicolas Gaullier wrote:
>> The message "Recognize and Ignore" does not make it clear what issue or grave defect is solved here.
>> I see in the code that fill items are currently recognized as dark metadata and ignored likewise, but I don't see any issue here.
>> Maybe could you comment a little bit about your intent ?
>
>While developing this DNxUncompressed code I always got lots of this "Dark key ..." log messages in the debug output. This kind of output wouldn't be a surprise, if the key belongs to some rare and utterly irrelevant data >box. but in this particular case the key stands for empty "fill" blocks, which are frequently used in various places in MXF files. They are an elementary building block of this container format.
>
>On the muxer side of ffmpegs MXF code 'fill' is known and used in many places, but the demuxer doesn't recognize this element and just always prints these warnings about something "unknown". That's highly irritating and >also inefficient, because 'fill' is used quite frequently e.g. as place holder to align frame data on 256byte boundaries etc.
>
>It's really trivial to fix and I don't see, why we should debate any longer about this obvious flaw instead of just quickly solving the issue.
>
>But if you want, you can rewrite the wording to "Recognize 'fill' in MXF data and suppress output" or whatever you like...

I am not against the idea of the patch: filler items should not be logged as dark metadata. I just wanted to check with you that it was indeed the only issue.
So it is not a big issue, and I find the patch confusing both because of the commit msg and code:
- the commit msg should not claim it "adds support for" filler items: mxf files with fillers are already supported/playable.
Maybe just a single line like "avformat/mxfdec: suppress verbose log for fillers" ?
- why not using a simple "if" on the av_log rather than inserting a new block of code ?

Thank you,
Nicolas