[FFmpeg-devel] [PATCH v3] libavcodec/flacenc: add backward-compatible 32 bit-per-sample capability

[Lynne]

3 Jan 2022, 21:26 by mvanb1 at gmail.com:

> +        if (pmax > 0) {
> +            if (lpc_reduction_tries >= 2)
> +                return 0;
> +            lpc_reduction_tries++;
> +            for (int i = 0; i < order; i++)
> +                coefs[i] = ((int64_t)coefs[i] * INT32_MAX) / pmax;
> +        }
> +    } while (pmax > 0);
> +    return 1;
> +}
>

Could you invert the return value of the function?
Return 0 if there's no overflow and 1 if there is? That's
usually how we use return values throughout the code
too, and it saves you from having to invert it when you
use it.


>  static void flac_lpc_16_c(int32_t *decoded, const int coeffs[32],
>  int pred_order, int qlevel, int len)
>  {
> diff --git a/libavcodec/flacdsp.h b/libavcodec/flacdsp.h
> index 7bb0dd0e9a..5978a4722a 100644
> --- a/libavcodec/flacdsp.h
> +++ b/libavcodec/flacdsp.h
> @@ -40,4 +40,7 @@ void ff_flacdsp_init(FLACDSPContext *c, enum AVSampleFormat fmt, int channels, i
>  void ff_flacdsp_init_arm(FLACDSPContext *c, enum AVSampleFormat fmt, int channels, int bps);
>  void ff_flacdsp_init_x86(FLACDSPContext *c, enum AVSampleFormat fmt, int channels, int bps);
>  
> +int ff_flacdsp_lpc_encode_c_32_overflow_detect(int32_t *res, const int32_t *smp, int len,
> +                                               int order, int32_t *coefs, int shift);
>

Could you also move the function to flacenc.c? The flacdsp.c
file gets compiled if either is enabled, but if flacenc.c isn't
enabled, then this code shouldn't be enabled, to save space.
The function isn't likely to get SIMD, and the compiler could
do more aggressive optimizations if it's in the same file.
 

>  put_sbits(&s->pb, sub->obits, res[0]);
>  } else if (sub->type == FLAC_SUBFRAME_VERBATIM) {
> -            while (res < frame_end)
> -                put_sbits(&s->pb, sub->obits, *res++);
> +            if (sub->obits < 32) {
> +                while (res < frame_end)
> +                    put_sbits(&s->pb, sub->obits, *res++);
> +            } else {
> +                while (res < frame_end)
> +                    put_bits32(&s->pb, *res++);
> +            }
>  } else {
>  /* warm-up samples */
> -            for (i = 0; i < sub->order; i++)
> -                put_sbits(&s->pb, sub->obits, *res++);
> +            if (sub->obits < 32) {
> +                for (i = 0; i < sub->order; i++)
> +                    put_sbits(&s->pb, sub->obits, *res++);
> +            }else{
> +                for (i = 0; i < sub->order; i++)
> +                    put_bits32(&s->pb, *res++);
> +            }
>

Small nit, but could you put spaces in the }else{

Apart from that, looks good.