[FFmpeg-devel] [PATCH] libavcodec/mpegaudio_parser.c: differentiate MPEG audio dual mono

[Scott Theisen]

On 12/10/24 19:16, James Almer wrote:
> On 12/10/2024 9:06 PM, Scott Theisen wrote:
>> On 12/9/24 02:30, Anton Khirnov wrote:
>>> Quoting James Almer (2024-11-30 14:41:20)
>>>> On 11/14/2024 1:37 AM, Scott Theisen wrote:
>>>>> When attempting to upstream this MythTV change in September 2022, 
>>>>> it was recommended to
>>>>> use AV_CHANNEL_ORDER_CUSTOM with two AV_CHAN_FRONT_CENTER 
>>>>> channels. See
>>>>> https://patchwork.ffmpeg.org/project/ffmpeg/ 
>>>>> patch/20220921192611.3241-1-scott.the.elm at gmail.com/
>>>>> ---
>>>>>    libavcodec/audiotoolboxdec.c    |  4 ++--
>>>>>    libavcodec/mpegaudio_parser.c   | 12 +++++++++---
>>>>>    libavcodec/mpegaudiodecheader.c |  4 +++-
>>>>>    libavcodec/mpegaudiodecheader.h |  2 +-
>>>>>    tests/ref/fate/pva-demux        |  2 +-
>>>>>    5 files changed, 16 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/libavcodec/audiotoolboxdec.c b/libavcodec/ 
>>>>> audiotoolboxdec.c
>>>>> index 0f7ce8e4eb..d279d7bbc4 100644
>>>>> --- a/libavcodec/audiotoolboxdec.c
>>>>> +++ b/libavcodec/audiotoolboxdec.c
>>>>> @@ -346,10 +346,10 @@ static av_cold int 
>>>>> ffat_create_decoder(AVCodecContext *avctx,
>>>>>                    avctx->codec_id == AV_CODEC_ID_MP2 ||
>>>>>                    avctx->codec_id == AV_CODEC_ID_MP3)) {
>>>>>            enum AVCodecID codec_id;
>>>>> -        int bit_rate;
>>>>> +        int bit_rate, dual_mono;
>>>>>            if (ff_mpa_decode_header(AV_RB32(pkt->data), &avctx- 
>>>>> >sample_rate,
>>>>> &in_format.mChannelsPerFrame, &avctx->frame_size,
>>>>> -                                 &bit_rate, &codec_id) < 0)
>>>>> +                                 &bit_rate, &codec_id, 
>>>>> &dual_mono) < 0)
>>>>>                return AVERROR_INVALIDDATA;
>>>>>            avctx->bit_rate = bit_rate;
>>>>>            in_format.mSampleRate = avctx->sample_rate;
>>>>> diff --git a/libavcodec/mpegaudio_parser.c b/libavcodec/ 
>>>>> mpegaudio_parser.c
>>>>> index d54366f10a..d1a4ee6434 100644
>>>>> --- a/libavcodec/mpegaudio_parser.c
>>>>> +++ b/libavcodec/mpegaudio_parser.c
>>>>> @@ -65,12 +65,12 @@ static int 
>>>>> mpegaudio_parse(AVCodecParserContext *s1,
>>>>>                }
>>>>>            }else{
>>>>>                while(i<buf_size){
>>>>> -                int ret, sr, channels, bit_rate, frame_size;
>>>>> +                int ret, sr, channels, bit_rate, frame_size, 
>>>>> dual_mono;
>>>>>                    enum AVCodecID codec_id = avctx->codec_id;
>>>>>                    state= (state<<8) + buf[i++];
>>>>> -                ret = ff_mpa_decode_header(state, &sr, &channels, 
>>>>> &frame_size, &bit_rate, &codec_id);
>>>>> +                ret = ff_mpa_decode_header(state, &sr, &channels, 
>>>>> &frame_size, &bit_rate, &codec_id, &dual_mono);
>>>>>                    if (ret < 4) {
>>>>>                        if (i > 4)
>>>>>                            s->header_count = -2;
>>>>> @@ -85,7 +85,13 @@ static int mpegaudio_parse(AVCodecParserContext 
>>>>> *s1,
>>>>>                        if (s->header_count > header_threshold) {
>>>>>                            avctx->sample_rate= sr;
>>>>> av_channel_layout_uninit(&avctx->ch_layout);
>>>>> - av_channel_layout_default(&avctx- >ch_layout, channels);
>>>>> +                        if (dual_mono) {
>>>>> + av_channel_layout_custom_init(&avctx- >ch_layout, 2);
>>>>> + avctx->ch_layout.u.map[0].id = AV_CHAN_FRONT_CENTER;
>>>>> + avctx->ch_layout.u.map[1].id = AV_CHAN_FRONT_CENTER;
>>>> Kind of sucks we have used FC to represent mono for so long that we
>>>> can't cleanly introduce a new channel that's strictly mono (with no
>>>> speaker location implied) and give it meaningful use.
>>> AV_CHAN_UNKNOWN?
>>>
>>
>> Using AV_CHAN_UNKNOWN would be fine with me.
>>
>> However, would using AV_CHANNEL_ORDER_UNSPEC instead of 
>> AV_CHANNEL_ORDER_CUSTOM be better?
>
> UNSPEC means the channels have no definition. We could allow the user 
> to assume 1 channel is "mono", but definitely not 2 channels for dual 
> mono.

The documentation for AV_CHANNEL_ORDER_UNSPEC says "Only the channel 
count is specified, without any further information about the channel 
order. ", not that the channels have no definition.  (I'm not sure what 
you mean by definition, though.)

Based on av_channel_layout_retype(), AV_CHANNEL_ORDER_CUSTOM with two 
AV_CHAN_UNKNOWN channels is equivalent to AV_CHANNEL_ORDER_UNSPEC with 
two channels.

Regards,

Scott Theisen