[FFmpeg-devel] [PATCH] Add DPX decoder rev-13

[Vitor Sessak]

Jimmy Christensen wrote:
> On 2009-05-30 12:53, Michael Niedermayer wrote:
>> On Fri, May 29, 2009 at 08:43:00AM +0200, Jimmy Christensen wrote:
>>> On 2009-05-28 12:52, Michael Niedermayer wrote:
>>>> On Thu, May 28, 2009 at 07:19:48AM +0200, Jimmy Christensen wrote:
>> [...]
>>>> [...]
>>>>> +    for (x = 0; x<   s->height; x++) {
>>>>> +        uint16_t *dst = ptr;
>>>>> +        for (y = 0; y<   s->width; y++) {
>>>>> +            rgbBuffer = AV_RB32(buf);
>>>>
>>>>> +            red16   = RED10(rgbBuffer) * 64; // 10-bit>   16-bit
>>>>> +            green16 = GREEN10(rgbBuffer) * 64; // 10-bit>   16-bit
>>>>> +            blue16  = BLUE10(rgbBuffer) * 64; // 10-bit>   16-bit
>>>>
>>>> there are 3 avoidable operations in there at least
>>>>
>>>
>>> Changed
>>
>> no they are still there, the maximum is a bit wise and, and a shift 
>> for simple
>> convertion, you use a multiplication in addition. Also thats not to 
>> say this cant
>> be done with even less.
>> That said more correct would be a
>> (x<<6)+(x>>4)
>> [the difference being that white (1023) is kept white (65535)]
>>
>>
>> [...]
> 
> Misunderstood the first time, but you are are indeed correct. Would 
> still keep it out of the RED/GREEN/BLUE10 macros so it's easier to 
> change the output format to eg. RGB96 (32-bit per color) if/when that 
> gets added.
> 
>>
>>
>>> +    if (magic_num == MKTAG('S', 'D', 'P', 'X'))
>>> +        endian = 0;
>>> +    else if (magic_num == MKTAG('X', 'P', 'D', 'S'))
>>
>> these things can be written like AV_RB32("SDPX")
>> IMHO thats slightly more readable
>>
> 
> True, changed that.
> 
>>
>> [...]
>>> +    buf += offset;
>>> +
>>> +    ptr    = p->data[0];
>>> +    stride = p->linesize[0];
>>> +
>>> +    for (x = 0; x<  s->height; x++) {
>>> +        uint8_t *dst = ptr;
>>> +        for (y = 0; y<  s->width; y++) {
>>> +            rgbBuffer = AV_RB32(buf);
>>
>>> +            if(PIX_FMT_RGB48 == PIX_FMT_RGB48BE) {
>>> +                bytestream_put_be16(&dst, RED10(rgbBuffer) * 64); // 
>>> 10-bit>  16-bit
>>> +                bytestream_put_be16(&dst, GREEN10(rgbBuffer) * 64); 
>>> // 10-bit>  16-bit
>>> +                bytestream_put_be16(&dst, BLUE10(rgbBuffer) * 64); 
>>> // 10-bit>  16-bit
>>> +            } else {
>>> +                bytestream_put_le16(&dst, RED10(rgbBuffer) * 64); // 
>>> 10-bit>  16-bit
>>> +                bytestream_put_le16(&dst, GREEN10(rgbBuffer) * 64); 
>>> // 10-bit>  16-bit
>>> +                bytestream_put_le16(&dst, BLUE10(rgbBuffer) * 64); 
>>> // 10-bit>  16-bit
>>> +            }
>>
>> *dst16++ = (rgbBuffer&  0x...)>>  ..;
>> *dst16++ = (rgbBuffer&  0x...)>>  ..;
>> *dst16++ = (rgbBuffer&  0x...)>>  ..;
>>
> 
> Does makes code simpler, but can it always be guarantied that 
> PIX_FMT_RGB48 is PIX_FMT_RGB48BE on big endian systems and 
> PIX_FMT_RGB48LE on little endian systems?

Quoting libavutils/pix_fmt.h

> #ifdef WORDS_BIGENDIAN
> #   define PIX_FMT_NE(be, le) PIX_FMT_##be
> #else
> #   define PIX_FMT_NE(be, le) PIX_FMT_##le
> #endif
> 
> #define PIX_FMT_RGB32   PIX_FMT_NE(ARGB, BGRA)
> #define PIX_FMT_RGB32_1 PIX_FMT_NE(RGBA, ABGR)
> #define PIX_FMT_BGR32   PIX_FMT_NE(ABGR, RGBA)
> #define PIX_FMT_BGR32_1 PIX_FMT_NE(BGRA, ARGB)
> 
> #define PIX_FMT_GRAY16 PIX_FMT_NE(GRAY16BE, GRAY16LE)
> #define PIX_FMT_RGB48  PIX_FMT_NE(RGB48BE,  RGB48LE)

The whole point of having three pix formats is having one for big 
endian, one for little endian and one for native endianness.

-Vitor