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

[Alex Converse]

On Tue, Feb 20, 2018 at 4:56 PM, Dale Curtis  wrote:
> On Tue, Feb 20, 2018 at 4:18 PM, Alex Converse 
> wrote:
>
>>
>> I would make this uint8_t
>>
>
> Done.
>
>
>> 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
>>
>
> Done.
>
>
>> 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.
>>
>
> Done. I've added an AACContext::warn_gain_control member to do this.
>
> This patch set also changes the attribution from Robert Swain to Maxim
> Gavrilov based on svn blame of the SoC repository after discussion at
> https://trac.ffmpeg.org/ticket/1693#comment:34
>

LGTM
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Dale Curtis]

On Tue, Feb 20, 2018 at 4:18 PM, Alex Converse 
wrote:

>
> I would make this uint8_t
>

Done.


> 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
>

Done.


> 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.
>

Done. I've added an AACContext::warn_gain_control member to do this.

This patch set also changes the attribution from Robert Swain to Maxim
Gavrilov based on svn blame of the SoC repository after discussion at
https://trac.ffmpeg.org/ticket/1693#comment:34
From 8e5a3cc04400a1186df0714524ccb24dbe3f627d Mon Sep 17 00:00:00 2001
From: Dale Curtis 
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 pulls in the decode_gain_control() function from the
ffmpeg summer-of-code repo (original author Maxim Gavrilov) at
svn://svn.mplayerhq.hu/soc/aac/aac.c with some minor modifications
and adds AOT_AAC_SSR to decode_audio_specific_config_gb().

Signed-off-by: Dale Curtis 
Co-authored-by: Maxim Gavrilov 
---
 libavcodec/aac.h |  2 ++
 libavcodec/aacdec_template.c | 36 +---
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/libavcodec/aac.h b/libavcodec/aac.h
index 4910c661d6..05bc95385f 100644
--- a/libavcodec/aac.h
+++ b/libavcodec/aac.h
@@ -357,6 +357,8 @@ struct AACContext {
 int warned_num_aac_frames;
 int warned_960_sbr;
 
+int warned_gain_control;
+
 /* aacdec functions pointers */
 void (*imdct_and_windowing)(AACContext *ac, SingleChannelElement *sce);
 void (*apply_ltp)(AACContext *ac, SingleChannelElement *sce);
diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
index c2d9802023..cf97181092 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,33 @@ static void apply_prediction(AACContext *ac, SingleChannelElement *sce)
 reset_all_predictors(sce->predictor_state);
 }
 
+static void decode_gain_control(SingleChannelElement * sce, GetBitContext * gb)
+{
+// wd_num, wd_test, aloc_size
+static const uint8_t gain_mode[4][3] = {
+{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
+};
+
+const int mode = sce->ics.window_sequence[0];
+uint8_t bd, wd, ad;
+
+// FIXME: Store the gain control data on |sce| and do something with it.
+uint8_t max_band = get_bits(gb, 2);
+for (bd = 0; bd < max_band; bd++) {
+for (wd = 0; wd < gain_mode[mode][0]; wd++) {
+uint8_t adjust_num = get_bits(gb, 3);
+for (ad = 0; ad < adjust_num; ad++) {
+skip_bits(gb, 4 + ((wd == 0 && gain_mode[mode][1])
+ ? 4
+ : gain_mode[mode][2]));
+}
+}
+}
+}
+
 /**
  * Decode an individual_channel_stream payload; reference: table 4.44.
  *
@@ -2034,9 +2062,11 @@ 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;
+decode_gain_control(sce, gb);
+if (!ac->warned_gain_control) {
+avpriv_report_missing_feature(ac->avctx, "Gain control");
+ac->warned_gain_control = 1;
+}
 }
 // 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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [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  wrote:
> From 97380752ef12337d9b9a01801a9a84df3b4b9c0a Mon Sep 17 00:00:00 2001
> From: Dale Curtis 
> 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 
> Co-authored-by: Robert Swain 
> ---
>  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
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Dale Curtis]

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 
Co-authored-by: Robert Swain 
From 97380752ef12337d9b9a01801a9a84df3b4b9c0a Mon Sep 17 00:00:00 2001
From: Dale Curtis 
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 
Co-authored-by: Robert Swain 
---
 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] = {
+{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;
+
+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;
+}
 }
 // 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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel