[FFmpeg-devel] [PATCH] OpenEXR decoder rev-3

[Vitor Sessak]

Jimmy Christensen wrote:
> On 2009-07-01 15:55, Diego Biurrun wrote:
>> On Wed, Jul 01, 2009 at 03:34:19PM +0200, Jimmy Christensen wrote:
>>> On 2009-07-01 15:10, Diego Biurrun wrote:
>>>> On Wed, Jul 01, 2009 at 02:46:54PM +0200, Jimmy Christensen wrote:
>>>>>
>>>>> --- libavcodec/exr.c    (revision 0)
>>>>> +++ libavcodec/exr.c    (revision 0)
>>>>> @@ -0,0 +1,275 @@
>>>>> +        if (strcmp(variable_buffer_name, "dataWindow") == 0) {
>>>>> +            xmin = AV_RL32(buf);
>>>>> +            ymin = AV_RL32(buf+4);
>>>>> +            xmax = AV_RL32(buf+8);
>>>>> +            ymax = AV_RL32(buf+12);
>>>>> +            xdelta = (xmax-xmin)+1;
>>>>
>>>> Spaces around + would make this more readable.
>>>>
>>>>> +        if (strcmp(variable_buffer_name, "displayWindow") == 0) {
>>>>> +
>>>>> +        if (strcmp(variable_buffer_name, "lineOrder") == 0) {
>>>>
>>>> The '== 0' is unnecessary.
>>>
>>> Changed all the strcmp to something like this:
>>>
>>> if (!strcmp(variable_buffer_name, "lineOrder"))
>>>
>>>>
>>>>> +            if(*buf != 0) {
>>>>
>>>> similar
>>>
>>> I suppose you mean the spaces thing. I would however like to like to
>>> keep the "*buf != 0" part since it makes it a little more descriptive 
>>> IMHO.
>>
>>> --- libavcodec/exr.c    (revision 0)
>>> +++ libavcodec/exr.c    (revision 0)
>>> @@ -0,0 +1,276 @@
>>> +
>>> +//#include "libavutil/half.h"
>>
>> Why this commented out #include?
> 
> Whoops. From an old approach. Removed along with some other header files 
> which were unnecessary.
> 
>>
>>> +                if (!strcmp(channel_name, "R")) {
>>> +                    red_channel = channel_iter;
>>> +                    bits_per_color_id = AV_RL32(ptr_tmp);
>>> +                }
>>> +                if (!strcmp(channel_name, "G")) {
>>> +                    green_channel = channel_iter;
>>> +                }
>>> +                if (!strcmp(channel_name, "B")) {
>>> +                    blue_channel = channel_iter;
>>> +                }
>>
>> pointless {}
>>
>>> +        if (!strcmp(variable_buffer_name, "lineOrder")) {
>>> +            if (*buf != 0) {
>>> +                av_log(avctx, AV_LOG_ERROR, "Doesn't support this 
>>> line order : %d\n", *buf);
>>> +            }
>>> +        }
>>
>> pointless {}
> 
> Corrected.
> 
>>
>> The '!= 0' is still pointless.
> 
> Corrected.

Just one suggestion:

> +
> +        // Process the channel list
> +        if (!strcmp(variable_buffer_name, "channels")) {
> +            const uint8_t *ptr_tmp = buf;
> +            for (channel_iter = 0; (buf + variable_buffer_data_size) > (ptr_tmp + 1); channel_iter++) {
> +                strcpy(channel_name, ptr_tmp);
> +                ptr_tmp += strlen(channel_name) + 1;
> +                if (!strcmp(channel_name, "R")) {
> +                    red_channel = channel_iter;
> +                    bits_per_color_id = AV_RL32(ptr_tmp);
> +                }
> +                if (!strcmp(channel_name, "G"))
> +                    green_channel = channel_iter;
> +                if (!strcmp(channel_name, "B"))
> +                    blue_channel = channel_iter;
> +                ptr_tmp += 16;
> +            }
> +        }
> +
> +        if (!strcmp(variable_buffer_name, "dataWindow")) {
> +            xmin = AV_RL32(buf);
> +            ymin = AV_RL32(buf + 4);
> +            xmax = AV_RL32(buf + 8);
> +            ymax = AV_RL32(buf + 12);
> +            xdelta = (xmax-xmin) + 1;
> +        }
> +
> +        if (!strcmp(variable_buffer_name, "displayWindow")) {
> +            w = AV_RL32(buf+8)+1;
> +            h = AV_RL32(buf+12)+1;
> +        }
> +
> +        if (!strcmp(variable_buffer_name, "lineOrder")) {
> +            if (*buf) {
> +                av_log(avctx, AV_LOG_ERROR, "Doesn't support this line order : %d\n", *buf);

av_log_missing_feature()

[...]

> +    switch (bits_per_color_table[bits_per_color_id]) {
> +        case 32:
> +            avctx->pix_fmt = PIX_FMT_RGB48LE;
> +            break;
> +        case 16:
> +            avctx->pix_fmt = PIX_FMT_RGB48LE;
> +            break;
> +        default:
> +            av_log(avctx, AV_LOG_ERROR, "Unkown color format : %d\n", bits_per_color_id);

Same

-Vitor