| 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() | expand |
| 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 |
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
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
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
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
lgtm
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];
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(-)