[FFmpeg-devel] [PATCH v11 1/3] libavcodec/dnxucdec: DNxUncompressed decoder

Anton Khirnov anton at khirnov.net
Fri Oct 11 11:57:00 EEST 2024


Quoting Martin Schitter (2024-10-11 03:08:23)
> On 10.10.24 14:13, Anton Khirnov wrote:
> > I've already commented on this in a previous version - this should be
> > directly exported as rawvideo by the demuxer rather than requiring a
> > special encoder.
> 
> 
> hmm -- you touched this topic once in an annotation one month ago:
> 
>    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.
>    >
>    > I'm also wondering if this shouldn't be handled as demuxer exporting
>    > raw video, at least in some of cases if not all.
> 
> And I quickly responded:
> 
>    On 10.09.24 13:58, martin schitter wrote:
>    > 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.
> 
> Although I accepted all your other suggestions for improvement and 
> rewrote lots of code, I didn't change my mid about this particular 
> topic. And I also didn't get any response or further defense of your 
> point of view concerning this issue until now.

It seems rather obvious to me - you're making a demuxer export something
that IS raw video, yet you're tagging it as a new codec ID with a new
decoder that (for these specific pixel format) duplicates what the
rawvideo decoder does.

Just to be clear, I'm not saying the entirety of your decoder should be
handled in the demuxer - just those pixel formats that can be. That
would also have the advantage that the remainder of your decoder could
now enable direct rendering.

> >> +static int float2planes(AVCodecContext *avctx, AVFrame *frame, const AVPacket *pkt)
> >> +{
> >> +    int lw;
> >> +    const size_t sof = 4;
> >> +
> >> +    lw = frame->width;
> >> +
> >> +    for(int y = 0; y < frame->height; y++){
> >> +        for(int x = 0; x < frame->width; x++){
> >> +            memcpy(&frame->data[2][sof*(lw*y + x)], &pkt->data[sof* 3*(lw*y + x)], sof);
> >> +            memcpy(&frame->data[0][sof*(lw*y + x)], &pkt->data[sof*(3*(lw*y + x) + 1)], sof);
> >> +            memcpy(&frame->data[1][sof*(lw*y + x)], &pkt->data[sof*(3*(lw*y + x) + 2)], sof);
> >> +        }
> >> +    }
> > 
> > Same here, deinterleaving packed to planar is a job for swscale.
> 
> Some of these rather inefficient interleave<->planar conversions where 
> just necessary in practice, because swscale wasn't able to figure out a 
> working pipeline construction otherwise, although in theory the affected 
> pixel format closer to the data input should be supported and work as 
> well!:(
> 
> At the end I just looked for something that simply works in real world, too!

But we do have a packed RGBF32 pixel format, how is it different from
this?

> >> +    return pkt->size;
> >> +}
> >> +
> >> +static int half_add_alpha(AVCodecContext *avctx, AVFrame *frame, const AVPacket *pkt)
> >> +{
> >> +    /* ffmpeg doesn't provide any Float16 RGB pixel format without alpha channel
> >> +     * right now. As workaround we simply add an opaque alpha layer. */
> > 
> > So why not add the format then?
> 
> No! -- I definitely don't want to add another ad-hoc provisional 
> solution to swscale! :(

Why would it be ad-hoc or provisional? We already have the exact format,
except that it contains alpha. Adding an analogue without alpha should
be very simple, and as there's a lot of swscale work going on right now
I'm sure someone will be able to help you with it if you need it.

Pixel format conversion in decoders should really be a last resort. We
consciously moved away from it about 15 years ago, with good reason.

Besides the above, your code does very uncomfortable amounts of raw
access to pkt->data. That is inefficient, hard to read, and potentially
unsafe. Seems to me the bytestream2 API would make things a lot better.

Also, all those commented out av_log() calls are visual noise that needs
to go.

-- 
Anton Khirnov


More information about the ffmpeg-devel mailing list