Re: [FFmpeg-devel] [PATCH 2/2] avcodec/apedec: Implement interim mode detection

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 5661 bytes --]

On Sat, Aug 26, 2023 at 07:29:40PM +0200, Paul B Mahol wrote:
> On Sat, Aug 26, 2023 at 6:54 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > Fixes: NoLegacy.ape
> > Found-by: Matt Ashland <mail@monkeysaudio.com>
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/apedec.c | 106 +++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 84 insertions(+), 22 deletions(-)
> >
> > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
> > index 8bd625ca05..249fc22e24 100644
> > --- a/libavcodec/apedec.c
> > +++ b/libavcodec/apedec.c
> > @@ -171,6 +171,9 @@ typedef struct APEContext {
> >      int32_t *decoded_buffer;
> >      int decoded_size;
> >      int32_t *decoded[MAX_CHANNELS];          ///< decoded data for each
> > channel
> > +    int32_t *interim_buffer;
> > +    int interim_size;
> > +    int32_t *interim[MAX_CHANNELS];          ///< decoded data for each
> > channel
> >      int blocks_per_loop;                     ///< maximum number of
> > samples to decode for each call
> >
> >      int16_t* filterbuf[APE_FILTER_LEVELS];   ///< filter memory
> > @@ -187,6 +190,7 @@ typedef struct APEContext {
> >      const uint8_t *ptr;                      ///< current position in
> > frame data
> >
> >      int error;
> > +    int interim_mode;
> >
> >      void (*entropy_decode_mono)(struct APEContext *ctx, int
> > blockstodecode);
> >      void (*entropy_decode_stereo)(struct APEContext *ctx, int
> > blockstodecode);
> > @@ -223,6 +227,7 @@ static av_cold int ape_decode_close(AVCodecContext
> > *avctx)
> >          av_freep(&s->filterbuf[i]);
> >
> >      av_freep(&s->decoded_buffer);
> > +    av_freep(&s->interim_buffer);
> >      av_freep(&s->data);
> >      s->decoded_size = s->data_size = 0;
> >
> > @@ -248,12 +253,15 @@ static av_cold int ape_decode_init(AVCodecContext
> > *avctx)
> >      switch (s->bps) {
> >      case 8:
> >          avctx->sample_fmt = AV_SAMPLE_FMT_U8P;
> > +        s->interim_mode = 0;
> >          break;
> >      case 16:
> >          avctx->sample_fmt = AV_SAMPLE_FMT_S16P;
> > +        s->interim_mode = 0;
> >          break;
> >      case 24:
> >          avctx->sample_fmt = AV_SAMPLE_FMT_S32P;
> > +        s->interim_mode = -1;
> >          break;
> >      default:
> >          avpriv_request_sample(avctx,
> > @@ -1181,7 +1189,7 @@ static av_always_inline int
> > predictor_update_filter(APEPredictor64 *p,
> >                                                      const int decoded,
> > const int filter,
> >                                                      const int delayA,
> > const int delayB,
> >                                                      const int adaptA,
> > const int adaptB,
> > -                                                    int compression_level)
> > +                                                    int interim_mode)
> >  {
> >      int64_t predictionA, predictionB;
> >      int32_t sign;
> > @@ -1209,7 +1217,7 @@ static av_always_inline int
> > predictor_update_filter(APEPredictor64 *p,
> >                    p->buf[delayB - 3] * p->coeffsB[filter][3] +
> >                    p->buf[delayB - 4] * p->coeffsB[filter][4];
> >
> > -    if (compression_level < COMPRESSION_LEVEL_INSANE) {
> > +    if (interim_mode < 1) {
> >          predictionA = (int32_t)predictionA;
> >          predictionB = (int32_t)predictionB;
> >          p->lastA[filter] = decoded + ((int32_t)(predictionA +
> > (predictionB >> 1)) >> 10);
> > @@ -1234,33 +1242,74 @@ static av_always_inline int
> > predictor_update_filter(APEPredictor64 *p,
> >
> >  static void predictor_decode_stereo_3950(APEContext *ctx, int count)
> >  {
> > -    APEPredictor64 *p = &ctx->predictor64;
> > -    int32_t *decoded0 = ctx->decoded[0];
> > -    int32_t *decoded1 = ctx->decoded[1];
> > +    APEPredictor64 *p_default = &ctx->predictor64;
> > +    APEPredictor64 p_interim;
> > +    int lcount = count;
> > +    int num_passes = 1;
> >
> >      ape_apply_filters(ctx, ctx->decoded[0], ctx->decoded[1], count);
> > +    if (ctx->interim_mode == -1) {
> > +        p_interim = *p_default;
> > +        num_passes ++;
> > +        memcpy(ctx->interim[0], ctx->decoded[0],
> > sizeof(*ctx->interim[0])*count);
> > +        memcpy(ctx->interim[1], ctx->decoded[1],
> > sizeof(*ctx->interim[1])*count);
> > +    }
> >
> > -    while (count--) {
> > -        /* Predictor Y */
> > -        *decoded0 = predictor_update_filter(p, *decoded0, 0, YDELAYA,
> > YDELAYB,
> > -                                            YADAPTCOEFFSA, YADAPTCOEFFSB,
> > -                                            ctx->compression_level);
> > -        decoded0++;
> > -        *decoded1 = predictor_update_filter(p, *decoded1, 1, XDELAYA,
> > XDELAYB,
> > -                                            XADAPTCOEFFSA, XADAPTCOEFFSB,
> > -                                            ctx->compression_level);
> > -        decoded1++;
> > +    for(int pass = 0; pass < num_passes; pass++) {
> >
> 
> Please fix your style in patches.

Noone can know from this what exactly you meant but
ill apply with the whitespace issue i found, which may be what you meant.
No need to be precisse, thats ok with me if you prefer that but a
inprecisse review comment will result in inprecisse results

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".