[09/10] dcadsp: Add a new method, qmf_32_subbands

Message ID 1373980994-30628-9-git-send-email-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö July 16, 2013, 1:23 p.m.
From: Ben Avison <bavison@riscosopen.org>

This does most of the work formerly carried out by
the static function qmf_32_subbands() in dcadec.c.
---
 libavcodec/dcadec.c |   32 ++++++++------------------------
 libavcodec/dcadsp.c |   32 ++++++++++++++++++++++++++++++++
 libavcodec/dcadsp.h |    6 ++++++
 3 files changed, 46 insertions(+), 24 deletions(-)

Comments

Luca Barbato July 16, 2013, 3:19 p.m. | #1
On 07/16/2013 03:23 PM, Martin Storsjö wrote:
> From: Ben Avison <bavison@riscosopen.org>
> 
> This does most of the work formerly carried out by
> the static function qmf_32_subbands() in dcadec.c.
> ---
>  libavcodec/dcadec.c |   32 ++++++++------------------------
>  libavcodec/dcadsp.c |   32 ++++++++++++++++++++++++++++++++
>  libavcodec/dcadsp.h |    6 ++++++
>  3 files changed, 46 insertions(+), 24 deletions(-)

Seems a good refactoring.
Martin Storsjö July 16, 2013, 4:26 p.m. | #2
On Tue, 16 Jul 2013, Martin Storsjö wrote:

> From: Ben Avison <bavison@riscosopen.org>
>
> This does most of the work formerly carried out by
> the static function qmf_32_subbands() in dcadec.c.
> ---
> libavcodec/dcadec.c |   32 ++++++++------------------------
> libavcodec/dcadsp.c |   32 ++++++++++++++++++++++++++++++++
> libavcodec/dcadsp.h |    6 ++++++
> 3 files changed, 46 insertions(+), 24 deletions(-)
>
> diff --git a/libavcodec/dcadec.c b/libavcodec/dcadec.c
> index 84d522d..8be4b74 100644
> --- a/libavcodec/dcadec.c
> +++ b/libavcodec/dcadec.c
> @@ -945,39 +945,23 @@ static void qmf_32_subbands(DCAContext *s, int chans,
>                             float samples_in[32][8], float *samples_out,
>                             float scale)
> {
> -    const float *prCoeff;
> -    int i;
> +    const float (*prCoeff)[512];
>
>     int sb_act = s->subband_activity[chans];
> -    int subindex;
>
>     scale *= sqrt(1 / 8.0);
>
>     /* Select filter */
>     if (!s->multirate_inter)    /* Non-perfect reconstruction */
> -        prCoeff = fir_32bands_nonperfect;
> +        prCoeff = &fir_32bands_nonperfect;
>     else                        /* Perfect reconstruction */
> -        prCoeff = fir_32bands_perfect;

Ben, what's the idea behind passing this and all the other arrays as 
pointers to pointers instead of plain pointers? Is there any benefit to it 
in the VFP assembly code? For the plain C code it feels like it makes 
things a bit more complicated at least.

// Martin
Ben Avison July 16, 2013, 4:55 p.m. | #3
On Tue, 16 Jul 2013 17:26:23 +0100, Martin Storsjö <martin@martin.st> wrote:
> Ben, what's the idea behind passing this and all the other arrays as
> pointers to pointers instead of plain pointers? Is there any benefit to
> it in the VFP assembly code?

Pointers to arrays, not pointers to pointers, that's an important
distinction. Makes no difference at the assembly level at all - it's the
same number being passed around, without any extra dereferences, as there
would be with a pointer to a pointer. It's effectively just an extra layer
of type safety applied by the C compiler, as well as a reminder to authors
of the nature of the data. By converting a pointer to an array to a pointer
to the first element of an array (which is what it used to do) you're just
throwing away information about the extent of the data.

It came about when I noticed that in the type definition for
synth_filter_float, the parameter "window" was given as a const float[512],
and that it was initialised from either fir_32bands_perfect or
fir_32bands_nonperfect, both of which are arrays of 512 const floats. I
thought it made sense to preserve the data type as the address of an array
of 512 floats through the intermediate variables down to
synth_filter_float, rather than using generic float * types. Notice how no
casts are required at any stage.

Ben
Martin Storsjö July 16, 2013, 5:18 p.m. | #4
On Tue, 16 Jul 2013, Ben Avison wrote:

> On Tue, 16 Jul 2013 17:26:23 +0100, Martin Storsjö <martin@martin.st> wrote:
>> Ben, what's the idea behind passing this and all the other arrays as
>> pointers to pointers instead of plain pointers? Is there any benefit to
>> it in the VFP assembly code?
>
> Pointers to arrays, not pointers to pointers, that's an important
> distinction. Makes no difference at the assembly level at all - it's the
> same number being passed around, without any extra dereferences, as there
> would be with a pointer to a pointer. It's effectively just an extra layer
> of type safety applied by the C compiler, as well as a reminder to authors
> of the nature of the data. By converting a pointer to an array to a pointer
> to the first element of an array (which is what it used to do) you're just
> throwing away information about the extent of the data.
>
> It came about when I noticed that in the type definition for
> synth_filter_float, the parameter "window" was given as a const float[512],
> and that it was initialised from either fir_32bands_perfect or
> fir_32bands_nonperfect, both of which are arrays of 512 const floats. I
> thought it made sense to preserve the data type as the address of an array
> of 512 floats through the intermediate variables down to
> synth_filter_float, rather than using generic float * types. Notice how no
> casts are required at any stage.

Right. But "const float foo[512]" is, at the C level, one level of 
indirection less than "const float (*foo)[512]". I got rid of that in 
https://github.com/mstorsjo/libav/commit/41b32c3577f11584d45c2542c81285596c50e998 
(which I'll squash into the two preceding commits soon, unless you've got 
objects to it).

I still had to keep the prCoeff variable within qmf_32_subbands as pointer 
to array, to avoid having to use casts, while still retaining the array 
length meta-information.

// Martin
Ben Avison July 16, 2013, 5:49 p.m. | #5
> Right. But "const float foo[512]" is, at the C level, one level of
> indirection less than "const float (*foo)[512]". I got rid of that in
> https://github.com/mstorsjo/libav/commit/41b32c3577f11584d45c2542c81285596c50e998
> (which I'll squash into the two preceding commits soon, unless you've got
> objects to it).

One thing I forgot to say about where pointers to arrays are useful
compared to pointers to first elements is within debuggers, where you can
easily print out the whole array and let the debugger work out how many
elements it has from the type information.

That said, I don't have a big problem with you changing it if you prefer.
It certainly shouldn't have any effect on the speed of the resulting
binary, which was my primary concern.

Ben
Luca Barbato July 16, 2013, 6:14 p.m. | #6
On 07/16/2013 07:49 PM, Ben Avison wrote:
>> Right. But "const float foo[512]" is, at the C level, one level of
>> indirection less than "const float (*foo)[512]". I got rid of that in
>> https://github.com/mstorsjo/libav/commit/41b32c3577f11584d45c2542c81285596c50e998
>>
>> (which I'll squash into the two preceding commits soon, unless you've got
>> objects to it).
> 
> One thing I forgot to say about where pointers to arrays are useful
> compared to pointers to first elements is within debuggers, where you can
> easily print out the whole array and let the debugger work out how many
> elements it has from the type information.

The debugger is usually smart enough and we have __asan_describe_address
and, once I feel less lazy and just do it, ff_valgrind_describe_address
with the similar semantic.

lu
Martin Storsjö July 17, 2013, 6:48 p.m. | #7
On Tue, 16 Jul 2013, Ben Avison wrote:

> On Tue, 16 Jul 2013 17:26:23 +0100, Martin Storsjö <martin@martin.st> wrote:
>> Ben, what's the idea behind passing this and all the other arrays as
>> pointers to pointers instead of plain pointers? Is there any benefit to
>> it in the VFP assembly code?
>
> Pointers to arrays, not pointers to pointers, that's an important
> distinction. Makes no difference at the assembly level at all - it's the
> same number being passed around, without any extra dereferences, as there
> would be with a pointer to a pointer. It's effectively just an extra layer
> of type safety applied by the C compiler, as well as a reminder to authors
> of the nature of the data. By converting a pointer to an array to a pointer
> to the first element of an array (which is what it used to do) you're just
> throwing away information about the extent of the data.
>
> It came about when I noticed that in the type definition for
> synth_filter_float, the parameter "window" was given as a const float[512],
> and that it was initialised from either fir_32bands_perfect or
> fir_32bands_nonperfect, both of which are arrays of 512 const floats. I
> thought it made sense to preserve the data type as the address of an array
> of 512 floats through the intermediate variables down to
> synth_filter_float, rather than using generic float * types. Notice how no
> casts are required at any stage.

For the record - while it's a good thing to try to preserve as much extra 
metadata like this as possible, neither gcc (tested on 4.6) nor clang 
(3.2) will actually tell you if you're passing an array of incorrect 
length to a function (not with the current libav warning flags at least).

// Martin

Patch

diff --git a/libavcodec/dcadec.c b/libavcodec/dcadec.c
index 84d522d..8be4b74 100644
--- a/libavcodec/dcadec.c
+++ b/libavcodec/dcadec.c
@@ -945,39 +945,23 @@  static void qmf_32_subbands(DCAContext *s, int chans,
                             float samples_in[32][8], float *samples_out,
                             float scale)
 {
-    const float *prCoeff;
-    int i;
+    const float (*prCoeff)[512];
 
     int sb_act = s->subband_activity[chans];
-    int subindex;
 
     scale *= sqrt(1 / 8.0);
 
     /* Select filter */
     if (!s->multirate_inter)    /* Non-perfect reconstruction */
-        prCoeff = fir_32bands_nonperfect;
+        prCoeff = &fir_32bands_nonperfect;
     else                        /* Perfect reconstruction */
-        prCoeff = fir_32bands_perfect;
-
-    for (i = sb_act; i < 32; i++)
-        s->raXin[i] = 0.0;
-
-    /* Reconstructed channel sample index */
-    for (subindex = 0; subindex < 8; subindex++) {
-        /* Load in one sample from each subband and clear inactive subbands */
-        for (i = 0; i < sb_act; i++) {
-            unsigned sign = (i - 1) & 2;
-            uint32_t v    = AV_RN32A(&samples_in[i][subindex]) ^ sign << 30;
-            AV_WN32A(&s->raXin[i], v);
-        }
+        prCoeff = &fir_32bands_perfect;
 
-        s->synth.synth_filter_float(&s->imdct,
-                                    s->subband_fir_hist[chans],
-                                    &s->hist_index[chans],
-                                    s->subband_fir_noidea[chans], prCoeff,
-                                    samples_out, s->raXin, scale);
-        samples_out += 32;
-    }
+    s->dcadsp.qmf_32_subbands(samples_in, sb_act, &s->synth, &s->imdct,
+                              &s->subband_fir_hist[chans],
+                              &s->hist_index[chans],
+                              &s->subband_fir_noidea[chans], prCoeff,
+                              samples_out, &s->raXin, scale);
 }
 
 static void lfe_interpolation_fir(DCAContext *s, int decimation_select,
diff --git a/libavcodec/dcadsp.c b/libavcodec/dcadsp.c
index 8b2dd42..b94ed56 100644
--- a/libavcodec/dcadsp.c
+++ b/libavcodec/dcadsp.c
@@ -21,7 +21,10 @@ 
 
 #include "config.h"
 #include "libavutil/attributes.h"
+#include "avfft.h"
+#include "synth_filter.h"
 #include "dcadsp.h"
+#include "libavutil/intreadwrite.h"
 
 static void dca_lfe_fir_c(float *out, const float *in, const float *coefs,
                           int decifactor, float scale)
@@ -45,8 +48,37 @@  static void dca_lfe_fir_c(float *out, const float *in, const float *coefs,
     }
 }
 
+static void dca_qmf_32_subbands(float samples_in[32][8], int sb_act,
+                                SynthFilterContext *synth, FFTContext *imdct,
+                                float (*synth_buf_ptr)[512],
+                                int *synth_buf_offset, float (*synth_buf2)[32],
+                                const float (*window)[512], float *samples_out,
+                                float (*raXin)[32], float scale)
+{
+    int i;
+    int subindex;
+
+    for (i = sb_act; i < 32; i++)
+        (*raXin)[i] = 0.0;
+
+    /* Reconstructed channel sample index */
+    for (subindex = 0; subindex < 8; subindex++) {
+        /* Load in one sample from each subband and clear inactive subbands */
+        for (i = 0; i < sb_act; i++) {
+            unsigned sign = (i - 1) & 2;
+            uint32_t v    = AV_RN32A(&samples_in[i][subindex]) ^ sign << 30;
+            AV_WN32A(&(*raXin)[i], v);
+        }
+
+        synth->synth_filter_float(imdct, *synth_buf_ptr, synth_buf_offset,
+                                  *synth_buf2, *window, samples_out, *raXin, scale);
+        samples_out += 32;
+    }
+}
+
 av_cold void ff_dcadsp_init(DCADSPContext *s)
 {
     s->lfe_fir = dca_lfe_fir_c;
+    s->qmf_32_subbands = dca_qmf_32_subbands;
     if (ARCH_ARM) ff_dcadsp_init_arm(s);
 }
diff --git a/libavcodec/dcadsp.h b/libavcodec/dcadsp.h
index 3c6f1f9..4905a22 100644
--- a/libavcodec/dcadsp.h
+++ b/libavcodec/dcadsp.h
@@ -22,6 +22,12 @@ 
 typedef struct DCADSPContext {
     void (*lfe_fir)(float *out, const float *in, const float *coefs,
                     int decifactor, float scale);
+    void (*qmf_32_subbands)(float samples_in[32][8], int sb_act,
+                            SynthFilterContext *synth, FFTContext *imdct,
+                            float (*synth_buf_ptr)[512],
+                            int *synth_buf_offset, float (*synth_buf2)[32],
+                            const float (*window)[512], float *samples_out,
+                            float (*raXin)[32], float scale);
 } DCADSPContext;
 
 void ff_dcadsp_init(DCADSPContext *s);