| Message ID | 20240404162936.4581-2-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avcodec/liblc3dec: sanitize channel count in avctx | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On date Thursday 2024-04-04 13:29:36 -0300, James Almer wrote: > We only care about channel count. Layout details will be ignored either way. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/liblc3enc.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/libavcodec/liblc3enc.c b/libavcodec/liblc3enc.c > index 63d1645b10..5f8169a0cf 100644 > --- a/libavcodec/liblc3enc.c > +++ b/libavcodec/liblc3enc.c > @@ -61,6 +61,11 @@ static av_cold int liblc3_encode_init(AVCodecContext *avctx) > "Unsupported frame duration %.1f ms.\n", frame_us / 1000.f); > return AVERROR(EINVAL); > } > + if (channels < 0 || channels > ENCODER_MAX_CHANNELS) { > + av_log(avctx, AV_LOG_ERROR, > + "Unsupported channel count %d. Should be 1 or 2\n", channels); > + return AVERROR(EINVAL); > + } > > hr_mode |= srate_hz > 48000; > hr_mode &= srate_hz >= 48000; > @@ -195,9 +200,6 @@ const FFCodec ff_liblc3_encoder = { > .p.type = AVMEDIA_TYPE_AUDIO, > .p.id = AV_CODEC_ID_LC3, > .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY, > - .p.ch_layouts = (const AVChannelLayout[]) > - { { AV_CHANNEL_ORDER_UNSPEC, 1 }, > - { AV_CHANNEL_ORDER_UNSPEC, 2 }, { 0 } }, shouldn't this be equivalent? Should be good anyway.
On 4/4/2024 1:45 PM, Stefano Sabatini wrote: > On date Thursday 2024-04-04 13:29:36 -0300, James Almer wrote: >> We only care about channel count. Layout details will be ignored either way. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/liblc3enc.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/libavcodec/liblc3enc.c b/libavcodec/liblc3enc.c >> index 63d1645b10..5f8169a0cf 100644 >> --- a/libavcodec/liblc3enc.c >> +++ b/libavcodec/liblc3enc.c >> @@ -61,6 +61,11 @@ static av_cold int liblc3_encode_init(AVCodecContext *avctx) >> "Unsupported frame duration %.1f ms.\n", frame_us / 1000.f); >> return AVERROR(EINVAL); >> } >> + if (channels < 0 || channels > ENCODER_MAX_CHANNELS) { >> + av_log(avctx, AV_LOG_ERROR, >> + "Unsupported channel count %d. Should be 1 or 2\n", channels); >> + return AVERROR(EINVAL); >> + } >> >> hr_mode |= srate_hz > 48000; >> hr_mode &= srate_hz >= 48000; >> @@ -195,9 +200,6 @@ const FFCodec ff_liblc3_encoder = { >> .p.type = AVMEDIA_TYPE_AUDIO, >> .p.id = AV_CODEC_ID_LC3, >> .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY, > >> - .p.ch_layouts = (const AVChannelLayout[]) >> - { { AV_CHANNEL_ORDER_UNSPEC, 1 }, >> - { AV_CHANNEL_ORDER_UNSPEC, 2 }, { 0 } }, > > shouldn't this be equivalent? > > Should be good anyway. No, because if you pass it a mono or stereo layout, the generic encode code will reject it, whereas after this change it will work because the encoder will only cares about channel count, not overall layout.
James Almer: > We only care about channel count. Layout details will be ignored either way. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/liblc3enc.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/libavcodec/liblc3enc.c b/libavcodec/liblc3enc.c > index 63d1645b10..5f8169a0cf 100644 > --- a/libavcodec/liblc3enc.c > +++ b/libavcodec/liblc3enc.c > @@ -61,6 +61,11 @@ static av_cold int liblc3_encode_init(AVCodecContext *avctx) > "Unsupported frame duration %.1f ms.\n", frame_us / 1000.f); > return AVERROR(EINVAL); > } > + if (channels < 0 || channels > ENCODER_MAX_CHANNELS) { > + av_log(avctx, AV_LOG_ERROR, > + "Unsupported channel count %d. Should be 1 or 2\n", channels); > + return AVERROR(EINVAL); > + } > > hr_mode |= srate_hz > 48000; > hr_mode &= srate_hz >= 48000; > @@ -195,9 +200,6 @@ const FFCodec ff_liblc3_encoder = { > .p.type = AVMEDIA_TYPE_AUDIO, > .p.id = AV_CODEC_ID_LC3, > .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY, > - .p.ch_layouts = (const AVChannelLayout[]) > - { { AV_CHANNEL_ORDER_UNSPEC, 1 }, > - { AV_CHANNEL_ORDER_UNSPEC, 2 }, { 0 } }, > .p.supported_samplerates = (const int []) > { 96000, 48000, 32000, 24000, 16000, 8000, 0 }, > .p.sample_fmts = (const enum AVSampleFormat[]) I think we should rather change encode.c to mean that an UNSPEC channel layout (in AVCodec.ch_layouts) works with every channel layout with the same number of channels. - Andreas
On date Thursday 2024-04-04 18:59:09 +0200, Andreas Rheinhardt wrote: > James Almer: > > We only care about channel count. Layout details will be ignored either way. > > > > Signed-off-by: James Almer <jamrial@gmail.com> > > --- > > libavcodec/liblc3enc.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/libavcodec/liblc3enc.c b/libavcodec/liblc3enc.c > > index 63d1645b10..5f8169a0cf 100644 > > --- a/libavcodec/liblc3enc.c > > +++ b/libavcodec/liblc3enc.c > > @@ -61,6 +61,11 @@ static av_cold int liblc3_encode_init(AVCodecContext *avctx) > > "Unsupported frame duration %.1f ms.\n", frame_us / 1000.f); > > return AVERROR(EINVAL); > > } > > + if (channels < 0 || channels > ENCODER_MAX_CHANNELS) { > > + av_log(avctx, AV_LOG_ERROR, > > + "Unsupported channel count %d. Should be 1 or 2\n", channels); > > + return AVERROR(EINVAL); > > + } > > > > hr_mode |= srate_hz > 48000; > > hr_mode &= srate_hz >= 48000; > > @@ -195,9 +200,6 @@ const FFCodec ff_liblc3_encoder = { > > .p.type = AVMEDIA_TYPE_AUDIO, > > .p.id = AV_CODEC_ID_LC3, > > .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY, > > - .p.ch_layouts = (const AVChannelLayout[]) > > - { { AV_CHANNEL_ORDER_UNSPEC, 1 }, > > - { AV_CHANNEL_ORDER_UNSPEC, 2 }, { 0 } }, > > .p.supported_samplerates = (const int []) > > { 96000, 48000, 32000, 24000, 16000, 8000, 0 }, > > .p.sample_fmts = (const enum AVSampleFormat[]) > > I think we should rather change encode.c to mean that an UNSPEC channel > layout (in AVCodec.ch_layouts) works with every channel layout with the > same number of channels. This was my naive interpretation of the code, without checking the code. This would probably save some boilerplate (OTOH it should also be fine to apply the hotfix).
diff --git a/libavcodec/liblc3enc.c b/libavcodec/liblc3enc.c index 63d1645b10..5f8169a0cf 100644 --- a/libavcodec/liblc3enc.c +++ b/libavcodec/liblc3enc.c @@ -61,6 +61,11 @@ static av_cold int liblc3_encode_init(AVCodecContext *avctx) "Unsupported frame duration %.1f ms.\n", frame_us / 1000.f); return AVERROR(EINVAL); } + if (channels < 0 || channels > ENCODER_MAX_CHANNELS) { + av_log(avctx, AV_LOG_ERROR, + "Unsupported channel count %d. Should be 1 or 2\n", channels); + return AVERROR(EINVAL); + } hr_mode |= srate_hz > 48000; hr_mode &= srate_hz >= 48000; @@ -195,9 +200,6 @@ const FFCodec ff_liblc3_encoder = { .p.type = AVMEDIA_TYPE_AUDIO, .p.id = AV_CODEC_ID_LC3, .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY, - .p.ch_layouts = (const AVChannelLayout[]) - { { AV_CHANNEL_ORDER_UNSPEC, 1 }, - { AV_CHANNEL_ORDER_UNSPEC, 2 }, { 0 } }, .p.supported_samplerates = (const int []) { 96000, 48000, 32000, 24000, 16000, 8000, 0 }, .p.sample_fmts = (const enum AVSampleFormat[])
We only care about channel count. Layout details will be ignored either way. Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/liblc3enc.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)