[FFmpeg-devel] [PATCH] JPEG2000: SSE optimisation of DWT decoding

[Carl Eugen Hoyos]

2017-10-06 17:30 GMT+02:00 Nicolas Bertrand <nicoinattendu at gmail.com>:
> From: Maxime Taisant <maximetaisant at hotmail.fr>
>
> ---
>  libavcodec/jpeg2000dwt.c          |   45 +-
>  libavcodec/jpeg2000dwt.h          |    5 +
>  libavcodec/x86/jpeg2000dsp.asm    | 1339 +++++++++++++++++++++++++++++++++++++
>  libavcodec/x86/jpeg2000dsp_init.c |  119 ++++
>  tests/checkasm/jpeg2000dsp.c      |    1 +
>  5 files changed, 1496 insertions(+), 13 deletions(-)
>
> diff --git a/libavcodec/jpeg2000dwt.c b/libavcodec/jpeg2000dwt.c
> index 55dd5e89b5..1a0c3fc034 100644
> --- a/libavcodec/jpeg2000dwt.c
> +++ b/libavcodec/jpeg2000dwt.c
> @@ -30,6 +30,7 @@
>  #include "libavutil/mem.h"
>  #include "jpeg2000dwt.h"
>  #include "internal.h"
> +#include "libavutil/timer.h"

This include should not be part of the final patch, same
for the commented TIMER calls.
But the results of your performance tests should be
at least part of the email, can be part of the commit
message.

> +    s->sse = 0;

I originally assumed this is a leftover but iiuc, you
need to add function_pointers to the the context
and replace every function call with a call to the
function in the context.

> -    switch (s->type) {
> -    case FF_DWT97:
> -        dwt_decode97_float(s, t);
> -        break;
> -    case FF_DWT97_INT:
> -        dwt_decode97_int(s, t);
> -        break;
> -    case FF_DWT53:
> -        dwt_decode53(s, t);
> -        break;
> -    default:
> -        return -1;
> +    switch(s->type){

> +        case FF_DWT97:

We like the indentation here as it is.

> +            if (s->sse)

> +            //{

Please keep the brackets in the final patch,
this variant is a very good example on why
they are useful;-)

> +            //    START_TIMER

But remove this.

Consider waiting for a review of the asm code.

Thank you, Carl Eugen