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

[martin schitter]

Thanks for this careful review.

On 09.09.24 12:56, Anton Khirnov wrote:
> Quoting Martin Schitter (2024-09-08 20:41:38)
>> diff --git a/libavcodec/dnxucdec.c b/libavcodec/dnxucdec.c
>> new file mode 100644
>> index 0000000..455c374
>> --- /dev/null
>> +++ b/libavcodec/dnxucdec.c
>> @@ -0,0 +1,495 @@
>> +/*
>> + * Avid DNxUncomressed / SMPTE RDD 50 demuxer
> 
> This says it's a demuxer, while it's implemented as a decoder.

Fixed.

> I'm also wondering if this shouldn't be handled as demuxer exporting
> raw video, at least in some of cases if not all.

When I started the work, I also thought, it will not require much more 
than minimal modifications of MXF related raw data transport details, 
but during reverse engineering the actually used dense bitpacking turned 
out to be more complicated.

The codec section should fit rather well for this component, although 
large parts of it could be also categorized just as bitstream filter.

>> +typedef struct DNxUcParseContext {
>> +    uint32_t fourcc_tag;
>> +    uint32_t width;
>> +    uint32_t height;
>> +    uint32_t nr_bytes;
>> +} DNxUcParseContext;
> 
> Parser should be in its own file, as it can be enabled or disabled
> independently from the encoder.

O.k. that's possible.

Done.

> Opening brace of a function body should be on its own line.

Fixed.

>> +            av_log(0, AV_LOG_ERROR, "can't read DNxUncompressed metadata.\n");
> 
> av_log() should always get a proper logging context, avctx in this case

Fixed.

>> +            *poutbuf_size = 0;
>> +            return buf_size;
>> +    }
>> +
>> +    pc->fourcc_tag = *(uint32_t*)(buf+24);
>> +    pc->width =  *(uint32_t*)(buf+16);
>> +    pc->height =  *(uint32_t*)(buf+20);
>> +    pc->nr_bytes = *(uint32_t*)(buf+29) - 8;
> 
> Use AV_RL32()

Done.

>> +    ret = ff_thread_get_buffer(avctx, frame, 0);
>> +    if (ret < 0)
>> +        return ret;
> 
> This call should not be duplicated in every handler.

Eliminated by some refactoring and an additional helper function.

>> +    if (avctx->width % 4){
>> +        av_log(0, AV_LOG_ERROR,
>> +        "Image width has to be dividable by 4 for 10bit RGB DNxUncompressed!\n");
>> +        return AVERROR_EXIT;
>> +    }
> 
> These checks can be done once during init.

Yes -- A line width check for a multiple of 2 is now done only once for 
all yuv4:2:2 compression variants. This is more standard compliant and 
should work for all implemented format handlers.

>> +    ret = ff_thread_get_buffer(avctx, frame, 0);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    lw = frame->width;
>> +
>> +    for(y = 0; y < frame->height; y++){
>> +        for(x = 0; x < frame->width; x++){
>> +            msp = pkt->data[y*3*(lw + lw/4) + 3*x];
> 
> Does anything guarantee the packet is large enough?

I added another packet size check, which should calculate the needed 
size of data for this kind of very dense bit packed input.

I hope you like the chosen solutions.

Martin