On Mon, Mar 10, 2014 at 12:35:38AM +0100, Vittorio Giovara wrote:
> --- a/configure
> +++ b/configure
> @@ -1672,6 +1672,7 @@ eac3_encoder_select="ac3_encoder"
> eamad_decoder_select="aandcttables dsputil mpegvideo"
> eatgq_decoder_select="aandcttables dsputil"
> eatqi_decoder_select="aandcttables error_resilience mpegvideo"
> +exr_decoder_select="zlib"
This is incorrect. You cannot select an external library as that would
break the build if the lib was not available. Depend on it instead.
> --- /dev/null
> +++ b/libavcodec/exr.c
> @@ -0,0 +1,1342 @@
> +#include <zlib.h>
> +
> +#include "get_bits.h"
> +#include "avcodec.h"
> +#include "bytestream.h"
> +#include "internal.h"
> +#include "mathops.h"
> +#include "thread.h"
> +#include "libavutil/imgutils.h"
> +#include "libavutil/avassert.h"
> +#include "libavutil/opt.h"
Switch this into canonical order.
> +enum ExrCompr {
> + EXR_RAW = 0,
> + EXR_RLE = 1,
> + EXR_ZIP1 = 2,
> + EXR_ZIP16 = 3,
> + EXR_PIZ = 4,
> + EXR_PXR24 = 5,
> + EXR_B44 = 6,
> + EXR_B44A = 7,
> +};
> +
> +enum ExrPixelType {
> + EXR_UINT,
> + EXR_HALF,
> + EXR_FLOAT,
> + EXR_UNKNOWN,
> +};
> +
> +typedef struct EXRChannel {
> + int xsub, ysub;
> + enum ExrPixelType pixel_type;
> +} EXRChannel;
> +
> +typedef struct EXRThreadData {
> + uint8_t *uncompressed_data;
> + int uncompressed_size;
> +
> + uint8_t *tmp;
> + int tmp_size;
> +
> + uint8_t *bitmap;
> + uint16_t *lut;
> +} EXRThreadData;
> +
> +typedef struct EXRContext {
> + AVClass *class;
> + AVFrame *picture;
> + int compr;
> + enum ExrPixelType pixel_type;
> + int channel_offsets[4]; // 0 = red, 1 = green, 2 = blue and 3 = alpha
> + const AVPixFmtDescriptor *desc;
> +
> + uint32_t xmax, xmin;
> + uint32_t ymax, ymin;
> + uint32_t xdelta, ydelta;
> +
> + int ysize;
> +
> + uint64_t scan_line_size;
> + int scan_lines_per_block;
> +
> + const uint8_t *buf, *table;
> + int buf_size;
> +
> + EXRChannel *channels;
> + int nb_channels;
> +
> + EXRThreadData *thread_data;
> + int thread_data_size;
> +
> + const char *layer;
> +} EXRContext;
> +
> +/**
> + * Converts from 32-bit float as uint32_t to uint16_t
"Convert", end in a period.
> +/**
> + * Converts from 16-bit float as uint16_t to uint16_t
same
> +/**
> + * Gets the size of the header variable
same
> + * @param **buf the current pointer location in the header where
> + * the variable data starts
> + * @param *buf_end pointer location of the end of the buffer
Drop the stars, Doxygen probably only complains anyway.
> +static unsigned int get_header_variable_length(const uint8_t **buf,
> + const uint8_t *buf_end)
> +{
> + unsigned int variable_buffer_data_size = bytestream_get_le32(buf);
IMO s/unsigned int/unsigned/ everywhere for brevity.
> +/**
> + * Checks if the variable name corresponds with it's data type
see above
"corresponds to", "its"
> + * @param *avctx the AVCodecContext
> + * @param **buf the current pointer location in the header where
> + * the variable name starts
> + * @param *buf_end pointer location of the end of the buffer
> + * @param *value_name name of the varible to check
> + * @param *value_type type of the varible to check
> + * @param minimum_length minimum length of the variable data
> + * @param variable_buffer_data_size variable length read from the header
> + * after it's checked
see above
Doxygen comments become more readable if you align the parameter
descriptions.
> +static int check_header_variable(AVCodecContext *avctx,
> + const uint8_t **buf,
> + const uint8_t *buf_end,
> + const char *value_name,
> + const char *value_type,
> + unsigned int minimum_length,
> + unsigned int *variable_buffer_data_size)
> +{
> + if (buf_end - *buf >= minimum_length && !strcmp(*buf, value_name)) {
> + *buf += strlen(value_name) + 1;
> + if (!strcmp(*buf, value_type)) {
> + *buf += strlen(value_type) + 1;
> + *variable_buffer_data_size = get_header_variable_length(buf,
> buf_end);
> + if (!*variable_buffer_data_size)
> + av_log(avctx, AV_LOG_ERROR, "Incomplete header.\n");
> + return 1;
> + }
> + *buf -= strlen(value_name) + 1;
> + av_log(avctx, AV_LOG_WARNING,
> + "Unknown data type for header variable %s.\n", value_name);
> + }
> + return -1;
> +}
Returning a negative value on success is kinda backwards.
> +static int zip_uncompress(const uint8_t *src, int compressed_size,
> + int uncompressed_size, EXRThreadData *td)
> +{
> + unsigned long dest_len = uncompressed_size;
> +
> + if (uncompress(td->tmp, &dest_len, src, compressed_size) != Z_OK ||
> + dest_len != uncompressed_size)
> + return AVERROR(EINVAL);
Don't we usually employ INVALIDDATA for broken bitstream?
> +static int decode_block(AVCodecContext *avctx, void *tdata,
> + int jobnr, int threadnr)
> +{
> + EXRContext *s = avctx->priv_data;
> + AVFrame *const p = s->picture;
> + EXRThreadData *td = &s->thread_data[threadnr];
> + const uint8_t *channel_buffer[4] = { 0 };
> + const uint8_t *buf = s->buf;
IMO excessive alignment; only some of the assignments are related.
> +static int decode_frame(AVCodecContext *avctx,
> + void *data,
> + int *got_frame,
> + AVPacket *avpkt)
Coalesce these lines.
> + if (buf_size < 10) {
> + av_log(avctx, AV_LOG_ERROR, "Too short header to parse.\n");
Header too short to parse.
> + buf += 4;
> + xsub = bytestream_get_le32(&buf);
> + ysub = bytestream_get_le32(&buf);
> + if (xsub != 1 || ysub != 1) {
> + avpriv_report_missing_feature(avctx, "Subsampling
> %dx%d", xsub, ysub);
> + return AVERROR_PATCHWELCOME;
> + }
> +
> + if (channel_index >= 0) {
> + if (s->pixel_type != EXR_UNKNOWN &&
> + s->pixel_type != current_pixel_type) {
> + av_log(avctx, AV_LOG_ERROR,
> + "RGB channels not of the same depth.\n");
> + return AVERROR_INVALIDDATA;
> + }
> + s->pixel_type = current_pixel_type;
> + s->channel_offsets[channel_index] =
> current_channel_offset;
align
> + }
> +
> + s->channels = av_realloc(s->channels, ++s->nb_channels *
> sizeof(EXRChannel));
long line
> + // Check if there is enough bytes for a header
s/is/are/
> + // Verify the xmin, xmax, ymin, ymax and xdelta before setting the
> actual image size
> + if (s->xmin > s->xmax ||
> + s->ymin > s->ymax ||
> + s->xdelta != s->xmax - s->xmin + 1 ||
> + s->xmax >= w || s->ymax >= h) {
> + av_log(avctx, AV_LOG_ERROR, "Wrong sizing or missing size
> information.\n");
Wrong size? A "sizing" is not a size.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
