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

Tue Oct 22 02:51:28 EEST 2024

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

I didn't participate in earlier rounds, but at least my opinion is that 
we shouldn't add new pixel formats if they are only used for a single 
codec and have no RIFF or ISOBMFF mapping.

fwiw, YUV444 is fine as a name if we end up adding it, but i dislike 
UYVY422_16 and YUV444_16.
Unfortunately we can't use Y216 (Would need to match existing Y210 and 
Y212, which are ordered YUYV), or Y416 (ISOBMFF mapped it to AYUV64).

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

-------------- 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/61562abf/attachment.sig>