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

[James Almer]

On 10/21/2024 8:40 PM, martin schitter wrote:
> 
> 
> 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. !:(

My point is, it's not a good idea to commit code that's not tested. 
Assuming that's what you meant.

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

Yes, i agree. But what gets committed should be know to work.

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

Ok, looks like that spec defined some crazy packing and reused existing 
naming schemes. Great.

If you're going to unpack these MSB and LSB blocks so the output may be 
something that can be used by an AVPixFmtDescriptor, then might as well 
not add so many new pixel formats that will see no other use and instead 
rely on existing, widely supported ones. As in:

> +        case MKTAG('y','2','1','6'):
> +            ret = fmt_frame(avctx, frame, avpkt, AV_PIX_FMT_UYVY422_16LE, 32, pass_through);
> +            break;

You could unpack this into AV_PIX_FMT_YUV422P16LE.

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

You could rearrange this into AV_PIX_FMT_VYU444, or unpack into 
AV_PIX_FMT_YUV444P.

> +        case MKTAG('y','4','1','6'):
> +            ret = fmt_frame(avctx, frame, avpkt, AV_PIX_FMT_YUV444_16LE, 48, pass_through);
> +            break;

You could unpack this into AV_PIX_FMT_YUV444P16LE, or rearrange it into 
AV_PIX_FMT_AYUV64 (setting the alpha channel to opaque).

The float ones are ok since we have no YUV float formats. But in any 
case, I'd like to hear what others think.

Thanks for your work.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20241021/cacabdaa/attachment.sig>