[3/3] libavutil: Make LOCAL_ALIGNED(xx be equal to LOCAL_ALIGNED_xx(

Message ID 20170329110232.22383-3-martin@martin.st
State Committed
Headers show

Commit Message

Martin Storsjö March 29, 2017, 11:02 a.m.
Previously, the former form always produced a manually aligned,
padded buffer, while the latter can use DECLARE_ALIGNED, if that
amount of stack alignment is supported.
---
The LOCAL_ALIGNED(4) case in hevc_mvs.c had to be removed, since we
don't have any LOCAL_ALIGNED_4 macro.

This avoids having to use the slightly awkward LOCAL_ALIGNED_xx syntax.
Contrary to the patches posted yesterday, this leaves the source
inconsistent, using both syntaxes though. Should we settle on using
LOCAL_ALIGNED(xx) consistently instead?

That is, this is an alternative to https://patches.libav.org/patch/63160/.
---
 libavutil/internal.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Luca Barbato March 29, 2017, 11:23 a.m. | #1
On 29/03/2017 13:02, Martin Storsjö wrote:
> Should we settle on using LOCAL_ALIGNED(xx) consistently instead?

I'm all for consistency.

the set looks fine to me btw.
Diego Biurrun March 29, 2017, 5:50 p.m. | #2
On Wed, Mar 29, 2017 at 02:02:32PM +0300, Martin Storsjö wrote:
> Previously, the former form always produced a manually aligned,
> padded buffer, while the latter can use DECLARE_ALIGNED, if that
> amount of stack alignment is supported.
> ---
> The LOCAL_ALIGNED(4) case in hevc_mvs.c had to be removed, since we
> don't have any LOCAL_ALIGNED_4 macro.
> 
> This avoids having to use the slightly awkward LOCAL_ALIGNED_xx syntax.
> Contrary to the patches posted yesterday, this leaves the source
> inconsistent, using both syntaxes though. Should we settle on using
> LOCAL_ALIGNED(xx) consistently instead?
> 
> That is, this is an alternative to https://patches.libav.org/patch/63160/.

I prefer this approach and I think we should settle on LOCAL_ALIGNED(xx).

OK

Diego
Martin Storsjö March 29, 2017, 6:45 p.m. | #3
On Wed, 29 Mar 2017, Martin Storsjö wrote:

> Previously, the former form always produced a manually aligned,
> padded buffer, while the latter can use DECLARE_ALIGNED, if that
> amount of stack alignment is supported.
> ---
> The LOCAL_ALIGNED(4) case in hevc_mvs.c had to be removed, since we
> don't have any LOCAL_ALIGNED_4 macro.
>
> This avoids having to use the slightly awkward LOCAL_ALIGNED_xx syntax.
> Contrary to the patches posted yesterday, this leaves the source
> inconsistent, using both syntaxes though. Should we settle on using
> LOCAL_ALIGNED(xx) consistently instead?
>
> That is, this is an alternative to https://patches.libav.org/patch/63160/.
> ---
> libavutil/internal.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)

FWIW, I had to amend this patch to include mem.h in internal.h, as it uses 
DECLARE_ALIGNED which is defined in mem.h. (The same was present in the 
alternative patch as well, but I didn't notice the issue so far.)

// Martin

Patch

diff --git a/libavutil/internal.h b/libavutil/internal.h
index d96762c75d..75ae680b91 100644
--- a/libavutil/internal.h
+++ b/libavutil/internal.h
@@ -97,24 +97,24 @@ 
     DECLARE_ALIGNED(a, t, la_##v) s o;                  \
     t (*v) o = la_##v
 
-#define LOCAL_ALIGNED(a, t, v, ...) E1(LOCAL_ALIGNED_A(a, t, v, __VA_ARGS__,,))
+#define LOCAL_ALIGNED(a, t, v, ...) LOCAL_ALIGNED_##a(t, v, __VA_ARGS__)
 
 #if HAVE_LOCAL_ALIGNED_8
 #   define LOCAL_ALIGNED_8(t, v, ...) E1(LOCAL_ALIGNED_D(8, t, v, __VA_ARGS__,,))
 #else
-#   define LOCAL_ALIGNED_8(t, v, ...) LOCAL_ALIGNED(8, t, v, __VA_ARGS__)
+#   define LOCAL_ALIGNED_8(t, v, ...) E1(LOCAL_ALIGNED_A(8, t, v, __VA_ARGS__,,))
 #endif
 
 #if HAVE_LOCAL_ALIGNED_16
 #   define LOCAL_ALIGNED_16(t, v, ...) E1(LOCAL_ALIGNED_D(16, t, v, __VA_ARGS__,,))
 #else
-#   define LOCAL_ALIGNED_16(t, v, ...) LOCAL_ALIGNED(16, t, v, __VA_ARGS__)
+#   define LOCAL_ALIGNED_16(t, v, ...) E1(LOCAL_ALIGNED_A(16, t, v, __VA_ARGS__,,))
 #endif
 
 #if HAVE_LOCAL_ALIGNED_32
 #   define LOCAL_ALIGNED_32(t, v, ...) E1(LOCAL_ALIGNED_D(32, t, v, __VA_ARGS__,,))
 #else
-#   define LOCAL_ALIGNED_32(t, v, ...) LOCAL_ALIGNED(32, t, v, __VA_ARGS__)
+#   define LOCAL_ALIGNED_32(t, v, ...) E1(LOCAL_ALIGNED_A(32, t, v, __VA_ARGS__,,))
 #endif
 
 #define FF_ALLOC_OR_GOTO(ctx, p, size, label)\