[FFmpeg-devel] [PATCH] OpenEXR decoder rev-12
Jimmy Christensen
jimmy
Wed Jul 29 07:50:47 CEST 2009
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 (exp>= 127)
>> + return 0xffff;
>> + v&= 0x007fffff;
>> + return (v+(1<<23))>> (127+7-exp);
>> +}
> [...]
>> +static int decode_frame(AVCodecContext *avctx,
>> + void *data,
>> + int *data_size,
>> + AVPacket *avpkt)
>> +{
>> + const uint8_t *buf = avpkt->data;
>> + int buf_size = avpkt->size;
>> + const uint8_t *buf_end = buf + buf_size;
>> +
>> + EXRContext *const s = avctx->priv_data;
>> + AVFrame *picture = data;
>> + AVFrame *const p =&s->picture;
>> + uint8_t *ptr;
>> +
>> + int x, y, stride, magic_number;
>> + int w = 0;
>> + int h = 0;
>> + int xmin = -1;
>> + int xmax = -1;
>> + int ymin = -1;
>> + int ymax = -1;
>> + int xdelta = -1;
>> +
>> + int red_channel = -1;
>> + int green_channel = -1;
>> + int blue_channel = -1;
>> + int bits_per_color_id = -1;
>
>> + int red_channel_offset = 0;
>> + int green_channel_offset = 0;
>> + int blue_channel_offset = 0;
>> + int current_channel_offset = 0;
>
> nitpick:
> int red_channel_offset = 0;
> int green_channel_offset = 0;
> int blue_channel_offset = 0;
>
>
Will fix that.
>> +
>> + static int bytes_per_color_table[3] = {1, 2, 4};
>
> missing const
>
>
Will fix that.
>> +
>> + magic_number = bytestream_get_le32(&buf);
>> + if (magic_number != 20000630) { // As per documentation of OpenEXR it's supposed to be int 20000630 little-endian
>> + av_log(avctx, AV_LOG_ERROR, "Wrong magic number %d\n", magic_number);
>> + return -1;
>> + }
>> +
>> + // Move to header start
>> + buf += 4;
>> +
>
>> + if (buf_end - buf< 10) {
>> + av_log(avctx, AV_LOG_ERROR, "Too short header to parse\n");
>> + return -1;
>> + } else {
>> + // Parse the header
>> + while (buf[0] != 0x0) {
>
> there is somethig wrong with the indention
>
>
Hmmmm... didn't see that. Wonder how it got there.
>> + int variable_buffer_data_size;
>> + // Process the channel list
>> + if (buf_end - buf>= 20&& !strncmp(buf, "channels", 9)) {
>> + buf += 9;
>> + if (strncmp(buf, "chlist", 7)) {
>> + av_log(avctx, AV_LOG_ERROR, "Unknown type for channels\n");
>> + return -1;
>> + }
>> + buf += 7;
>> + variable_buffer_data_size = bytestream_get_le32(&buf);
>> + if (buf_end - buf<= variable_buffer_data_size) {
>> + av_log(avctx, AV_LOG_ERROR, "Incomplete channel list\n");
>> + return -1;
>
> have you considered negative variable_buffer_data_size ?
>
>
You're right it should be unsigned.
>> + } else {
>> + const uint8_t *ptr_tmp = buf;
>> + int channel_iter = -1;
>> + int current_bits_per_color_id = 0;
>> + buf += variable_buffer_data_size;
>> + while (ptr_tmp + 1< buf) {
>
> i think readability woud be better if there was an empty line in there
> somewhere
>
>
Added that now.
>> + 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.
> [...]
>> + // Skip variable name
>> + while (buf++< buf_end) {
>> + if (buf[0] == 0x0)
>> + break;
>> + }
>> + // Skip variable type
>> + while (buf++< buf_end) {
>> + if (buf[0] == 0x0)
>> + break;
>> + }
>
> code duplication
>
Can add a for loop in front, but it needs to be done exactly twice. And
without a loop, it's easier to comment which skips what.
> [...]
>> + if (red_channel != -1&& blue_channel != -1&& green_channel != -1) {
>> + if (s->picture.data[0])
>> + avctx->release_buffer(avctx,&s->picture);
>> + if (avcodec_check_dimensions(avctx, w, h))
>> + return -1;
>> + if (w != avctx->width || h != avctx->height) {
>> + // Verify the xmin, xmax, ymin, ymax and xdelta before setting the actual image size
>
>> + if (xmin> w || xmin< 0 || xmax> w || xmax< 0 || ymin> h || ymin< 0 || ymax> h || ymax< 0 || xdelta< 0) {
>> + av_log(avctx, AV_LOG_ERROR, "Wrong sizing or missing size information\n");
>> + return -1;
>> + }
>
> it looks like these variables should be unsigned
>
You're right. Changed it back.
Attached new patch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenEXR-rev12.diff
Type: text/x-patch
Size: 22253 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090729/cbcdb991/attachment.bin>
More information about the ffmpeg-devel
mailing list