[FFmpeg-devel] [PATCHv2 2/2] all: do standards compliant absdiff computation

Ganesh Ajjanagadde gajjanagadde at gmail.com
Fri Sep 11 05:16:55 CEST 2015


On Sun, Aug 23, 2015 at 6:11 PM, Ganesh Ajjanagadde
<gajjanagadde at gmail.com> wrote:
> This resolves undefined behavior, and also silences -Wabsolute-value in clang 3.5+.
>
> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
> ---
>  libavcodec/flashsv2enc.c | 6 +++---
>  libavfilter/vf_hqx.c     | 6 +++---
>  libavfilter/vf_xbr.c     | 6 +++---
>  3 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/libavcodec/flashsv2enc.c b/libavcodec/flashsv2enc.c
> index c2c00f4..75b9a32 100644
> --- a/libavcodec/flashsv2enc.c
> +++ b/libavcodec/flashsv2enc.c
> @@ -415,9 +415,9 @@ static inline unsigned int chroma_diff(unsigned int c1, unsigned int c2)
>      unsigned int t1 = (c1 & 0x000000ff) + ((c1 & 0x0000ff00) >> 8) + ((c1 & 0x00ff0000) >> 16);
>      unsigned int t2 = (c2 & 0x000000ff) + ((c2 & 0x0000ff00) >> 8) + ((c2 & 0x00ff0000) >> 16);
>
> -    return abs(t1 - t2) + abs((c1 & 0x000000ff) - (c2 & 0x000000ff)) +
> -        abs(((c1 & 0x0000ff00) >> 8) - ((c2 & 0x0000ff00) >> 8)) +
> -        abs(((c1 & 0x00ff0000) >> 16) - ((c2 & 0x00ff0000) >> 16));
> +    return FFABSDIFF(t1, t2) + FFABSDIFF(c1 & 0x000000ff, c2 & 0x000000ff) +
> +        FFABSDIFF((c1 & 0x0000ff00) >> 8 , (c2 & 0x0000ff00) >> 8) +
> +        FFABSDIFF((c1 & 0x00ff0000) >> 16, (c2 & 0x00ff0000) >> 16);
>  }
>
>  static inline int pixel_color7_fast(Palette * palette, unsigned c15)
> diff --git a/libavfilter/vf_hqx.c b/libavfilter/vf_hqx.c
> index fa15d9c..407b9b5 100644
> --- a/libavfilter/vf_hqx.c
> +++ b/libavfilter/vf_hqx.c
> @@ -65,9 +65,9 @@ static av_always_inline int yuv_diff(uint32_t yuv1, uint32_t yuv2)
>  #define YMASK 0xff0000
>  #define UMASK 0x00ff00
>  #define VMASK 0x0000ff
> -    return abs((yuv1 & YMASK) - (yuv2 & YMASK)) > (48 << 16) ||
> -           abs((yuv1 & UMASK) - (yuv2 & UMASK)) > ( 7 <<  8) ||
> -           abs((yuv1 & VMASK) - (yuv2 & VMASK)) > ( 6 <<  0);
> +    return FFABSDIFF(yuv1 & YMASK, yuv2 & YMASK) > (48 << 16) ||
> +           FFABSDIFF(yuv1 & UMASK, yuv2 & UMASK) > ( 7 <<  8) ||
> +           FFABSDIFF(yuv1 & VMASK, yuv2 & VMASK) > ( 6 <<  0);
>  }
>
>  /* (c1*w1 + c2*w2) >> s */
> diff --git a/libavfilter/vf_xbr.c b/libavfilter/vf_xbr.c
> index 38c3b70..1c6a8ea 100644
> --- a/libavfilter/vf_xbr.c
> +++ b/libavfilter/vf_xbr.c
> @@ -69,9 +69,9 @@ static uint32_t pixel_diff(uint32_t x, uint32_t y, const uint32_t *r2y)
>      uint32_t yuv1 = r2y[x & 0xffffff];
>      uint32_t yuv2 = r2y[y & 0xffffff];
>
> -    return (abs((yuv1 & YMASK) - (yuv2 & YMASK)) >> 16) +
> -           (abs((yuv1 & UMASK) - (yuv2 & UMASK)) >>  8) +
> -           abs((yuv1 & VMASK) - (yuv2 & VMASK));
> +    return (FFABSDIFF(yuv1 & YMASK, yuv2 & YMASK) >> 16) +
> +           (FFABSDIFF(yuv1 & UMASK, yuv2 & UMASK) >>  8) +
> +            FFABSDIFF(yuv1 & VMASK, yuv2 & VMASK);
>  }
>
>  #define ALPHA_BLEND_128_W(a, b) ((((a) & LB_MASK) >> 1) + (((b) & LB_MASK) >> 1))
> --
> 2.5.0
>

I do not recall any objections to this patchv2 (v1 had efficiency
issues that are fixed with this). IIRC, the only comment that applies
to this v2 patch is one from Michael:
https://ffmpeg.org/pipermail/ffmpeg-devel/2015-August/177796.html.

I believe this patchv2 series should be applied. True, FFmpeg did work
correctly before this patch, but it was relying on implementation
defined behavior. This patch series not only silences the warnings on
clang, but also does so at no efficiency loss (compare the generated
asm; v1 had this issue while v2 does not), and has the added benefit
of not relying on implementation defined behavior.

IMHO, when one looks at the things the codebase already does to
silence warnings (see e.g
https://ffmpeg.org/pipermail/ffmpeg-devel/2015-August/177184.html, a
workaround useful for GCC < 4.6), this should be ok.

Note that the commit message should be changed from "undefined" to
"implementation defined".


More information about the ffmpeg-devel mailing list