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

[Jimmy Christensen]

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? As far as I can see "*dst16++ 
= (rgbBuffer&  0x...)>> .." will not work if a little endian system is 
using a PIX_FMT_RGB48BE pixel format. Doesn't seem wise to me. IMHO it's 
a little better to do an extra double check to make sure. Did a small 
test and it showed only about 0.7% increase in performance.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpegDPX-rev13.diff
Type: text/x-patch
Size: 8547 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090602/52b535e3/attachment.bin>