On Tue, Nov 01, 2016 at 09:37:50AM +0100, Alexandra Hájková wrote:
> From: Alexandra Hajkova <alexandra.khirn...@gmail.com>
Your name lost some accents here..
> ---
> libavcodec/hevcdsp.c | 2 +
> libavcodec/hevcdsp.h | 1 +
> libavcodec/ppc/Makefile | 1 +
> libavcodec/ppc/hevcdsp_ppc.c | 111
> ++++++++++++++++++++++++++++++++++++++
> libavcodec/ppc/hevcdsp_template.c | 52 ++++++++++++++++++
> 5 files changed, 167 insertions(+)
> create mode 100644 libavcodec/ppc/hevcdsp_ppc.c
> create mode 100644 libavcodec/ppc/hevcdsp_template.c
>
> --- a/libavcodec/hevcdsp.c
> +++ b/libavcodec/hevcdsp.c
> @@ -247,4 +247,6 @@ void ff_hevc_dsp_init(HEVCDSPContext *hevcdsp, int
> bit_depth)
>
> if (ARCH_X86)
> ff_hevc_dsp_init_x86(hevcdsp, bit_depth);
> + if (ARCH_PPC)
> + ff_hevc_dsp_init_altivec(hevcdsp, bit_depth);
order
Look at how other such init functions are named.
> --- a/libavcodec/hevcdsp.h
> +++ b/libavcodec/hevcdsp.h
> @@ -116,6 +116,7 @@ typedef struct HEVCDSPContext {
> void ff_hevc_dsp_init(HEVCDSPContext *hpc, int bit_depth);
>
> void ff_hevc_dsp_init_x86(HEVCDSPContext *c, const int bit_depth);
> +void ff_hevc_dsp_init_altivec(HEVCDSPContext *c, const int bit_depth);
order
> --- /dev/null
> +++ b/libavcodec/ppc/hevcdsp_ppc.c
> @@ -0,0 +1,111 @@
> +
> +/*
> + * Copyright (c) Alexandra Hajkova
stray empty line
> +static const vector int16_t trans4[4] = {
> + { 64, 64, 64, 64, 64, 64, 64, 64 },
> + { 83, 36, 83, 36, 83, 36, 83, 36 },
> + { 64, -64, 64, -64, 64, -64, 64, -64 },
> + { 36, -83, 36, -83, 36, -83, 36, -83 },
> +};
This fits in int8_t, is there a reason to have it int16_t?
> +static const vec_u8 mask[2] = {
> + { 0x00, 0x01, 0x08, 0x09, 0x10, 0x11, 0x18, 0x19, 0x02, 0x03, 0x0A,
> 0x0B, 0x12, 0x13, 0x1A, 0x1B },
> + { 0x04, 0x05, 0x0C, 0x0D, 0x14, 0x15, 0x1C, 0x1D, 0x06, 0x07, 0x0E,
> 0x0F, 0x16, 0x17, 0x1E, 0x1F },
> +};
Where do these tables come from? I would expect them to be shared
across arches.
> +#if HAVE_ALTIVEC
> +static void transform4x4(vector int16_t src_01, vector int16_t src_23,
> + vector int32_t res[4], const int shift, int16_t
> *coeffs)
long line
> + // if is not used by the other transform
This sentence lacks a proper subject and is therefore confusing.
> +av_cold void ff_hevc_dsp_init_altivec(HEVCDSPContext *c, const int bit_depth)
> +{
> +#undef FUNC
> +#define FUNC(a, bit_depth) a ## _ ## bit_depth
This macro seems pretty pointless. It adds one level of indirection and
makes the expression longer.
> +#if HAVE_ALTIVEC
What about endianness?
> --- /dev/null
> +++ b/libavcodec/ppc/hevcdsp_template.c
> @@ -0,0 +1,52 @@
> +
> +#include "libavcodec/bit_depth_template.c"
> +
> +#if HAVE_ALTIVEC
Moving this ifdef out of the template seems simpler and is how the other
template files are handled.
> +static void FUNC(ff_hevc_idct_4x4)(int16_t *coeffs, int col_limit)
> +{
> + const int shift = 7;
> + const int shift2 = 20 - BIT_DEPTH;
> + vector int16_t src_01, src_23;
> + vector int32_t res[4];
> + vector int16_t res_packed[2];
> +
> + src_01 = vec_ld(0, (short *) coeffs);
> + src_23 = vec_ld(16, (short *) coeffs);
Is the cast required?
Diego
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
