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

This adds support for empty 'fill' boxes while decoding MXF files.
---
 libavformat/mxfdec.c | 7 +++++++
 1 file changed, 7 insertions(+)

ons 2024-09-11 klockan 10:15 +0200 skrev Martin Schitter:
> This adds support for empty 'fill' boxes while decoding MXF files.
> ---
>  libavformat/mxfdec.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 142b3e6..701fc9f 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -367,6 +367,8 @@ static const uint8_t
> mxf_mca_rfc5646_spoken_language[]     = { 0x06,0x0e,0x2b,0x
>  
>  static const uint8_t mxf_sub_descriptor[]                  = {
> 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x09,0x06,0x01,0x01,0x04,0x06,0x10
> ,0x00,0x00 };
>  
> +static const uint8_t mxf_fill_key[]                        = {
> 0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x02,0x03,0x01,0x02,0x10,0x01,0x00
> ,0x00,0x00 };
> +
>  static const uint8_t mxf_mastering_display_prefix[13]      = {
> FF_MXF_MasteringDisplay_PREFIX };
>  static const uint8_t mxf_mastering_display_uls[4][16] = {
>      FF_MXF_MasteringDisplayPrimaries,
> @@ -3730,6 +3732,11 @@ static int mxf_read_header(AVFormatContext *s)
>                  continue;
>          }
>  
> +        if (IS_KLV_KEY(klv.key, mxf_fill_key)){
> +            avio_skip(s->pb, klv.length);
> +            continue;
> +        }

This could also be done using mxf_metadata_read_table[] using a simple
stub callback.

Alternatively we could add some logic to the parsing loop that skips
KLVs whose key's entry in the table have read == NULL. The loop
termination condition would need to either change to checking if the
first byte of the UL is zero, or maybe just use FF_ARRAY_ELEMS. This
way we could add more keys to skip in the future. This is also possible
with the stub approach

Another thing we could do while we're at it is to add a matching length
to MXFMetadataReadTableEntry, which would allow simplifying the table

Also IS_KLV_KEY() seems wrong. It should skip the version byte I think
(I'd have to double-check the spec). That doesn't need to hold up this
patch of course

/Tomas

On 11.09.24 10:15, Tomas Härdin wrote:
> This could also be done using mxf_metadata_read_table[] using a simple
> stub callback.
> 
> Alternatively we could add some logic to the parsing loop that skips
> KLVs whose key's entry in the table have read == NULL. The loop
> termination condition would need to either change to checking if the
> first byte of the UL is zero, or maybe just use FF_ARRAY_ELEMS. This
> way we could add more keys to skip in the future. This is also possible
> with the stub approach

Yes -- it could be done in many ways!

I have indeed chosen a very simple and trivial solution. experts, which 
are more familiar with this code base, may very likely see and even 
prefer more complex alternatives. But the trivial variant, which is very 
easy graspable by any developer, also has its value as long as it does 
its job.

But an elementary feature like 'fill' should be simply supported by any 
MXF demuxer in a suitable manner, otherwise it's IMHO a grave defect.

> Also IS_KLV_KEY() seems wrong. It should skip the version byte I think
> (I'd have to double-check the spec). That doesn't need to hold up this
> patch of course

IS_KLV_KEY is just used as a trivial memcmp paraphrase for static keys:

  #define IS_KLV_KEY(x, y) (!memcmp(x, y, sizeof(y)))


 > That doesn't need to hold up this patch of course

If you don't see any further objections, please just merge the patch and 
finally solve this issue by this trivial solution till someone actually 
contributes a better alternative.

Martin

>De : ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> De la part de martin schitter
>Envoyé : jeudi 12 septembre 2024 02:36
>
>But an elementary feature like 'fill' should be simply supported by any MXF demuxer in a suitable manner, otherwise it's IMHO a grave defect.

>If you don't see any further objections, please just merge the patch and finally solve this issue by this trivial solution till someone actually contributes a better alternative.

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 ?

Thanks
Nicolas

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...

martin

>De : ffmpeg-devel <ffmpeg-devel-bounces@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

On 13.09.24 10:23, Nicolas Gaullier wrote:
> I am not against the idea of the patch: filler items should not be logged as dark metadata. 

Fine! -- that's the main point!

> 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.

It adds a small amount of support, because without adding this key entry 
you can not identify and selective process 'fill' boxes.

Sure -- the files are already playable, and the processing is even 
faster without any further handling of this specific kind of content, 
but you will never be able to suppress the misleading verbose logging 
without such a modification.

> - why not using a simple "if" on the av_log rather than inserting a new block of code ?

Somehow you always have to differentiate 'fill' boxes from other unknown 
elements if you want to change the present behavior.

Sure -- it would be possible to change just the printed output in this 
already present `av_log` call, but the necessary code would IMHO much 
more "confusing":

a selective handling of a specific kind of entity in a branch of the 
code flow, which is strictly dedicated to warn about "unknown" content. 
That's simply a paradox, a developers joke or just something highly 
"confusing".

But change and rename my patch however you like!

I can not do more than suggesting this very simple improvement.
Or at least something, which I personally would see as an improvement.

martin

fre 2024-09-13 klockan 08:23 +0000 skrev Nicolas Gaullier:
> 
> - why not using a simple "if" on the av_log rather than inserting a
> new block of code ?

That's way ugly

RE: performance, we might want to move to a hash table based approach
so we don't scan the entire table every time. The approach in this
patch would be detrimental to such a change (it's an extra if that's
always evaluated), hence why I prefer a table based approach. I'll post
an alternative patch in a bit

Also I just noticed the "Dark key" nag only get printed with -loglevel
verbose. Hence why it's gone unnoticed

/Tomas

On 13.09.24 13:59, Tomas Härdin wrote:
> RE: performance, we might want to move to a hash table based approach
> so we don't scan the entire table every time. The approach in this
> patch would be detrimental to such a change (it's an extra if that's
> always evaluated), hence why I prefer a table based approach. I'll post
> an alternative patch in a bit

Your suggested alternative looks indeed much more elegant! :)

Martin