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

[Jimmy Christensen]

On 2009-09-14 14:21, Reimar D?ffinger wrote:
> On Mon, Sep 14, 2009 at 01:59:57PM +0200, Jimmy Christensen wrote:
>>>> +    if (!strcmp(*buf, "R"))
>>>> +        s->red_channel = channel_iter;
>>>> +    if (!strcmp(*buf, "G"))
>>>> +        s->green_channel = channel_iter;
>>>> +    if (!strcmp(*buf, "B"))
>>>> +        s->blue_channel = channel_iter;
>>>> +
>>>> +    while (bytestream_get_byte(buf)&&   *buf<   channel_list_end)
>>>> +        continue; /* skip */
>>>> +
>>>> +    current_bits_per_color_id = bytestream_get_le32(buf);
>>>> +    if (current_bits_per_color_id>   2)
>>>> +        return -1;
>>>> +
>>>> +    if (channel_iter == s->red_channel   ||
>>>> +        channel_iter == s->green_channel ||
>>>> +        channel_iter == s->blue_channel) {
>>>> +        if (channel_iter == s->red_channel) {
>>>> +            s->bits_per_color_id  = current_bits_per_color_id;
>>>> +            s->channel_offsets[0] = *current_channel_offset;
>>>> +        }
>>>> +        if (channel_iter == s->green_channel)
>>>> +            s->channel_offsets[1] = *current_channel_offset;
>>>> +        if (channel_iter == s->blue_channel)
>>>> +            s->channel_offsets[2] = *current_channel_offset;
>>>> +    }
>>>> +    *current_channel_offset += bytes_per_color_table[current_bits_per_color_id];
>>>> +    *buf += 12;
>>>> +    return 1;
>>>
>>> You misunderstood my comment on that. All those comparisons against
>>> channel_iter are completely obfuscating things.
>>> I guess you could do something like:
>>>
>>> int channel_index = -1;
>>> if (!buf[1]) { // only handle 1-character channel strings so far
>>>       switch (buf[0]) {
>>>       case 'R': channel_index++;
>>>       case 'G': channel_index++;
>>>       case 'B': channel_index++;
>>>       }
>>> }
>>> [... while an current_bits_per_color_id as above ...]
>>> if (channel_index>= 0) {
>>>       s->bits_per_color_id  = current_bits_per_color_id;
>>>       s->channel_offsets[channel_index] = *current_channel_offset;
>>> }
>>> *current_channel_offset[... rest identical...]
>>>
>>> You do not need channel_iter, nor do you need s->red_channel etc.,
>>> though you might want to know if a channel is missing.
>>> You can either do that with a bit mask, or you could initialize
>>> channel_offsets to something certainly invalid like -1.
>>>
>>>
>>
>> IMHO that's optimizing a bit too much. With the current implementation
>> it would be much easier to have the decoder use channel names other than
>> just R, G and B. If eg. I wanted to expand the decoder to also decoder
>> left eye of a OpenSXR file (OpenEXR stereo file). Their names are
>> left.R, left.G and left.B. Optimizing for only 1 character channel names
>> seems a bit too much, especially since it's only in the header parser
>> and is that much speed important.
>
> Well you still got the basic idea. Except that you are suddenly using
> strncmp again instead of strcmp.

I wasn't sure if it was safe to use only strcmp. Will change it for next 
revision if more changes are needed. (otherwise just posting a new one 
with that change).