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

[martin schitter]

On 22.10.24 00:31, James Almer wrote:
> 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.

Yes -- I also don't like to publish untested code.

Unfortunately I do not have access to any software, which could produce 
the needed samples. But I was looking at least for very similar code 
structures -- e.g. RGB / YUV 4:4:4 similarities.

But I definitely will not work on more complex feature suport (like the 
combined components etc.) without real world samples.

All the possible output variants of DaVinci resolve are tested and work!

There still some unpleasant color deviations noticeable in some cases, 
but that's mainly caused elsewhere.

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

I hope, that's the case!


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

Yes -- this super dense bit packing looks really crazy in case of an 
otherwise extremely bloated "uncompressed" and not just "lossless" 
format. But it's still more are less the only option to export all these 
different pixel formats (especially the more advanced floating point 
ones) together with additional audio, TC etc. out of popular closed 
source solutions until now.

and btw.: FOURCC is naturally case-sensitive ;)

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

This was exactly my suggestion in the version v10 of these patch series, 
but others didn't like it.

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

I know -- it's trivial to choose this way, but there has to be some 
common agreement or docu, how it should be done, otherwise we run circles.

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

detto

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

I'm also interested, how others think about this topic.

In case of the Float variants I'm not really happy about this actual 
implementation, because the imported data gets modified in a lossy 
manner by the 16bit integer conversion in this case. This doesn't make 
any visible difference in practice, but it simply shouldn't happen 
nowadays high-end workflows.

I really hope to work around this issue by an additional vulkan 
implementation in the long run.

> Thanks for your work.
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".