On Sat, Nov 03, 2012 at 10:14:05PM +0100, Jordi Ortiz wrote:
>
> --- /dev/null
> +++ b/libavcodec/dirac_dwt.c
> @@ -0,0 +1,614 @@
> +
> +static void vertical_compose53iL0(IDWTELEM *b0, IDWTELEM *b1, IDWTELEM *b2,
> + int width)
indentation
> +static void horizontal_compose_daub97i(IDWTELEM *b, IDWTELEM *temp, int w)
> +{
> + b[w - 1] = (COMPOSE_DAUB97iH0(b2, temp[w - 1], b2) + 1) >> 1;
pointless ()
> +static void spatial_compose_dirac53i_dy(DiracDWTContext *d, int level, int
> width,
> + int height, int stride)
> +{
> + if (y + 1 < (unsigned)height)
> + vertical_compose_l0(b[1], b[2], b[3], width);
> + if (y + 0 < (unsigned)height)
> + vertical_compose_h0(b[0], b[1], b[2], width);
> + if (y - 1 < (unsigned)height)
> + d->horizontal_compose(b[0], d->temp, width);
> + if (y + 0 < (unsigned)height)
> + d->horizontal_compose(b[1], d->temp, width);
> +}
> +
> +static void spatial_compose_dd137i_dy(DiracDWTContext *d, int level, int
> width,
> + int height, int stride)
> +{
> + if (y + 5 < (unsigned)height)
> + vertical_compose_l0(b[3], b[5], b[6], b[7], b[9], width);
> + if (y + 1 < (unsigned)height)
> + vertical_compose_h0(b[0], b[2], b[3], b[4], b[6], width);
> + if (y - 1 < (unsigned)height)
> + d->horizontal_compose(b[0], d->temp, width);
> + if (y + 0 < (unsigned)height)
> + d->horizontal_compose(b[1], d->temp, width);
> +}
> +
> +static void spatial_compose_daub97i_dy(DiracDWTContext *d, int level, int
> width,
> + int height, int stride)
> +{
> + if (y + 3 < (unsigned)height)
> + vertical_compose_l1(b[3], b[4], b[5], width);
> + if (y + 2 < (unsigned)height)
> + vertical_compose_h1(b[2], b[3], b[4], width);
> + if (y + 1 < (unsigned)height)
> + vertical_compose_l0(b[1], b[2], b[3], width);
> + if (y + 0 < (unsigned)height)
> + vertical_compose_h0(b[0], b[1], b[2], width);
> +
> + if (y - 1 < (unsigned)height)
> + d->horizontal_compose(b[0], d->temp, width);
> + if (y + 0 < (unsigned)height)
> + d->horizontal_compose(b[1], d->temp, width);
> +}
I wonder if you can't just make "height" unsigned in the function pointer
prototype and then drop all those casts.
> +static void spatial_compose97i_init2(DiracDWTCompose *cs, IDWTELEM *buffer,
> + int height, int stride)
> +
> +static void spatial_compose53i_init2(DiracDWTCompose *cs, IDWTELEM *buffer,
> + int height, int stride)
> +
> +int ff_spatial_idwt_init2(DiracDWTContext *d, IDWTELEM *buffer, int width,
> + int height, int stride, enum dwt_type type,
> + int decomposition_count, IDWTELEM *temp)
Why are all these functions called "_init2()"? Where's the corresponding
"_init()"?
> + switch (type) {
> + case DWT_DIRAC_HAAR0:
> + case DWT_DIRAC_HAAR1:
> + d->spatial_compose = spatial_compose_haari_dy;
> + d->vertical_compose = vertical_compose_haar;
> + if (type == DWT_DIRAC_HAAR0)
> + d->horizontal_compose = horizontal_compose_haar0i;
> + else
> + d->horizontal_compose = horizontal_compose_haar1i;
> + d->support = 1;
> + break;
Maybe just duplicate the lines instead of the if-block, dunno.
> +void ff_spatial_idwt_slice2(DiracDWTContext *d, int y)
> +
> +int ff_spatial_idwt2(IDWTELEM *buffer, int width, int height, int stride,
> + enum dwt_type type, int decomposition_count,
> + IDWTELEM *temp)
Here I also wonder about the "2" suffix in the function name.
> --- /dev/null
> +++ b/libavcodec/dirac_dwt.h
> @@ -0,0 +1,132 @@
> +
> +#ifndef AVCODEC_DIRAC_DWT_H
> +#define AVCODEC_DIRAC_DWT_H
> +
> +#include <stdint.h>
> +
> +typedef int DWTELEM;
> +typedef short IDWTELEM;
nit: align
> +typedef struct {
> + IDWTELEM *b[MAX_DWT_SUPPORT];
> + int y;
> +} DiracDWTCompose;
no anymous structs please
> +typedef struct DiracDWTContext {
> + void (*vertical_compose_h1)(void);
> + void (*vertical_compose)(void);
> + ///< vertical_compose -> one set of lowpass and highpass combined
void (*vertical_compose)(void); ///< vertical_compose -> one set of
lowpass and highpass combined
> +// Possible prototypes for vertical_compose functions
> +typedef void (*vertical_compose_2tap)(IDWTELEM *b0, IDWTELEM *b1, int width);
pointless comment
> +// -1 if an error occurred, e.g. the dwt_type isn't recognized
> +int ff_spatial_idwt_init3(DiracDWTContext *d, IDWTELEM *buffer, int width,
> + int height, int stride, enum dwt_type type,
> + int decomposition_count, IDWTELEM *temp);
-1 what? The comment is rather mysterious...
> +// shared stuff for simd optimiztions
SIMD optimiz_a_tions
> --- /dev/null
> +++ b/libavcodec/diracdsp.c
> @@ -0,0 +1,301 @@
> +
> +#include "dsputil.h"
> +#include "diracdsp.h"
> +#include "libavutil/common.h"
nit: Move the libavutil #include before the others.
> + dc->dsp.OPNAME ## _pixels_l2[0](dst + 16, src[0] + 16, src[1] + 16,\
> + stride, stride, stride, h); \
> + dc->dsp.OPNAME ## _pixels_l4[1](dst, src[0], src[1], src[2], src[3],\
> + stride, stride, stride, stride, \
> + stride, h); \
nit: Break the lines differently to avoid the missing space after comma
> +static void biweight_dirac_pixels ## W ## _c(uint8_t *dst, \
> + const uint8_t *src, \
> + int stride, int log2_denom,\
> + int weightd, int weights, \
> + int h) \
same
> --- /dev/null
> +++ b/libavcodec/diracdsp.h
> @@ -0,0 +1,95 @@
> +
> +#include <stdint.h>
> +#include "dsputil.h"
nit: empty line between local and system headers.
> +typedef struct DiracDSPContext{
space before {
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
