[FFmpeg-devel] [PATCH v10 4/6] libavcodec/dnxucdec: DNxUncompressed decoder

[martin schitter]

Thanks for looking again in such an accurate manner over this code!

On 09.10.24 18:13, Michael Niedermayer wrote:
> On Fri, Oct 04, 2024 at 11:07:33PM +0200, Martin Schitter wrote:
> [...]
> 
>> +static int half_add_alpha(AVCodecContext *avctx, AVFrame *frame, const AVPacket *pkt)
>> +{
>> +    /* ffmpeg doesn't provide RGB half bit depth without alpha channel right now
>> +     * we simply add an opaque alpha layer as workaround */
>> +
>> +    int lw;
>> +    const size_t soh = 2;
>> +    const uint16_t opaque = 0x3c00;
>> +
>> +    lw = frame->width;
>> +
>> +    for(int y = 0; y < frame->height; y++){
>> +        for(int x = 0; x < frame->width; x++){
>> +            memcpy(&frame->data[0][soh*4*(lw*y + x)], &pkt->data[soh*3*(lw*y + x)], soh*3);
>> +            memcpy(&frame->data[0][soh*(4*(lw*y + x) + 3)], &opaque, soh);
> 
> opaque is in native byte order within 16bit, pkt->data is a byte array
> copying them together cannot be right
> also as this is a LE pixfmt, the 2nd part should be wrong

`opaque` is just a binary representation of the float16/half value 1.0, 
but it should be indeed byte swapped here if used on big endian machines 
-- good point!

fixed.

btw:
Does this workaround of adding an alpha channel look acceptable to you?

I'm not very happy about this solution, but adding a more suitable pixel 
format and all associated sws routines would take much more efforts.


>> +            lsp = (pack >> (3*x%4)*2) & 0x3;
> 
> I would suggest & instead of % because if the compiler doesnt optimize the modulo
> then it is the slower operation

I don't think it makes any difference performance wise because the 
optimizer is always smarter than us, and I personally somehow preferred 
the original expression because of better readability, but I replaced 
now all similar occurrences in v11.

Martin