[FFmpeg-devel] [aacdec] Parse and drop gain control data, so that SSR packets decode.

[Alex Converse]

On Tue, Feb 20, 2018 at 3:37 PM, Dale Curtis <dalecurtis at chromium.org> wrote:
> From 97380752ef12337d9b9a01801a9a84df3b4b9c0a Mon Sep 17 00:00:00 2001
> From: Dale Curtis <dalecurtis at chromium.org>
> Date: Thu, 15 Feb 2018 16:22:55 -0800
> Subject: [PATCH] [aacdec] Parse and drop gain control data, so that SSR
>  packets decode.
>
> This will result in poor quality audio for SSR streams, but they
> will at least demux and decode without error; partially fixing
> ticket #1693.
>
> This just pulls in the decode_gain_control() function from the
> ffmpeg summer-of-code repo (original author Robert Swain) at
> svn://svn.mplayerhq.hu/soc/aac/aac.c and adds AOT_AAC_SSR to
> decode_audio_specific_config_gb().
>
> Signed-off-by: Dale Curtis <dalecurtis at chromium.org>
> Co-authored-by: Robert Swain <robert.swain at gmail.com>
> ---
>  libavcodec/aacdec_template.c | 49 +++++++++++++++++++++++++++++++++---
>  1 file changed, 46 insertions(+), 3 deletions(-)
>
> diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
> index c2d9802023..c3ec3eefe8 100644
> --- a/libavcodec/aacdec_template.c
> +++ b/libavcodec/aacdec_template.c
> @@ -997,6 +997,7 @@ static int decode_audio_specific_config_gb(AACContext *ac,
>      switch (m4ac->object_type) {
>      case AOT_AAC_MAIN:
>      case AOT_AAC_LC:
> +    case AOT_AAC_SSR:
>      case AOT_AAC_LTP:
>      case AOT_ER_AAC_LC:
>      case AOT_ER_AAC_LD:
> @@ -1967,6 +1968,44 @@ static void apply_prediction(AACContext *ac, SingleChannelElement *sce)
>          reset_all_predictors(sce->predictor_state);
>  }
>
> +static int decode_gain_control(SingleChannelElement * sce, GetBitContext * gb)
> +{
> +    // wd_num, wd_test, aloc_size
> +    static const int gain_mode[4][3] = {

I would make this uint8_t

> +        {1, 0, 5},  // ONLY_LONG_SEQUENCE = 0,
> +        {2, 1, 2},  // LONG_START_SEQUENCE,
> +        {8, 0, 2},  // EIGHT_SHORT_SEQUENCE,
> +        {2, 1, 5},  // LONG_STOP_SEQUENCE
> +    };
> +
> +    // per-element gain control for SSR
> +    struct {
> +      int max_band;
> +      int adjust_num[4][8];
> +      int alev[4][8][8];
> +      int aloc[4][8][8];
> +    } ssr;

I would drop this stack struck and replace the leaf get_bits() with
skip bits. It makes the code that much harder to exploit and there is
no point in storing data we don't plan on decoding

> +
> +    const int mode = sce->ics.window_sequence[0];
> +    int bd, wd, ad;
> +
> +    // FIXME: Store the gain control data on |sce| and do something with it.
> +    ssr.max_band = get_bits(gb, 2);
> +    for (bd = 0; bd < ssr.max_band; bd++) {
> +        for (wd = 0; wd < gain_mode[mode][0]; wd++) {
> +            ssr.adjust_num[bd][wd] = get_bits(gb, 3);
> +            for (ad = 0; ad < ssr.adjust_num[bd][wd]; ad++) {
> +                ssr.alev[bd][wd][ad] = get_bits(gb, 4);
> +                if (wd == 0 && gain_mode[mode][1])
> +                    ssr.aloc[bd][wd][ad] = get_bits(gb, 4);
> +                else
> +                    ssr.aloc[bd][wd][ad] = get_bits(gb, gain_mode[mode][2]);
> +            }
> +        }
> +    }
> +    return 0;
> +}
> +
>  /**
>   * Decode an individual_channel_stream payload; reference: table 4.44.
>   *
> @@ -2034,9 +2073,13 @@ static int decode_ics(AACContext *ac, SingleChannelElement *sce,
>                  goto fail;
>          }
>          if (!eld_syntax && get_bits1(gb)) {
> -            avpriv_request_sample(ac->avctx, "SSR");
> -            ret = AVERROR_PATCHWELCOME;
> -            goto fail;
> +            // FIXME: Do something with the gain control data...
> +            ret = decode_gain_control(sce, gb);
> +            if (ret < 0) {
> +                ret = AVERROR_PATCHWELCOME;
> +                avpriv_request_sample(ac->avctx, "SSR");
> +                goto fail;
> +            }

This block seems funny. decode_gain_control() always returns zero.
Maybe make this warn once per stream when present like some of the
other AAC warn
cases.

>          }
>          // I see no textual basis in the spec for this occurring after SSR gain
>          // control, but this is what both reference and real implmentations do
> --
> 2.16.1.291.g4437f3f132-goog
>