[FFmpeg-devel] [PATCH 1/2] lavc/internal: add skip_samples2 field

[FFmpeg-devel] [PATCH 1/2] lavc/internal: add skip_samples2 field

[Lynne]

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

The issue is that avci->skip_samples will be overridden by any side-data.

When operating on raw files (adts, for example), the decoder is free
to decide the amount of samples to skip. Usually, this is the algorithmic
delay of the decoder.

When operating on more complete containers, like ISOBMFF, the amount of
samples to be skipped is recorded and signalled by the encoder.

However, it turns out many encoders have an arbitrary choice of padding
to insert at the start. Normally, they would signal the amount into
the container. But with ISOBMFF, there isn't just a single option -
the format has been extended multiple times, and has multiple ways
to signal padding. In the case of fdkaac-encoded samples, the STTS
is used, rather than the CTTS, which ends up with us leaving the padding
in.

But it's not just containers, as it turns out, most AAC encoders use
an arbitrary amount of padding at the start that may, or may not be
trimmed (usually, it won't be).

Furthermore, AAC has specific amount of algorithmic delay for SBR
operation. This delay is not accounter for anywhere. While it's an
option to skip the samples in the decoder, doing this in decode.c,
along with the rest of the skip adjustments, is a neater way, and
can be extended to other codecs.

Patch attached.


[-- Attachment #2: 0001-lavc-internal-add-skip_samples2-field.patch --]
[-- Type: text/x-diff, Size: 2723 bytes --]

From 9986c7f0c71d944101f1c7fe7b1395ee21e34a8e Mon Sep 17 00:00:00 2001
From: Lynne <dev@lynne.ee>
Date: Thu, 19 Oct 2023 04:28:03 +0200
Subject: [PATCH 1/2] lavc/internal: add skip_samples2 field

The issue is that avci->skip_samples will be overridden by any side-data.

When operating on raw files (adts, for example), the decoder is free
to decide the amount of samples to skip. Usually, this is the algorithmic
delay of the decoder.

When operating on more complete containers, like ISOBMFF, the amount of
samples to be skipped is recorded and signalled by the encoder.

However, it turns out many encoders have an arbitrary choice of padding
to insert at the start. Normally, they would signal the amount into
the container. But with ISOBMFF, there isn't just a single option -
the format has been extended multiple times, and has multiple ways
to signal padding. In the case of fdkaac-encoded samples, the STTS
is used, rather than the CTTS, which ends up with us leaving the padding
in.

But it's not just containers, as it turns out, most AAC encoders use
an arbitrary amount of padding at the start that may, or may not be
trimmed (usually, it won't be).

Furthermore, AAC has specific amount of algorithmic delay for SBR
operation. This delay is not accounter for anywhere. While it's an
option to skip the samples in the decoder, doing this in decode.c,
along with the rest of the skip adjustments, is a neater way, and
can be extended to other codecs.
---
 libavcodec/decode.c   | 2 ++
 libavcodec/internal.h | 9 +++++++++
 2 files changed, 11 insertions(+)

diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index ad39021354..32944a6b6a 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -338,6 +338,8 @@ static int discard_samples(AVCodecContext *avctx, AVFrame *frame, int64_t *disca
         return AVERROR(EAGAIN);
     }
 
+    avci->skip_samples += avci->skip_samples2;
+
     if (avci->skip_samples > 0) {
         if (frame->nb_samples <= avci->skip_samples){
             *discarded_samples += frame->nb_samples;
diff --git a/libavcodec/internal.h b/libavcodec/internal.h
index eb9e0d707c..3d8d4d9a4d 100644
--- a/libavcodec/internal.h
+++ b/libavcodec/internal.h
@@ -115,9 +115,18 @@ typedef struct AVCodecInternal {
 
     /**
      * Number of audio samples to skip at the start of the next decoded frame
+     *
+     * Note: This will be overridden by any side data.
      */
     int skip_samples;
 
+    /**
+     * Additional samples to skip ad the start of the next decoded frame.
+     *
+     * These will be added to any skip amount after taking side data into account.
+     */
+    int skip_samples2;
+
     /**
      * hwaccel-specific private data
      */
-- 
2.42.0


[-- 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] [PATCH 2/2] aacdec: padding skip improvements

[Lynne]

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

Change from the previous version: by default, the current behaviour
of the decoder is preserved. Samples to skip are zero by default.
We can improve this later.

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 different amounts of padding.

Patch attached.


[-- Attachment #2: 0001-aacdec-padding-skip-improvements.patch --]
[-- Type: text/x-diff, Size: 2962 bytes --]

From 691fdb2952707fffd6975ca7a555e198236e9168 Mon Sep 17 00:00:00 2001
From: Lynne <dev@lynne.ee>
Date: Tue, 3 Oct 2023 05:57:50 +0200
Subject: [PATCH] aacdec: padding skip improvements

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.
---
 libavcodec/aac.h             |  1 +
 libavcodec/aacdec_template.c | 20 +++++++++++++++-----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/libavcodec/aac.h b/libavcodec/aac.h
index 285d3b7482..79bbb3cce5 100644
--- a/libavcodec/aac.h
+++ b/libavcodec/aac.h
@@ -298,6 +298,7 @@ struct AACContext {
     AVCodecContext *avctx;
     AVFrame *frame;
 
+    int override_padding;
     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 954399f86b..a5b46b09e3 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 = ac->override_padding;
+
     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) {
-    uint8_t buf[256];
-    int i, major, minor;
+static int decode_fill(AACContext *ac, GetBitContext *gb, int len)
+{
+    uint8_t buf[256] = { 0 };
+    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,9 @@ 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;
+    } else if ((sscanf(buf, "avc %d.%d.%d", &major, &minor, &micro) == 3)) {
+        ac->avctx->internal->skip_samples -= 1024;
     }
 
 unknown:
@@ -3471,6 +3478,9 @@ static const AVOption options[] = {
       { "coded",    "order in which the channels are coded in the bitstream",
         0, AV_OPT_TYPE_CONST, { .i64 = CHANNEL_ORDER_CODED }, .flags = AACDEC_FLAGS, "channel_order" },
 
+    { "padding", "Override the padding at the start of a stream.\n",
+        offsetof(AACContext, override_padding), AV_OPT_TYPE_INT, { .i64 = 1024 }, 1024, 8192, AACDEC_FLAGS },
+
     {NULL},
 };
 
-- 
2.42.0


[-- 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] [PATCH 2/2] aacdec: padding skip improvements

[Anton Khirnov]

Quoting Lynne (2023-10-19 04:38:51)
> @@ -3471,6 +3478,9 @@ static const AVOption options[] = {
>        { "coded",    "order in which the channels are coded in the bitstream",
>          0, AV_OPT_TYPE_CONST, { .i64 = CHANNEL_ORDER_CODED }, .flags = AACDEC_FLAGS, "channel_order" },
>  
> +    { "padding", "Override the padding at the start of a stream.\n",
> +        offsetof(AACContext, override_padding), AV_OPT_TYPE_INT, { .i64 = 1024 }, 1024, 8192, AACDEC_FLAGS },

Why should this be a decoder option when the caller can already control
it by inserting AV_PKT_DATA_SKIP_SAMPLES?

-- 
Anton Khirnov
_______________________________________________
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] [PATCH 2/2] aacdec: padding skip improvements

[Lynne]

Oct 19, 2023, 10:40 by anton@khirnov.net:

> Quoting Lynne (2023-10-19 04:38:51)
>
>> @@ -3471,6 +3478,9 @@ static const AVOption options[] = {
>>  { "coded",    "order in which the channels are coded in the bitstream",
>>  0, AV_OPT_TYPE_CONST, { .i64 = CHANNEL_ORDER_CODED }, .flags = AACDEC_FLAGS, "channel_order" },
>>  
>> +    { "padding", "Override the padding at the start of a stream.\n",
>> +        offsetof(AACContext, override_padding), AV_OPT_TYPE_INT, { .i64 = 1024 }, 1024, 8192, AACDEC_FLAGS },
>>
>
> Why should this be a decoder option when the caller can already control
> it by inserting AV_PKT_DATA_SKIP_SAMPLES?
>

CLI, sadly, mainly.
_______________________________________________
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] [PATCH 2/2] aacdec: padding skip improvements

[Anton Khirnov]

Quoting Lynne (2023-10-19 14:48:06)
> Oct 19, 2023, 10:40 by anton@khirnov.net:
> 
> > Quoting Lynne (2023-10-19 04:38:51)
> >
> >> @@ -3471,6 +3478,9 @@ static const AVOption options[] = {
> >>  { "coded",    "order in which the channels are coded in the bitstream",
> >>  0, AV_OPT_TYPE_CONST, { .i64 = CHANNEL_ORDER_CODED }, .flags = AACDEC_FLAGS, "channel_order" },
> >>  
> >> +    { "padding", "Override the padding at the start of a stream.\n",
> >> +        offsetof(AACContext, override_padding), AV_OPT_TYPE_INT, { .i64 = 1024 }, 1024, 8192, AACDEC_FLAGS },
> >>
> >
> > Why should this be a decoder option when the caller can already control
> > it by inserting AV_PKT_DATA_SKIP_SAMPLES?
> >
> 
> CLI, sadly, mainly.

So add a CLI option to override this side data along the lines of
display_rotation.

-- 
Anton Khirnov
_______________________________________________
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] [PATCH 2/2] aacdec: padding skip improvements

[Lynne]

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

Correct patch attached.


[-- Attachment #2: 0002-aacdec-padding-skip-improvements.patch --]
[-- Type: text/x-diff, Size: 2966 bytes --]

From 91ff271acffdb00bb8a7ace0ae1e811770104fc2 Mon Sep 17 00:00:00 2001
From: Lynne <dev@lynne.ee>
Date: Thu, 19 Oct 2023 04:36:12 +0200
Subject: [PATCH 2/2] aacdec: padding skip improvements

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 different amounts of padding.
---
 libavcodec/aac.h             |  1 +
 libavcodec/aacdec_template.c | 20 +++++++++++++++-----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/libavcodec/aac.h b/libavcodec/aac.h
index 285d3b7482..79bbb3cce5 100644
--- a/libavcodec/aac.h
+++ b/libavcodec/aac.h
@@ -298,6 +298,7 @@ struct AACContext {
     AVCodecContext *avctx;
     AVFrame *frame;
 
+    int override_padding;
     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 954399f86b..d5855a4f98 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_samples2 = ac->override_padding;
+
     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) {
-    uint8_t buf[256];
+static int decode_fill(AACContext *ac, GetBitContext *gb, int len)
+{
+    uint8_t buf[256] = { 0 };
     int i, major, minor;
 
     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);
@@ -2433,8 +2438,10 @@ static int decode_fill(AACContext *ac, GetBitContext *gb, int len) {
     if (ac->avctx->debug & FF_DEBUG_PICT_INFO)
         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;
+    if (!ac->override_padding) {
+        if (sscanf(buf, "libfaac %d.%d", &major, &minor) == 2){
+            ac->avctx->internal->skip_samples = 1024;
+        }
     }
 
 unknown:
@@ -3471,6 +3478,9 @@ static const AVOption options[] = {
       { "coded",    "order in which the channels are coded in the bitstream",
         0, AV_OPT_TYPE_CONST, { .i64 = CHANNEL_ORDER_CODED }, .flags = AACDEC_FLAGS, "channel_order" },
 
+    { "padding", "Override the padding at the start of a stream.\n",
+        offsetof(AACContext, override_padding), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 8192, AACDEC_FLAGS },
+
     {NULL},
 };
 
-- 
2.42.0


[-- 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] [PATCH 1/2] lavc/internal: add skip_samples2 field

[Anton Khirnov]

Current interaction between AV_FRAME_DATA_SKIP_SAMPLES and
AVCodecContext.skip_samples seems unncecessarily complicated to me and
you're just making it worse.

Is there any reason we can't drop AVCodecContext.skip_samples entirely
and signal it purely through side data? Then decoders could fully
control everything they wish by modifying side data on output frames.

-- 
Anton Khirnov
_______________________________________________
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] [PATCH 1/2] lavc/internal: add skip_samples2 field

[Lynne]

Oct 19, 2023, 10:39 by anton@khirnov.net:

> Current interaction between AV_FRAME_DATA_SKIP_SAMPLES and
> AVCodecContext.skip_samples seems unncecessarily complicated to me and
> you're just making it worse.
>
> Is there any reason we can't drop AVCodecContext.skip_samples entirely
> and signal it purely through side data? Then decoders could fully
> control everything they wish by modifying side data on output frames.
>

You mean let the decoder parse skip samples side data,
strip it from the packet, and attach a new side data to the frame?
_______________________________________________
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] [PATCH 1/2] lavc/internal: add skip_samples2 field

[Anton Khirnov]

Quoting Lynne (2023-10-19 14:49:46)
> Oct 19, 2023, 10:39 by anton@khirnov.net:
> 
> > Current interaction between AV_FRAME_DATA_SKIP_SAMPLES and
> > AVCodecContext.skip_samples seems unncecessarily complicated to me and
> > you're just making it worse.
> >
> > Is there any reason we can't drop AVCodecContext.skip_samples entirely
> > and signal it purely through side data? Then decoders could fully
> > control everything they wish by modifying side data on output frames.
> >
> 
> You mean let the decoder parse skip samples side data,
> strip it from the packet, and attach a new side data to the frame?

Not from the packet - that should be const for decoders. The generic
code currently translates AV_PKT_DATA_SKIP_SAMPLES from the packet to
the frame in ff_get_buffer(). The decoder can then override that in the
frame.

-- 
Anton Khirnov
_______________________________________________
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] [PATCH 1/2] lavc/internal: add skip_samples2 field

[James Almer]

On 10/19/2023 5:39 AM, Anton Khirnov wrote:
> Current interaction between AV_FRAME_DATA_SKIP_SAMPLES and
> AVCodecContext.skip_samples seems unncecessarily complicated to me and
> you're just making it worse.
> 
> Is there any reason we can't drop AVCodecContext.skip_samples entirely
> and signal it purely through side data? Then decoders could fully
> control everything they wish by modifying side data on output frames.

Given that now coded_side_data is fully implemented for decoding and 
encoding, including getting elements from the container, it's probably a 
good idea to do that.
_______________________________________________
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".