[FFmpeg-devel] [PATCH v12 5/9] libavcodec/dnxucdec: DNxUncompressed decoder

[martin schitter]

On 21.10.24 22:44, James Almer wrote:
> 
> That's not good...
> 
> [...]

Whenever and however I change it, there will allways be somebody who 
doesn't like it. !:(

This time I spend a lot of time to modify the code as close as possible 
to requests asked by one previous reviewer, who insisted, that for any 
format that isn't already supported by a simple raw data pass though, I 
have to add a fitting pixel format and the necessary input routines.

In this version I did exactly follow this advice to finally get accepted.

Only for those six formats, which need anyway some further processing, 
because of a very uncommon bitpacking stucture used by DNxUncompressed 
for 10 and 12bit formats, I still used some already available similar 
planar formats.

I personally still think it would wise to support just a minimal but 
carefully and systematically chosen set of internal pixel formats for 
lossless and efficient processing and avoid all other very similar 
combinatorial possibilities, but those other reviewer strictly denied 
this view.


>> +            break;
>> +        case MKTAG('y','4','0','8'):
>> +            ret = fmt_frame(avctx, frame, avpkt, AV_PIX_FMT_YUV444, 
>> 24, pass_through);
> 
> Y408 is mapped to AV_PIX_FMT_AYUV.

yes -- but "AYUV" is something different than "YUV"



>> +            break;
>> +        case MKTAG('y','2','1','0'):
>> +            ret = fmt_frame(avctx, frame, avpkt, 
>> AV_PIX_FMT_YUV422P10LE, 20, unpack_y210);
> 
> Y210 has no pixel format, and it's packed, not planar, so definitely not 
> AV_PIX_FMT_YUV422P10LE.

I just put here the final pixel format, which it will have after 
preprocessing, as an argument for this kind of input. The actual 
transformation resp. bit reassembling is happening in `unpack_y210()`.

>> +            break;
>> +        case MKTAG('y','4','1','0'):
>> +            ret = fmt_frame(avctx, frame, avpkt, 
>> AV_PIX_FMT_YUV444P10LE, 20, unpack_y410);
> 
> Y410 is mapped to AV_PIX_FMT_XV30LE.

no -- in case of the 10 and 12 bit variants I utilize 16bit aligned 
planar storage, because ignoring byte and 32bit boundaries for more 
dense storage and optimized pixel data locality isn't always useful.

>> +            break;
>> +        case MKTAG('y','2','1','2'):
>> +            ret = fmt_frame(avctx, frame, avpkt, 
>> AV_PIX_FMT_YUV422P12LE, 24, unpack_y212);
> 
> AV_PIX_FMT_Y212?

detto...

>> +            break;
>> +        case MKTAG('y','4','1','2'):
>> +            ret = fmt_frame(avctx, frame, avpkt, 
>> AV_PIX_FMT_YUV444P12LE, 24, unpack_y412);
> 
> This one is probably AV_PIX_FMT_XV36, and definitely not planar.

detto...

>> +            break;
>> +        case MKTAG('y','2','1','6'):
>> +            ret = fmt_frame(avctx, frame, avpkt, 
>> AV_PIX_FMT_UYVY422_16LE, 32, pass_through);
> 
> The order of y216 appears to be YUYV, not UYVY. https:// 
> learn.microsoft.com/en-us/windows/win32/medfound/10-bit-and-16-bit-yuv- 
> video-formats

Please also read the SMPTE RDD 50 specification as well!
(https://pub.smpte.org/doc/rdd50/20190730-pub/rdd50-2019.pdf)

But in fact I would even more suggest looking at the well structured 
system of vulkan pixel formats!

I simply had to implement all this additional pixel formats, because the 
byte order of already defined varaints were indeed different. And you 
can't simply ignore this fact, if you want to handle the raw data stream 
without any other kind of local modification.

Martin