[FFmpeg-devel] [PATCH 01/17] avcodec/ac3enc: Don't presume ch_layout to be AV_CHANNEL_ORDER_NATIVE

[Andreas Rheinhardt]

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 at 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;