| Message ID | GV1P250MB073758D71A50C11C0A3C26CE8F012@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Commit | c3fb0c5babb2ae3b8985d038dd0ce228d52f85d7 |
| Headers | show |
| Series | [FFmpeg-devel,01/17] avcodec/ac3enc: Don't presume ch_layout to be AV_CHANNEL_ORDER_NATIVE | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 4/7/2024 5:39 PM, Andreas Rheinhardt wrote: > It is perfectly legal for users to use a custom layout > that is equivalent to a supported native one. Is that really the case? FFCodec.p.ch_layouts[] has a list of native ones, and the generic encode.c code will reject anything not in it. I guess the encode.c check could be improved with av_channel_layout_retype(). > In this case the union in AVChannelLayout is not an uint64_t mask, > but a pointer to a custom map. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/ac3enc.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/libavcodec/ac3enc.c b/libavcodec/ac3enc.c > index 7a6bcf7900..dc197c1517 100644 > --- a/libavcodec/ac3enc.c > +++ b/libavcodec/ac3enc.c > @@ -2192,18 +2192,14 @@ av_cold int ff_ac3_encode_close(AVCodecContext *avctx) > static av_cold int set_channel_info(AVCodecContext *avctx) > { > AC3EncodeContext *s = avctx->priv_data; > + uint64_t mask = av_channel_layout_subset(&avctx->ch_layout, ~(uint64_t)0); Might as well use UINT64_MAX. > int channels = avctx->ch_layout.nb_channels; > - uint64_t mask = avctx->ch_layout.u.mask; > > if (channels < 1 || channels > AC3_MAX_CHANNELS) > return AVERROR(EINVAL); > if (mask > 0x7FF) > return AVERROR(EINVAL); > > - if (!mask) > - av_channel_layout_default(&avctx->ch_layout, channels); > - mask = avctx->ch_layout.u.mask; > - > s->lfe_on = !!(mask & AV_CH_LOW_FREQUENCY); > s->channels = channels; > s->fbw_channels = channels - s->lfe_on;
James Almer: > On 4/7/2024 5:39 PM, Andreas Rheinhardt wrote: >> It is perfectly legal for users to use a custom layout >> that is equivalent to a supported native one. > > Is that really the case? FFCodec.p.ch_layouts[] has a list of native > ones, and the generic encode.c code will reject anything not in it. > This is not true. It allows everything that is equivalent (in the av_channel_layout_compare() sense) to one of the channel layouts in AVCodec.ch_layouts. This means that if ch_layout.u.map[i].id is strictly ascending for 0 <= i <nb_channels and < 64, then it is equivalent to the native channel layout with the same nb_channels whose mask is the bitwise or of all (1ULL << ch_layout.u.map[i].id). > I guess the encode.c check could be improved with > av_channel_layout_retype(). > ch_layout is not supposed to be set by lavc for encoders. >> In this case the union in AVChannelLayout is not an uint64_t mask, >> but a pointer to a custom map. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavcodec/ac3enc.c | 6 +----- >> 1 file changed, 1 insertion(+), 5 deletions(-) >> >> diff --git a/libavcodec/ac3enc.c b/libavcodec/ac3enc.c >> index 7a6bcf7900..dc197c1517 100644 >> --- a/libavcodec/ac3enc.c >> +++ b/libavcodec/ac3enc.c >> @@ -2192,18 +2192,14 @@ av_cold int ff_ac3_encode_close(AVCodecContext >> *avctx) >> static av_cold int set_channel_info(AVCodecContext *avctx) >> { >> AC3EncodeContext *s = avctx->priv_data; >> + uint64_t mask = av_channel_layout_subset(&avctx->ch_layout, >> ~(uint64_t)0); > > Might as well use UINT64_MAX. > This is a bitfield, so I intentionally used a bit-operation. >> int channels = avctx->ch_layout.nb_channels; >> - uint64_t mask = avctx->ch_layout.u.mask; >> if (channels < 1 || channels > AC3_MAX_CHANNELS) >> return AVERROR(EINVAL); >> if (mask > 0x7FF) >> return AVERROR(EINVAL); >> - if (!mask) >> - av_channel_layout_default(&avctx->ch_layout, channels); >> - mask = avctx->ch_layout.u.mask; >> - >> s->lfe_on = !!(mask & AV_CH_LOW_FREQUENCY); >> s->channels = channels; >> s->fbw_channels = channels - s->lfe_on;
On 4/7/2024 6:53 PM, Andreas Rheinhardt wrote: > James Almer: >> On 4/7/2024 5:39 PM, Andreas Rheinhardt wrote: >>> It is perfectly legal for users to use a custom layout >>> that is equivalent to a supported native one. >> >> Is that really the case? FFCodec.p.ch_layouts[] has a list of native >> ones, and the generic encode.c code will reject anything not in it. >> > > This is not true. It allows everything that is equivalent (in the > av_channel_layout_compare() sense) to one of the channel layouts in > AVCodec.ch_layouts. This means that if ch_layout.u.map[i].id is strictly > ascending for 0 <= i <nb_channels and < 64, then it is equivalent to the > native channel layout with the same nb_channels whose mask is the > bitwise or of all (1ULL << ch_layout.u.map[i].id). Right, misread av_channel_layout_compare() as rejecting layouts that were not the same order. > >> I guess the encode.c check could be improved with >> av_channel_layout_retype(). >> > > ch_layout is not supposed to be set by lavc for encoders. I didn't mean to set the layout for encoders, i meant it as a way for encode.c to compare input layouts with encoder-supported native layouts, because of my misunderstanding of how av_channel_layout_compare() behaved. > >>> In this case the union in AVChannelLayout is not an uint64_t mask, >>> but a pointer to a custom map. >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>> --- >>> libavcodec/ac3enc.c | 6 +----- >>> 1 file changed, 1 insertion(+), 5 deletions(-) >>> >>> diff --git a/libavcodec/ac3enc.c b/libavcodec/ac3enc.c >>> index 7a6bcf7900..dc197c1517 100644 >>> --- a/libavcodec/ac3enc.c >>> +++ b/libavcodec/ac3enc.c >>> @@ -2192,18 +2192,14 @@ av_cold int ff_ac3_encode_close(AVCodecContext >>> *avctx) >>> static av_cold int set_channel_info(AVCodecContext *avctx) >>> { >>> AC3EncodeContext *s = avctx->priv_data; >>> + uint64_t mask = av_channel_layout_subset(&avctx->ch_layout, >>> ~(uint64_t)0); >> >> Might as well use UINT64_MAX. >> > > This is a bitfield, so I intentionally used a bit-operation. Then ~UINT64_C(0). > >>> int channels = avctx->ch_layout.nb_channels; >>> - uint64_t mask = avctx->ch_layout.u.mask; >>> if (channels < 1 || channels > AC3_MAX_CHANNELS) >>> return AVERROR(EINVAL); >>> if (mask > 0x7FF) >>> return AVERROR(EINVAL); >>> - if (!mask) >>> - av_channel_layout_default(&avctx->ch_layout, channels); >>> - mask = avctx->ch_layout.u.mask; >>> - >>> s->lfe_on = !!(mask & AV_CH_LOW_FREQUENCY); >>> s->channels = channels; >>> s->fbw_channels = channels - s->lfe_on; > > _______________________________________________ > 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".
James Almer: > On 4/7/2024 6:53 PM, Andreas Rheinhardt wrote: >> James Almer: >>> On 4/7/2024 5:39 PM, Andreas Rheinhardt wrote: >>>> It is perfectly legal for users to use a custom layout >>>> that is equivalent to a supported native one. >>> >>> Is that really the case? FFCodec.p.ch_layouts[] has a list of native >>> ones, and the generic encode.c code will reject anything not in it. >>> >> >> This is not true. It allows everything that is equivalent (in the >> av_channel_layout_compare() sense) to one of the channel layouts in >> AVCodec.ch_layouts. This means that if ch_layout.u.map[i].id is strictly >> ascending for 0 <= i <nb_channels and < 64, then it is equivalent to the >> native channel layout with the same nb_channels whose mask is the >> bitwise or of all (1ULL << ch_layout.u.map[i].id). > > Right, misread av_channel_layout_compare() as rejecting layouts that > were not the same order. > >> >>> I guess the encode.c check could be improved with >>> av_channel_layout_retype(). >>> >> >> ch_layout is not supposed to be set by lavc for encoders. > > I didn't mean to set the layout for encoders, i meant it as a way for > encode.c to compare input layouts with encoder-supported native layouts, > because of my misunderstanding of how av_channel_layout_compare() behaved. > >> >>>> In this case the union in AVChannelLayout is not an uint64_t mask, >>>> but a pointer to a custom map. >>>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>>> --- >>>> libavcodec/ac3enc.c | 6 +----- >>>> 1 file changed, 1 insertion(+), 5 deletions(-) >>>> >>>> diff --git a/libavcodec/ac3enc.c b/libavcodec/ac3enc.c >>>> index 7a6bcf7900..dc197c1517 100644 >>>> --- a/libavcodec/ac3enc.c >>>> +++ b/libavcodec/ac3enc.c >>>> @@ -2192,18 +2192,14 @@ av_cold int ff_ac3_encode_close(AVCodecContext >>>> *avctx) >>>> static av_cold int set_channel_info(AVCodecContext *avctx) >>>> { >>>> AC3EncodeContext *s = avctx->priv_data; >>>> + uint64_t mask = av_channel_layout_subset(&avctx->ch_layout, >>>> ~(uint64_t)0); >>> >>> Might as well use UINT64_MAX. >>> >> >> This is a bitfield, so I intentionally used a bit-operation. > > Then ~UINT64_C(0). > And the advantage of this is? - Andreas
On 4/7/2024 7:26 PM, Andreas Rheinhardt wrote: > James Almer: >> On 4/7/2024 6:53 PM, Andreas Rheinhardt wrote: >>> James Almer: >>>> On 4/7/2024 5:39 PM, Andreas Rheinhardt wrote: >>>>> It is perfectly legal for users to use a custom layout >>>>> that is equivalent to a supported native one. >>>> >>>> Is that really the case? FFCodec.p.ch_layouts[] has a list of native >>>> ones, and the generic encode.c code will reject anything not in it. >>>> >>> >>> This is not true. It allows everything that is equivalent (in the >>> av_channel_layout_compare() sense) to one of the channel layouts in >>> AVCodec.ch_layouts. This means that if ch_layout.u.map[i].id is strictly >>> ascending for 0 <= i <nb_channels and < 64, then it is equivalent to the >>> native channel layout with the same nb_channels whose mask is the >>> bitwise or of all (1ULL << ch_layout.u.map[i].id). >> >> Right, misread av_channel_layout_compare() as rejecting layouts that >> were not the same order. >> >>> >>>> I guess the encode.c check could be improved with >>>> av_channel_layout_retype(). >>>> >>> >>> ch_layout is not supposed to be set by lavc for encoders. >> >> I didn't mean to set the layout for encoders, i meant it as a way for >> encode.c to compare input layouts with encoder-supported native layouts, >> because of my misunderstanding of how av_channel_layout_compare() behaved. >> >>> >>>>> In this case the union in AVChannelLayout is not an uint64_t mask, >>>>> but a pointer to a custom map. >>>>> >>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>>>> --- >>>>> libavcodec/ac3enc.c | 6 +----- >>>>> 1 file changed, 1 insertion(+), 5 deletions(-) >>>>> >>>>> diff --git a/libavcodec/ac3enc.c b/libavcodec/ac3enc.c >>>>> index 7a6bcf7900..dc197c1517 100644 >>>>> --- a/libavcodec/ac3enc.c >>>>> +++ b/libavcodec/ac3enc.c >>>>> @@ -2192,18 +2192,14 @@ av_cold int ff_ac3_encode_close(AVCodecContext >>>>> *avctx) >>>>> static av_cold int set_channel_info(AVCodecContext *avctx) >>>>> { >>>>> AC3EncodeContext *s = avctx->priv_data; >>>>> + uint64_t mask = av_channel_layout_subset(&avctx->ch_layout, >>>>> ~(uint64_t)0); >>>> >>>> Might as well use UINT64_MAX. >>>> >>> >>> This is a bitfield, so I intentionally used a bit-operation. >> >> Then ~UINT64_C(0). >> > > And the advantage of this is? It's the proper way to expand a literal to ensure it's 64bits, and we use it everywhere for that purpose. But it's a nit in any case.
Andreas Rheinhardt: > It is perfectly legal for users to use a custom layout > that is equivalent to a supported native one. > In this case the union in AVChannelLayout is not an uint64_t mask, > but a pointer to a custom map. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/ac3enc.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/libavcodec/ac3enc.c b/libavcodec/ac3enc.c > index 7a6bcf7900..dc197c1517 100644 > --- a/libavcodec/ac3enc.c > +++ b/libavcodec/ac3enc.c > @@ -2192,18 +2192,14 @@ av_cold int ff_ac3_encode_close(AVCodecContext *avctx) > static av_cold int set_channel_info(AVCodecContext *avctx) > { > AC3EncodeContext *s = avctx->priv_data; > + uint64_t mask = av_channel_layout_subset(&avctx->ch_layout, ~(uint64_t)0); > int channels = avctx->ch_layout.nb_channels; > - uint64_t mask = avctx->ch_layout.u.mask; > > if (channels < 1 || channels > AC3_MAX_CHANNELS) > return AVERROR(EINVAL); > if (mask > 0x7FF) > return AVERROR(EINVAL); > > - if (!mask) > - av_channel_layout_default(&avctx->ch_layout, channels); > - mask = avctx->ch_layout.u.mask; > - > s->lfe_on = !!(mask & AV_CH_LOW_FREQUENCY); > s->channels = channels; > s->fbw_channels = channels - s->lfe_on; Will apply this patchset tomorrow unless there are objections. - Andreas
diff --git a/libavcodec/ac3enc.c b/libavcodec/ac3enc.c index 7a6bcf7900..dc197c1517 100644 --- a/libavcodec/ac3enc.c +++ b/libavcodec/ac3enc.c @@ -2192,18 +2192,14 @@ av_cold int ff_ac3_encode_close(AVCodecContext *avctx) static av_cold int set_channel_info(AVCodecContext *avctx) { AC3EncodeContext *s = avctx->priv_data; + uint64_t mask = av_channel_layout_subset(&avctx->ch_layout, ~(uint64_t)0); int channels = avctx->ch_layout.nb_channels; - uint64_t mask = avctx->ch_layout.u.mask; if (channels < 1 || channels > AC3_MAX_CHANNELS) return AVERROR(EINVAL); if (mask > 0x7FF) return AVERROR(EINVAL); - if (!mask) - av_channel_layout_default(&avctx->ch_layout, channels); - mask = avctx->ch_layout.u.mask; - s->lfe_on = !!(mask & AV_CH_LOW_FREQUENCY); s->channels = channels; s->fbw_channels = channels - s->lfe_on;
It is perfectly legal for users to use a custom layout that is equivalent to a supported native one. In this case the union in AVChannelLayout is not an uint64_t mask, but a pointer to a custom map. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/ac3enc.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)