[FFmpeg-devel] [RFC PATCH 1/3] aacdec: always skip the first 2048 samples if there's no side data

[FFmpeg-devel] [RFC PATCH 1/3] aacdec: always skip the first 2048 samples if there's no side data

[Lynne]

[-- Attachment #1: Type: text/plain, Size: 537 bytes --]

For some reason, this was never set, which meant all **raw** AAC in ADTS
streams, except faac, had extra samples at the start.

Despite this being a standard MDCT-based codec with a frame size of 1024,
hence a delay of 1024 samples at the start, all major encoders, excluding
faac and FFmpeg, use 2048 samples of padding.

The FFmpeg encoder will be modified to also output 2048 samples of padding
at the start, to make it in line with other encoders.

Yes, this leaves FATE pretty sad. Will fix it with the real version of the patch.



[-- Attachment #2: 0001-aacdec-always-skip-the-first-2048-samples-if-there-s.patch --]
[-- Type: text/x-diff, Size: 2218 bytes --]

From 079235e1f1a9caeadfd2b8d78b3fe2273d86018a Mon Sep 17 00:00:00 2001
From: Lynne <dev@lynne.ee>
Date: Fri, 11 Aug 2023 17:50:54 +0200
Subject: [PATCH 1/3] aacdec: always skip the first 2048 samples if there's no
 side data

For some reason, this was never set, which meant all **raw** AAC in ADTS
streams, except faac, had extra samples at the start.

Despite this being a standard MDCT-based codec with a frame size of 1024,
hence a delay of 1024 samples at the start, all major encoders, excluding
faac and FFmpeg, use 2048 samples of padding.

The FFmpeg encoder will be modified to also output 2048 samples of padding
at the start, to make it in line with other encoders.
---
 libavcodec/aacdec_template.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
index f8039e490b..0e4a274fea 100644
--- a/libavcodec/aacdec_template.c
+++ b/libavcodec/aacdec_template.c
@@ -1273,6 +1273,9 @@ static av_cold int aac_decode_init(AVCodecContext *avctx)
     if (ret < 0)
         return ret;
 
+    /* Usually overridden by side data */
+    avctx->internal->skip_samples = 2048;
+
     return 0;
 }
 
@@ -2417,14 +2420,16 @@ static int decode_dynamic_range(DynamicRangeControl *che_drc,
     return n;
 }
 
-static int decode_fill(AACContext *ac, GetBitContext *gb, int len) {
+static int decode_fill(AACContext *ac, GetBitContext *gb, int len)
+{
     uint8_t buf[256];
-    int i, major, minor;
+    int i, major, minor, micro;
 
     if (len < 13+7*8)
         goto unknown;
 
-    get_bits(gb, 13); len -= 13;
+    get_bits(gb, 13);
+    len -= 13;
 
     for(i=0; i+1<sizeof(buf) && len>=8; i++, len-=8)
         buf[i] = get_bits(gb, 8);
@@ -2434,7 +2439,11 @@ static int decode_fill(AACContext *ac, GetBitContext *gb, int len) {
         av_log(ac->avctx, AV_LOG_DEBUG, "FILL:%s\n", buf);
 
     if (sscanf(buf, "libfaac %d.%d", &major, &minor) == 2){
-        ac->avctx->internal->skip_samples = 1024;
+        ac->avctx->internal->skip_samples -= 1024;
+    }
+
+    if ((sscanf(buf, "avc %d.%d.%d", &major, &minor, &micro) == 3)) {
+        ac->avctx->internal->skip_samples -= 1024;
     }
 
 unknown:
-- 
2.40.1


[-- Attachment #3: 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".

[FFmpeg-devel] [RFC PATCH 2/3] decode: allow decoders to override skip_samples

[Lynne]

[-- Attachment #1: Type: text/plain, Size: 55 bytes --]

Very hacky. There must be a better way of doing this.


[-- Attachment #2: 0002-decode-allow-decoders-to-override-skip_samples.patch --]
[-- Type: text/x-diff, Size: 1533 bytes --]

From 843809ac072bbaf9ae0d3d3946723f1fcfb07923 Mon Sep 17 00:00:00 2001
From: Lynne <dev@lynne.ee>
Date: Tue, 12 Sep 2023 08:00:02 +0200
Subject: [PATCH 2/3] decode: allow decoders to override skip_samples

---
 libavcodec/decode.c   | 5 ++++-
 libavcodec/internal.h | 5 +++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index 169ee79acd..9e8a4532f2 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -312,7 +312,10 @@ static int discard_samples(AVCodecContext *avctx, AVFrame *frame, int64_t *disca
 
     side = av_frame_get_side_data(frame, AV_FRAME_DATA_SKIP_SAMPLES);
     if (side && side->size >= 10) {
-        avci->skip_samples = AV_RL32(side->data);
+        if (avci->skip_samples_add)
+            avci->skip_samples += AV_RL32(side->data);
+        else
+            avci->skip_samples = AV_RL32(side->data);
         avci->skip_samples = FFMAX(0, avci->skip_samples);
         discard_padding = AV_RL32(side->data + 4);
         av_log(avctx, AV_LOG_DEBUG, "skip %d / discard %d samples due to side data\n",
diff --git a/libavcodec/internal.h b/libavcodec/internal.h
index 83e0bc3fb2..8f59a117ba 100644
--- a/libavcodec/internal.h
+++ b/libavcodec/internal.h
@@ -118,6 +118,11 @@ typedef struct AVCodecInternal {
      */
     int skip_samples;
 
+    /**
+     * In case there's side data, add it to skip_samples, instead of overriding it.
+     */
+    int skip_samples_add;
+
     /**
      * hwaccel-specific private data
      */
-- 
2.40.1


[-- Attachment #3: 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".

[FFmpeg-devel] [RFC PATCH 3/3] aacdec: allow to skip sbr start-up delay

[Lynne]

[-- Attachment #1: Type: text/plain, Size: 288 bytes --]

As it happens, there's no standard between startup delay for SBR between
decoders either. libfdkaac uses 5056 samples, but Apple's encoder (via afconvert)
uses 3136.

Currently, this only fixes libfdk-aac. Would like to have more samples from more
encoders so I can fix all known cases.


[-- Attachment #2: 0001-aacdec-always-skip-the-first-2048-samples-if-there-s.patch --]
[-- Type: text/x-diff, Size: 2218 bytes --]

From 079235e1f1a9caeadfd2b8d78b3fe2273d86018a Mon Sep 17 00:00:00 2001
From: Lynne <dev@lynne.ee>
Date: Fri, 11 Aug 2023 17:50:54 +0200
Subject: [PATCH 1/3] aacdec: always skip the first 2048 samples if there's no
 side data

For some reason, this was never set, which meant all **raw** AAC in ADTS
streams, except faac, had extra samples at the start.

Despite this being a standard MDCT-based codec with a frame size of 1024,
hence a delay of 1024 samples at the start, all major encoders, excluding
faac and FFmpeg, use 2048 samples of padding.

The FFmpeg encoder will be modified to also output 2048 samples of padding
at the start, to make it in line with other encoders.
---
 libavcodec/aacdec_template.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
index f8039e490b..0e4a274fea 100644
--- a/libavcodec/aacdec_template.c
+++ b/libavcodec/aacdec_template.c
@@ -1273,6 +1273,9 @@ static av_cold int aac_decode_init(AVCodecContext *avctx)
     if (ret < 0)
         return ret;
 
+    /* Usually overridden by side data */
+    avctx->internal->skip_samples = 2048;
+
     return 0;
 }
 
@@ -2417,14 +2420,16 @@ static int decode_dynamic_range(DynamicRangeControl *che_drc,
     return n;
 }
 
-static int decode_fill(AACContext *ac, GetBitContext *gb, int len) {
+static int decode_fill(AACContext *ac, GetBitContext *gb, int len)
+{
     uint8_t buf[256];
-    int i, major, minor;
+    int i, major, minor, micro;
 
     if (len < 13+7*8)
         goto unknown;
 
-    get_bits(gb, 13); len -= 13;
+    get_bits(gb, 13);
+    len -= 13;
 
     for(i=0; i+1<sizeof(buf) && len>=8; i++, len-=8)
         buf[i] = get_bits(gb, 8);
@@ -2434,7 +2439,11 @@ static int decode_fill(AACContext *ac, GetBitContext *gb, int len) {
         av_log(ac->avctx, AV_LOG_DEBUG, "FILL:%s\n", buf);
 
     if (sscanf(buf, "libfaac %d.%d", &major, &minor) == 2){
-        ac->avctx->internal->skip_samples = 1024;
+        ac->avctx->internal->skip_samples -= 1024;
+    }
+
+    if ((sscanf(buf, "avc %d.%d.%d", &major, &minor, &micro) == 3)) {
+        ac->avctx->internal->skip_samples -= 1024;
     }
 
 unknown:
-- 
2.40.1


[-- Attachment #3: 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".

Re: [FFmpeg-devel] [RFC PATCH 3/3] aacdec: allow to skip sbr start-up delay

[Lynne]

[-- Attachment #1: Type: text/plain, Size: 362 bytes --]

Sep 12, 2023, 08:14 by dev@lynne.ee:

> As it happens, there's no standard between startup delay for SBR between
> decoders either. libfdkaac uses 5056 samples, but Apple's encoder (via afconvert)
> uses 3136.
>
> Currently, this only fixes libfdk-aac. Would like to have more samples from more
> encoders so I can fix all known cases.
>

Wrong patch attached.


[-- Attachment #2: 0003-aacdec-allow-to-skip-sbr-start-up-delay.patch --]
[-- Type: text/x-diff, Size: 2042 bytes --]

From c0e5659e5a2d56e883f9e1bb8c6d5bca1059721a Mon Sep 17 00:00:00 2001
From: Lynne <dev@lynne.ee>
Date: Tue, 12 Sep 2023 08:06:40 +0200
Subject: [PATCH 3/3] aacdec: allow to skip sbr start-up delay

---
 libavcodec/aac.h             |  1 +
 libavcodec/aacdec_template.c | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/libavcodec/aac.h b/libavcodec/aac.h
index 285d3b7482..3f67f353b7 100644
--- a/libavcodec/aac.h
+++ b/libavcodec/aac.h
@@ -298,6 +298,7 @@ struct AACContext {
     AVCodecContext *avctx;
     AVFrame *frame;
 
+    int sbr_state_changed;
     int is_saved;                 ///< Set if elements have stored overlap from previous frame.
     DynamicRangeControl che_drc;
 
diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
index 0e4a274fea..b6c6d19f61 100644
--- a/libavcodec/aacdec_template.c
+++ b/libavcodec/aacdec_template.c
@@ -1207,6 +1207,8 @@ static av_cold int aac_decode_init(AVCodecContext *avctx)
                                                 avctx->extradata_size * 8LL,
                                                 1)) < 0)
             return ret;
+
+        ac->sbr_state_changed = ac->oc[1].m4ac.sbr == 1;
     } else {
         int sr, i;
         uint8_t layout_map[MAX_ELEM_ID*4][3];
@@ -3154,6 +3156,7 @@ static int aac_decode_frame_int(AVCodecContext *avctx, AVFrame *frame,
     int is_dmono, sce_count = 0;
     int payload_alignment;
     uint8_t che_presence[4][MAX_ELEM_ID] = {{0}};
+    int sbr_state_start = ac->oc[1].m4ac.sbr;
 
     ac->frame = frame;
 
@@ -3346,6 +3349,14 @@ static int aac_decode_frame_int(AVCodecContext *avctx, AVFrame *frame,
             frame->data[0] = frame->data[1];
     }
 
+    ac->sbr_state_changed |= (ac->oc[1].m4ac.sbr != sbr_state_start) && (ac->oc[1].m4ac.sbr == 1);
+
+    if (ac->sbr_state_changed) {
+        avctx->internal->skip_samples = 5056;
+        avctx->internal->skip_samples_add = 1;
+        ac->sbr_state_changed = 0;
+    }
+
     return 0;
 fail:
     pop_output_configuration(ac);
-- 
2.40.1


[-- Attachment #3: 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".

Re: [FFmpeg-devel] [RFC PATCH 1/3] aacdec: always skip the first 2048 samples if there's no side data

[Andreas Rheinhardt]

Lynne:
> For some reason, this was never set, which meant all **raw** AAC in ADTS
> streams, except faac, had extra samples at the start.
> 
> Despite this being a standard MDCT-based codec with a frame size of 1024,
> hence a delay of 1024 samples at the start, all major encoders, excluding
> faac and FFmpeg, use 2048 samples of padding.
> 
> The FFmpeg encoder will be modified to also output 2048 samples of padding
> at the start, to make it in line with other encoders.

Does this also have actual advantages besides "being in line with other
encoders"?

> 
> Yes, this leaves FATE pretty sad. Will fix it with the real version of the patch.
> 

Didn't we once guess the number of skip samples like this, only for this
guesswork to be removed intentionally? (This is not a rhetorical
question; I thought it to be true, but I see that there is still code
for faac in decode_fill(); maybe I misremember.)

- Andreas

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

Re: [FFmpeg-devel] [RFC PATCH 1/3] aacdec: always skip the first 2048 samples if there's no side data

[Lynne]

Sep 12, 2023, 09:43 by andreas.rheinhardt@outlook.com:

> Lynne:
>
>> For some reason, this was never set, which meant all **raw** AAC in ADTS
>> streams, except faac, had extra samples at the start.
>>
>> Despite this being a standard MDCT-based codec with a frame size of 1024,
>> hence a delay of 1024 samples at the start, all major encoders, excluding
>> faac and FFmpeg, use 2048 samples of padding.
>>
>> The FFmpeg encoder will be modified to also output 2048 samples of padding
>> at the start, to make it in line with other encoders.
>>
>
> Does this also have actual advantages besides "being in line with other
> encoders"?
>

Not really. I don't have an opinion on this. 1024 is the natural
delay of the codec, so maybe it would be best to leave it at that.


>> Yes, this leaves FATE pretty sad. Will fix it with the real version of the patch.
>>
>
> Didn't we once guess the number of skip samples like this, only for this
> guesswork to be removed intentionally? (This is not a rhetorical
> question; I thought it to be true, but I see that there is still code
> for faac in decode_fill(); maybe I misremember.)
>

I don't remember something like that. The faac workaround dates back
from 2012 (bfe735b5824c7d10ba42932a17d786db50e3b2d4), and it's only for faac.
It's less of a guess, as most encoders to use the FIL extension to signal
themselves.
_______________________________________________
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".

Re: [FFmpeg-devel] [RFC PATCH 1/3] aacdec: always skip the first 2048 samples if there's no side data

[Thierry Foucu]

On Tue, Sep 12, 2023 at 9:25 AM Lynne <dev@lynne.ee> wrote:

> Sep 12, 2023, 09:43 by andreas.rheinhardt@outlook.com:
>
> > Lynne:
> >
> >> For some reason, this was never set, which meant all **raw** AAC in ADTS
> >> streams, except faac, had extra samples at the start.
> >>
> >> Despite this being a standard MDCT-based codec with a frame size of
> 1024,
> >> hence a delay of 1024 samples at the start, all major encoders,
> excluding
> >> faac and FFmpeg, use 2048 samples of padding.
> >>
> >> The FFmpeg encoder will be modified to also output 2048 samples of
> padding
> >> at the start, to make it in line with other encoders.
> >>
> >
> > Does this also have actual advantages besides "being in line with other
> > encoders"?
> >
>
> Not really. I don't have an opinion on this. 1024 is the natural
> delay of the codec, so maybe it would be best to leave it at that.
>
>
> Note:
Not all encoders add 2048. Another version of the Fraunhofer encoder will
add only 1600 samples

and for HE-AAC of the same encoder will add 3200 samples.
Should we not then have an option to set it ?


> >> Yes, this leaves FATE pretty sad. Will fix it with the real version of
> the patch.
> >>
> >
> > Didn't we once guess the number of skip samples like this, only for this
> > guesswork to be removed intentionally? (This is not a rhetorical
> > question; I thought it to be true, but I see that there is still code
> > for faac in decode_fill(); maybe I misremember.)
> >
>
> I don't remember something like that. The faac workaround dates back
> from 2012 (bfe735b5824c7d10ba42932a17d786db50e3b2d4), and it's only for
> faac.
> It's less of a guess, as most encoders to use the FIL extension to signal
> themselves.
> _______________________________________________
> 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".
>


-- 

Thierry Foucu
_______________________________________________
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".

Re: [FFmpeg-devel] [RFC PATCH 1/3] aacdec: always skip the first 2048 samples if there's no side data

[Lynne]

Sep 12, 2023, 23:25 by tfoucu@gmail.com:

> On Tue, Sep 12, 2023 at 9:25 AM Lynne <dev@lynne.ee> wrote:
>
>> Sep 12, 2023, 09:43 by andreas.rheinhardt@outlook.com:
>>
>> > Lynne:
>> >
>> >> For some reason, this was never set, which meant all **raw** AAC in ADTS
>> >> streams, except faac, had extra samples at the start.
>> >>
>> >> Despite this being a standard MDCT-based codec with a frame size of
>> 1024,
>> >> hence a delay of 1024 samples at the start, all major encoders,
>> excluding
>> >> faac and FFmpeg, use 2048 samples of padding.
>> >>
>> >> The FFmpeg encoder will be modified to also output 2048 samples of
>> padding
>> >> at the start, to make it in line with other encoders.
>> >>
>> >
>> > Does this also have actual advantages besides "being in line with other
>> > encoders"?
>> >
>>
>> Not really. I don't have an opinion on this. 1024 is the natural
>> delay of the codec, so maybe it would be best to leave it at that.
>>
>>
>> Note:
>>
> Not all encoders add 2048. Another version of the Fraunhofer encoder will
> add only 1600 samples
>
> and for HE-AAC of the same encoder will add 3200 samples.
> Should we not then have an option to set it ?
>

That sounds reasonable, I've resent the patch.
Do you think it's reasonable to go 2048 samples?

It does cut off the default AAC decoder, in case the FIL extension
has been stripped.
_______________________________________________
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".