[FFmpeg-devel] [PATCH v2 06/10] avcodec/aptx: Use AVCodecContext.frame_size according to the API

[Andreas Rheinhardt]

Hendrik Leppkes:
> On Tue, Aug 31, 2021 at 2:44 PM Andreas Rheinhardt
> <andreas.rheinhardt at outlook.com> wrote:
>>
>> Currently the APTX (HD) codecs set frame_size if unset and check
>> whether it is divisible by block_size (corresponding to block_align
>> as used by other codecs). But this is based upon a misunderstanding
>> of the API: frame_size is not in bytes, but in samples.*
>>
>> Said value is also not intended to be set by the user at all,
>> but set by encoders and (possibly) decoders if the number of channels
>> in a frame is constant. The latter condition is not fulfilled here,
>> so only set it for encoders to the value that it already had for APTX:
>> 1024 samples (per channel).
>>
>> *: If it were needed to check said value, one would need to check
>> for it to be divisible by four (four samples correspond to one block
>> of block_size bytes).
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
>> ---
>>  libavcodec/aptx.c | 10 ++--------
>>  1 file changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/libavcodec/aptx.c b/libavcodec/aptx.c
>> index 3aeee1907c..97397aca68 100644
>> --- a/libavcodec/aptx.c
>> +++ b/libavcodec/aptx.c
>> @@ -515,14 +515,8 @@ av_cold int ff_aptx_init(AVCodecContext *avctx)
>>      s->hd = avctx->codec->id == AV_CODEC_ID_APTX_HD;
>>      s->block_size = s->hd ? 6 : 4;
>>
>> -    if (avctx->frame_size == 0)
>> -        avctx->frame_size = 256 * s->block_size;
>> -
>> -    if (avctx->frame_size % s->block_size) {
>> -        av_log(avctx, AV_LOG_ERROR,
>> -               "Frame size must be a multiple of %d samples\n", s->block_size);
>> -        return AVERROR(EINVAL);
>> -    }
>> +    if (av_codec_is_encoder(avctx->codec))
>> +        avctx->frame_size = 1024;
>>
> 
> s->block_size can be 6 for HD or 4 for non-HD, making the default
> frame_size not always 1024 before this change, thus a change in
> behavior for aptx HD. Is this intended?

I believe that it was originally intended to encode frames of 256
samples for both variants. In order to minimize changes I just set it to
1024 instead of 256. Actually, RFC7310 [1] says that the default
packetization interval is 4ms which at 48kHz are just 192 samples, so
choosing the lower of the two numbers seemed sensible.
(It would be nice if the user were allowed to always send any multiple
of four samples to the encoder and if the last frame would be
automatically padded to four samples just as it is padded to frame_size
now for the encoders without the SMALL_LAST_FRAME cap. Maybe adding a
new codec cap for this made sense?)

- Andreas

[1]: https://datatracker.ietf.org/doc/html/rfc7310#section-5.3