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

[James Almer]

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 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.
>>
>> 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.