Re: [libav-devel] [PATCH] hevc: Add HEVC IDCT 4x4 for PPC

[Diego Biurrun]

On Sun, Dec 11, 2016 at 12:10:19PM +0100, Alexandra Hájková wrote:
> From: Alexandra Hajkova 
> 
> ---
>  libavcodec/hevcdsp.c  |   2 +
>  libavcodec/hevcdsp.h  |   1 +
>  libavcodec/ppc/Makefile   |   1 +
>  libavcodec/ppc/hevcdsp.c  | 108 
> ++
>  libavcodec/ppc/hevcdsp_template.c |  48 +
>  5 files changed, 160 insertions(+)
>  create mode 100644 libavcodec/ppc/hevcdsp.c
>  create mode 100644 libavcodec/ppc/hevcdsp_template.c

LGTM

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hevc: Add HEVC IDCT 4x4 for PPC

[Diego Biurrun]

On Thu, Dec 01, 2016 at 09:59:20PM +0100, Alexandra Hájková wrote:
> From: Alexandra Hajkova 
> 
> ---
>  libavcodec/hevcdsp.c  |   2 +
>  libavcodec/hevcdsp.h  |   1 +
>  libavcodec/ppc/Makefile   |   1 +
>  libavcodec/ppc/hevcdsp.c  | 110 
> ++
>  libavcodec/ppc/hevcdsp_template.c |  48 +
>  5 files changed, 162 insertions(+)
>  create mode 100644 libavcodec/ppc/hevcdsp.c
>  create mode 100644 libavcodec/ppc/hevcdsp_template.c

We're getting close ..

> --- /dev/null
> +++ b/libavcodec/ppc/hevcdsp.c
> +#if HAVE_ALTIVEC_H
> +#include "libavutil/ppc/cpu.h"
> +#include "libavutil/ppc/types_altivec.h"
> +#include "libavutil/ppc/util_altivec.h"
> +#endif

These headers do not in any way depend on HAVE_ALTIVEC_H.
You can #include them directly.

> +#if HAVE_ALTIVEC
> +#define FUNCDECL(a, depth) a ## _ ## depth ## _altivec
> +#define FUNC(a, b) FUNCDECL(a, b)

Move these macros down to where they are used ..

> +#define BIT_DEPTH 8
> +#include "libavcodec/ppc/hevcdsp_template.c"
> +#undef BIT_DEPTH
> +
> +#define BIT_DEPTH 10
> +#include "libavcodec/ppc/hevcdsp_template.c"
> +#undef BIT_DEPTH
> +#endif /* HAVE_ALTIVEC */

.. just above this block.

Also, you don't need the "libavcodec/ppc/" prefix for an inclusion in
the same directory.

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hevc: Add HEVC IDCT 4x4 for PPC

[Luca Barbato]

On 11/11/2016 12:21, Diego Biurrun wrote:
> But that type is not used in the code. I think it's an artifact of the
> bit_depth_template.c inclusion, which is a mistake.

If so probably it is safe to remove then.
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hevc: Add HEVC IDCT 4x4 for PPC

[Diego Biurrun]

On Fri, Nov 11, 2016 at 12:03:30PM +0100, Luca Barbato wrote:
> On 11/11/2016 11:02, Diego Biurrun wrote:
> > What is the undef for?
> 
> pixel is an altivec shortcut type defined in altivec.h

But that type is not used in the code. I think it's an artifact of the
bit_depth_template.c inclusion, which is a mistake.

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hevc: Add HEVC IDCT 4x4 for PPC

[Luca Barbato]

On 11/11/2016 11:02, Diego Biurrun wrote:
> What is the undef for?

pixel is an altivec shortcut type defined in altivec.h

lu
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hevc: Add HEVC IDCT 4x4 for PPC

[Diego Biurrun]

On Fri, Nov 11, 2016 at 10:36:21AM +0100, Alexandra Hájková wrote:
> --- /dev/null
> +++ b/libavcodec/ppc/hevcdsp.c
> @@ -0,0 +1,107 @@
> +
> +#include "config.h"
> +#if HAVE_ALTIVEC_H
> +#include 
> +#undef pixel
> +#endif

What is the undef for?

> +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 },
> +};
> +
> +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 },
> +};
> +
> +#if HAVE_ALTIVEC

Is there a reason to keep the tables out of the #if?

> +#endif /* HAVE_ALTIVEC */
> +
> +#define BIT_DEPTH 8
> +#include "libavcodec/ppc/hevcdsp_template.c"
> +#undef BIT_DEPTH
> +
> +#define BIT_DEPTH 10
> +#include "libavcodec/ppc/hevcdsp_template.c"
> +#undef BIT_DEPTH

.. or the templat files?

> +av_cold void ff_hevc_dsp_init_ppc(HEVCDSPContext *c, const int bit_depth)
> +{
> +
> +#if HAVE_ALTIVEC
> +if (!PPC_ALTIVEC(av_get_cpu_flags()))
> +return;

still stray empty line

> --- /dev/null
> +++ b/libavcodec/ppc/hevcdsp_template.c
> @@ -0,0 +1,54 @@
> +
> +#include "libavcodec/bit_depth_template.c"

What do you need this for?

> +#undef FUNC
> +#define FUNCDECL(a, depth) a ## _ ## depth ## _ ##altivec

Spaces around ##; but _ and altivec are not parameters, so just concatenate
them yourself instead of letting the preprocessor do it.

> +#define FUNC(a, b) FUNCDECL(a, b)

This redirection is good for - what?

What happens if you compile with AltiVec disabled? Does everything
compile and pass tests? Do you get warnings?

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hevc: Add HEVC IDCT 4x4 for PPC

[Diego Biurrun]

On Thu, Nov 10, 2016 at 02:59:15PM +0100, Diego Biurrun wrote:
> On Thu, Nov 10, 2016 at 02:41:05PM +0100, Alexandra Hájková wrote:
> > --- /dev/null
> > +++ b/libavcodec/ppc/hevcdsp.c
> > @@ -0,0 +1,107 @@
> > +if (!PPC_ALTIVEC(av_get_cpu_flags()))
> > +return;
> > +
> > +if (bit_depth == 8)
> > +c->idct[0] = ff_hevc_idct_4x4_8;
> > +if (bit_depth == 10)
> > +c->idct[0] = ff_hevc_idct_4x4_10;
> 
> These functions use AltiVec, so they should have a "_altivec" suffix.
> We always add a suffix indicating the SIMD type to SIMD-optimized
> functions.

The name without suffix is reserved for the C functions, which not
always have a "_c" suffix themselves.

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hevc: Add HEVC IDCT 4x4 for PPC

[Anton Khirnov]

Quoting Alexandra Hájková (2016-11-05 14:32:58)
> ---
> Applied review comments as discussed.
> Tested on both LE and BE.

The patch does not apply.

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hevc: Add HEVC IDCT 4x4 for PPC

[Luca Barbato]

On 05/11/2016 14:32, Alexandra Hájková wrote:
> ---
> Applied review comments as discussed.
> Tested on both LE and BE.
> 
>  libavcodec/hevcdsp.c  |   2 +
>  libavcodec/hevcdsp.h  |   1 +
>  libavcodec/ppc/Makefile   |   1 +
>  libavcodec/ppc/hevcdsp.c  | 107 
> ++
>  libavcodec/ppc/hevcdsp_template.c |  50 ++
>  5 files changed, 161 insertions(+)
>  create mode 100644 libavcodec/ppc/hevcdsp.c
>  create mode 100644 libavcodec/ppc/hevcdsp_template.c
> 

Looks fine to me.

lu

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hevc: Add HEVC IDCT 4x4 for PPC

[Alexandra Hájková]

>> +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?

the reason is I need to multiply the vectors of the same type
>
>> +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.

This is permutation mask used by vec_perm and it's specific for this
case (which is matrix tranposition).

Alexandra
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hevc: Add HEVC IDCT 4x4 for PPC

[Diego Biurrun]

On Tue, Nov 01, 2016 at 09:37:50AM +0100, Alexandra Hájková wrote:
> From: Alexandra Hajkova 

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