[FFmpeg-devel] [PATCH] swresample: Add support for clipping float/double to -1.0..1.0 range

[Ronald S. Bultje]

Hi,

On Fri, Oct 23, 2015 at 12:08 PM, Michael Niedermayer <michaelni at gmx.at>
wrote:

> From: Michael Niedermayer <michael at niedermayer.cc>
>
> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> ---
>  libswresample/aarch64/audio_convert_init.c |    8 ++++-
>  libswresample/arm/audio_convert_init.c     |    8 ++++-
>  libswresample/audioconvert.c               |   44
> ++++++++++++++++++++++++++--
>  libswresample/options.c                    |    1 +
>  libswresample/swresample.c                 |    6 ++--
>  libswresample/swresample.h                 |    1 +
>  libswresample/swresample_internal.h        |    6 ++--
>  libswresample/x86/audio_convert_init.c     |    8 ++++-
>  8 files changed, 71 insertions(+), 11 deletions(-)
>
> diff --git a/libswresample/aarch64/audio_convert_init.c
> b/libswresample/aarch64/audio_convert_init.c
> index 60e24ad..dedb1aa 100644
> --- a/libswresample/aarch64/audio_convert_init.c
> +++ b/libswresample/aarch64/audio_convert_init.c
> @@ -48,12 +48,18 @@ static void conv_fltp_to_s16_nch_neon(uint8_t **dst,
> const uint8_t **src, int le
>  av_cold void swri_audio_convert_init_aarch64(struct AudioConvert *ac,
>                                         enum AVSampleFormat out_fmt,
>                                         enum AVSampleFormat in_fmt,
> -                                       int channels)
> +                                       int channels, int flags)
>  {
>      int cpu_flags = av_get_cpu_flags();
>
>      ac->simd_f= NULL;
>
> +    if (   (flags & SWR_FLAG_CLIP)
> +        && av_get_packed_sample_fmt(in_fmt) == AV_SAMPLE_FMT_FLT
> +        && av_get_packed_sample_fmt(out_fmt) == AV_SAMPLE_FMT_FLT) {
> +        return;
> +    }
> +
>      if (have_neon(cpu_flags)) {
>          if(out_fmt == AV_SAMPLE_FMT_S16 && in_fmt == AV_SAMPLE_FMT_FLT ||
> out_fmt == AV_SAMPLE_FMT_S16P && in_fmt == AV_SAMPLE_FMT_FLTP)
>              ac->simd_f = conv_flt_to_s16_neon;


Hm, ... OK, so ... This reminds me of swscale. Let me explain why that's
bad before we try to think of solutions.

Once upon a time, there was no scaling library. Then, poof, like magic,
there was swscale. But, it had all kind of assumptions hardcoded - as is
common in any first iteration of a new software product. So, we introduced
flags, like the above:

ffmpeg -h full, searching to -sws_flags, gives this:
  -sws_flags         <flags>      E..V.... scaler flags (default 4)
     fast_bilinear                E..V.... fast bilinear
     bilinear                     E..V.... bilinear
     bicubic                      E..V.... bicubic
     experimental                 E..V.... experimental
     neighbor                     E..V.... nearest neighbor
     area                         E..V.... averaging area
     bicublin                     E..V.... luma bicubic, chroma bilinear
     gauss                        E..V.... Gaussian
     sinc                         E..V.... sinc
     lanczos                      E..V.... Lanczos
     spline                       E..V.... natural bicubic spline
     print_info                   E..V.... print info
     accurate_rnd                 E..V.... accurate rounding
     full_chroma_int              E..V.... full chroma interpolation
     full_chroma_inp              E..V.... full chroma input
     bitexact                     E..V....
     error_diffusion              E..V.... error diffusion dither

OK, so first, there's so many things wrong here, we're using flags as a way
to indicate the scaling method, and mixing up boolean flags. By the way,
did you know "experimental" is a scaling method? Fantastic help. The
default is "4", but we don't know what flag "4" corresponds to. But that's
not what my main concern is, so let's move beyond that.

My concern is with things like full_chroma_int, inp, bitexact,
error_diffusion. If you specify any of these flags, can you be sure they
are always applied? Your answer will be "try it and file bugs if not" - but
we have no scientific way of verifying the result. I challenge anyone going
through sws_init_context() and related functions and trying to figure out
which function is applied if I scale or not scale and I apply a particular
set of flags. (Who can tell me the difference between full_chroma_int and
inp without looking at the source or docs?)

For the most part, we _don't know_. I spent probably half a year trying to
understand it and I eventually gave up because, well, who cares... The
beast is too big to tame. We can do it on a case by case basis, but
systematically, there is no way to verify anything. I know there's bugs
where some of the above flags are applied only in some cases, and so even
though I ask it to do full chroma, it doesn't actually do it, because the
flag is not checked in all cases. Good luck trying to figure out if your
custom yuv-rgb coefficients are applied if you go from rgb to yuv and/or
back. I give you a 50% chance.

Now, we can prevent that from becoming an issue in swr. How do we do that.
We _test_ it. We test _everything_. _Every_ flag, when added, is tested. We
don't just test that the output didn't change versus yesterday, but we test
that the behaviour is actually correct. So in your case, we should add all
swr tests with and without this flag, and confirm that the results _change_
for the relevant (float) formats. If they don't change, either the
implementation is buggy, or the test is invalid because it doesn't trigger
all conditions.

This way, the test is self-documenting of the intention of the flag, and we
can confirm it works in all cases. Had we done that with swscale, we could
have prevented a lot of obscure misbehaviour, but again - too late. Nothing
can be done We can just scrape the bugs that are reported and hope we don't
make it worse... Others may have alternate ideas on how to accomplish the
same thing (checkasm comes to mind also; maybe unit tests?)

Ronald