[FFmpeg-devel] [PATCH] OpenEXR decoder rev-12
Jimmy Christensen
jimmy
Wed Jul 29 14:39:08 CEST 2009
On 2009-07-29 13:54, Reimar D?ffinger wrote:
> On Wed, Jul 29, 2009 at 07:50:47AM +0200, Jimmy Christensen wrote:
>> On 2009-07-29 00:46, Michael Niedermayer wrote:
>>> On Tue, Jul 21, 2009 at 08:45:10PM +0200, Jimmy Christensen wrote:
>>>> New patch. Re-arranged some small things. Now supports variable channel
>>>> size for all other than the R, G and B channels which are expected to be of
>>>> the same size.
>>>>
>>>> Also added a few more checks for possible buffer overruns.
>>> [...
>>>> +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;
>>>
>>> if v where signed then the first if would be unneded
>>>
>>
>> Not sure it would yield the expected result. If it's negative it needs
>> to output 0 and not a positive number.
>
> If v is signed, exp will be< for negative numbers. So yes it would work.
>
Will change it then.
>>>> + channel_iter++;
>>>> + if (buf - ptr_tmp>= 19&& !strncmp(ptr_tmp, "R", 2)) {
>>>> + ptr_tmp += 2;
>>>> + red_channel = channel_iter;
>>>> + current_bits_per_color_id = bytestream_get_le32(&ptr_tmp);
>>>> + if (current_bits_per_color_id> 2) {
>>>> + av_log(avctx, AV_LOG_ERROR, "Unkown color format : %d\n", bits_per_color_id);
>>>> + return -1;
>>>> + }
>>>> + red_channel_offset = current_channel_offset;
>>>> + current_channel_offset += bytes_per_color_table[current_bits_per_color_id];
>>>> + bits_per_color_id = current_bits_per_color_id;
>>>> + ptr_tmp += 12;
>>>> + continue;
>>>> + }
>>>> + if (buf - ptr_tmp>= 19&& !strncmp(ptr_tmp, "G", 2)) {
>>>> + ptr_tmp += 2;
>>>> + green_channel = channel_iter;
>>>> + current_bits_per_color_id = bytestream_get_le32(&ptr_tmp);
>>>> + if (current_bits_per_color_id> 2) {
>>>> + av_log(avctx, AV_LOG_ERROR, "Unkown color format : %d\n", bits_per_color_id);
>>>> + return -1;
>>>> + }
>>>> + green_channel_offset = current_channel_offset;
>>>> + current_channel_offset += bytes_per_color_table[current_bits_per_color_id];
>>>> + ptr_tmp += 12;
>>>> + continue;
>>>> + }
>>>
>>> code duplication
>>>
>>
>> Could probably make it less duplicate code, but these steps needs to be
>> done only for R, G and B and needs to assign specific variables
>> accordingly.
>
> That's what functions are there for. I think in quite a few places readability
> could be improved quite a bit by creating a well named function, but I think
> you so far ignored all my suggestions in that regard.
>
It's not as much as I ignored them. I tried making them into functions
but since it needs to exit with a return -1, I didn't know how to make
the function make the main function return -1.
>> + if (buf_end - buf< 10) {
>> + av_log(avctx, AV_LOG_ERROR, "Too short header to parse\n");
>> + return -1;
>> + } else {
>
> Pointless else, that was basically the whole point why I suggested changing
> the order of this.
> If you are more used to languages like Java etc., think of this as a poor-man's
> exception.
>
Missed this one. Will fix.
>> + if (buf_end - buf<= variable_buffer_data_size) {
>> + av_log(avctx, AV_LOG_ERROR, "Incomplete channel list\n");
>> + return -1;
>> + } else {
>
> Same.
>
>> + for (int i = 0; i< 2; i++) {
>
> That will break gcc 2.95 compilation. Declare i normally.
>
Hmmm.. didn't mean for this to get into the patch, was just a place holder.
>> + if ((buf_end - buf)> variable_buffer_data_size) {
>> + buf += variable_buffer_data_size;
>> + } else {
>> + av_log(avctx, AV_LOG_ERROR, "Incomplete header\n");
>> + return -1;
>> + }
>
> Pointless () around buf_end - buf, also I still think this check is duplicated
> all over the place and could be factored out at worst by using a macro.
>
AFAIK Michael Niedermayer hates macros :)
>> + if (buf< buf_end) {
>> + // Move pointer out of header
>> + buf++;
>> + } else {
>> + av_log(avctx, AV_LOG_ERROR, "Incomplete file\n");
>> + return -1;
>> + }
>
> As in the other code, this is more compact and IMO readable by packing the
> execptional case into the lower indentation level, i.e.
>
Missed than one too.
>> + if (buf>= buf_end) {
>> + av_log(avctx, AV_LOG_ERROR, "Incomplete file\n");
>> + return -1;
>> + }
>> + buf++;
>
> But since the input buffer is guaranteed to be padded by a few extra bytes, I think
> this check is not really necessary at all.
>
>> + // 32-bit
>> + case 2:
>> + // Process the actual lines
>> + for (y = ymin; y<= ymax; y++) {
>> + uint16_t *ptr_x = (uint16_t*)ptr;
>> + if (buf_end - buf> 8) {
>> + const uint32_t line_offset = bytestream_get_le64(&buf) + 8;
>> + // Check if the buffer has the required bytes needed from the offset
>> + if ((uint32_t)(buf_end - avpkt->data)>= line_offset + (xdelta) * current_channel_offset) {
>> + const uint8_t *red_channel_buffer = avpkt->data + line_offset + (xdelta) * red_channel_offset;
>> + const uint8_t *green_channel_buffer = avpkt->data + line_offset + (xdelta) * green_channel_offset;
>> + const uint8_t *blue_channel_buffer = avpkt->data + line_offset + (xdelta) * blue_channel_offset;
>> +
>> + // Zero out the start if xmin is not 0
>> + memset(ptr_x, 0, xmin*6);
>> + ptr_x += xmin*3;
>> +
>> + for (x = 0; x< xdelta; x++) {
>> + *ptr_x++ = exr_flt2uint(bytestream_get_le32(&red_channel_buffer));
>> + *ptr_x++ = exr_flt2uint(bytestream_get_le32(&green_channel_buffer));
>> + *ptr_x++ = exr_flt2uint(bytestream_get_le32(&blue_channel_buffer));
>> + }
>> +
>> + // Zero out the end if xmax+1 is not w
>> + memset(ptr_x, 0, (avctx->width - (xmax+1))*6);
>> + ptr_x += (avctx->width - (xmax+1))*3;
>> +
>> + // Move to next line
>> + ptr += stride;
>> + }
>> + }
>> + }
>> + break;
>> + // 16-bit
>> + case 1:
>> + // Process the actual lines
>> + for (y = ymin; y<= ymax; y++) {
>> + uint16_t *ptr_x = (uint16_t*)ptr;
>> + if (buf_end - buf> 8) {
>> + const uint32_t line_offset = bytestream_get_le64(&buf) + 8;
>> + // Check if the buffer has the required bytes needed from the offset
>> + if ((uint32_t)(buf_end - avpkt->data)>= line_offset + (xdelta) * current_channel_offset) {
>> + const uint8_t *red_channel_buffer = avpkt->data + line_offset + (xdelta) * red_channel_offset;
>> + const uint8_t *green_channel_buffer = avpkt->data + line_offset + (xdelta) * green_channel_offset;
>> + const uint8_t *blue_channel_buffer = avpkt->data + line_offset + (xdelta) * blue_channel_offset;
>> +
>> + // Zero out the start if xmin is not 0
>> + memset(ptr_x, 0, xmin*6);
>> + ptr_x += xmin*3;
>> +
>> + for (x = 0; x< xdelta; x++) {
>> + *ptr_x++ = exr_halflt2uint(bytestream_get_le16(&red_channel_buffer));
>> + *ptr_x++ = exr_halflt2uint(bytestream_get_le16(&green_channel_buffer));
>> + *ptr_x++ = exr_halflt2uint(bytestream_get_le16(&blue_channel_buffer));
>> + }
>> +
>> + // Zero out the end if xmax+1 is not w
>> + memset(ptr_x, 0, (avctx->width - (xmax+1))*6);
>> + ptr_x += (avctx->width - (xmax+1))*3;
>> +
>> + // Move to next line
>> + ptr += stride;
>> + }
>> + }
>> + }
>> + break;
>
> These two are almost the same except for a factor 2 in some formulas and the innermost loop.
> I think the performance impact of merging these into one piece of code should be small enough
> that it would be reasonable to avoid the duplication...
>
Will try and do a test with both approaches.
More information about the ffmpeg-devel
mailing list