diff mbox series

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

Message ID AM7PR03MB66605790D14C8FD9CD1FFEDA8FCC9@AM7PR03MB6660.eurprd03.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel,v2,01/10] tests/fate-run: Allow multiple inputs for transcode()
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Andreas Rheinhardt Aug. 31, 2021, 12:42 p.m. UTC
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@outlook.com>
---
 libavcodec/aptx.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

Comments

Hendrik Leppkes Aug. 31, 2021, 2:22 p.m. UTC | #1
On Tue, Aug 31, 2021 at 2:44 PM Andreas Rheinhardt
<andreas.rheinhardt@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@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?

- Hendrik
Hendrik Leppkes Aug. 31, 2021, 2:25 p.m. UTC | #2
On Tue, Aug 31, 2021 at 4:22 PM Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>
> On Tue, Aug 31, 2021 at 2:44 PM Andreas Rheinhardt
> <andreas.rheinhardt@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@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?
>

Nevermind, I now figured out what your commit message means. It wasn't
extremely clear.

- Hendrik
Andreas Rheinhardt Aug. 31, 2021, 2:45 p.m. UTC | #3
Hendrik Leppkes:
> On Tue, Aug 31, 2021 at 2:44 PM Andreas Rheinhardt
> <andreas.rheinhardt@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@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
Andreas Rheinhardt Aug. 31, 2021, 2:49 p.m. UTC | #4
Hendrik Leppkes:
> On Tue, Aug 31, 2021 at 4:22 PM Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>>
>> On Tue, Aug 31, 2021 at 2:44 PM Andreas Rheinhardt
>> <andreas.rheinhardt@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@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?
>>
> 
> Nevermind, I now figured out what your commit message means. It wasn't
> extremely clear.
> 
I reread it and still don't get what is unclear. Could you elaborate (or
even suggest changes)?

- Andreas
Paul B Mahol Sept. 1, 2021, 6:34 a.m. UTC | #5
lgtm
diff mbox series

Patch

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;
 
     for (chan = 0; chan < NB_CHANNELS; chan++) {
         Channel *channel = &s->channels[chan];