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

[Jimmy Christensen]

On 2009-09-13 20:35, Reimar D?ffinger wrote:
> On Sun, Sep 13, 2009 at 05:58:48PM +0200, Jimmy Christensen wrote:
> Replace
>
>> +static inline uint16_t exr_flt2uint(uint32_t v)
>> +{
>> +    int exp = v>>  23;
>> +    if (v&  0x80000000)
>> +        return 0;
>> +    if (exp<= 127+7-24) // we would shift out all bits anyway
>> +        return 0;
>
> by e.g.
>
>> +static inline uint16_t exr_flt2uint(int32_t v)
>> +{
>> +    int exp = v>>  23;
>> +    // "HACK": negative values result in exp<  0, so clipping them to 0
>> +    // is also handled by this condition, avoids explicit check for sign bit.
>> +    if (exp<= 127+7-24) // we would shift out all bits anyway
>> +        return 0;
>
>

Thanks, will change to that in rev19.

>> + * @return zero if variable is invalid
>
> That is what had me confused in my previous comments: It is 0 both when
> it is invalid (due to type mismatch) and when the name just does not match.
>
>> +    if (buf_end - buf>= minimum_length&&  !strncmp(buf, value_name, strlen(value_name))) {
>> +        buf += strlen(value_name)+1;
>
> Hmm.. Are you sure that strncmp is correct and you shouldn't actually be
> using strcmp? It looks to me like the string in buf must be
> 0-terminated, then strcmp would be correct.
>

Yes, it needs to be 0-terminated. Will change it to strcmp in rev19. 
Same goes for the variable type string. Changed for that aswell.

>> +    unsigned int variable_buffer_data_size = bytestream_get_le32(buf);
>> +    if (buf_end - *buf<= variable_buffer_data_size) {
>> +        av_log(NULL, AV_LOG_ERROR, "Incomplete header\n");
>> +        return 0;
>> +    }
>
> Why is == an error? At least the variable size data still fits in in the
> == case...
>

True. Will remove the == case. Although if this was the case there would 
be no data to have in the actual image in which case == would also be 
wrong :)

>> +    const uint8_t bytes_per_color_table[3] = {
>> +        1, 2, 4
>> +    };
>> +    unsigned int current_bits_per_color_id = 0;
>> +
>> +    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.

You're right that I might not need channel_iter since I added 
current_channel_offset.

Removed channel_iter and redid the code similar to yours.

>> +    if (buf_end - buf<  10) {
>> +            while (channel_list_end>= buf + 19) {
>> +        if (buf_end - buf>  9) {
>> +            if (buf_end - buf>= 5) {
>
> Does this clarify the inconsistency I pointed out before?
> The checks generally are "end - position>  ..."  but that one is
> of the kind "end>  position + ..."
>

Found it now and fixed in rev19.

>> +            const uint64_t line_offset = bytestream_get_le64(&buf) + 8;
>> +            // Check if the buffer has the required bytes needed from the offset
>> +            if ((buf_end - avpkt->data)<  line_offset + xdelta * current_channel_offset) {
>
> Pointless ().

Fixed.

> But somewhere along the line you changed get_le32 to get_le64 and now
> the right hand side can overflow.
> Another problem is that line_offset + xdelta * current_channel_offset is
> not even the right term, given that a malicious file could give r, g and
> b different bit depths.
> In that case the correct term would have to be
> line_offset + xdelta * (FFMAX3(s->channel_offsets[0],
> s->channel_offsets[1], s->channel_offsets[2]) + (s->bits_per_color_id ==
> 2 ? 4 : 2)).

You're right. No file should have different channel types anyway, so 
will check for that instead and error out if there is a mismatch. Also 
added better error output.

> I'd suggest to just verify that all bits_per_color_id are identical at
> the point where you read that value from the file.
> Also it seems to me that buf_end - avpkt->data would be simpler written
> as avpkt->size?

Fixed.

> I'd then extend the verification of xdelta to include
> if (xdelta>  avptk->size / current_channel_offset) error;
> and here do

Good point. Added.

> if (line_offset>  avpkt->size - xdelta * current_channel_offset) error;
>
> Simplifications may be possible though.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenEXR-rev19.diff
Type: text/x-patch
Size: 20685 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090914/55aab8e5/attachment.bin>