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

[Reimar Döffinger]

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.