[FFmpeg-devel] MPEG Audio elementary streams and layers

[Marc Mason]

Michael Niedermayer wrote:

> Marc Mason wrote:
> 
>> CODEC_ID_MP2 and CODEC_ID_MP3 are defined in avcodec.h
>>
>> As far as I understand,
>> CODEC_ID_MP2 = MPEG Audio Layer II
>> CODEC_ID_MP3 = MPEG Audio Layer III
>>
>> CODEC_ID_MP3 appeared in rev 2231 with the following comment.
>> /* preferred ID for MPEG Audio layer 1, 2 or 3 decoding */
>> http://svn.ffmpeg.org/ffmpeg/trunk/libavcodec/avcodec.h?r1=2217&r2=2231
>>
>> What does the comment mean ?

Does anybody remember what the comment means ?

>> ff_mpegaudio_decode_header() determines the layer of the ES.
>> http://cekirdek.pardus.org.tr/~ismail/ffmpeg-docs/mpegaudiodecheader_8c-source.html#l00033
>> s->layer = 4 - ((header >> 17) & 3);
>>
>> ff_mpa_decode_header() then copies the info to avctx->sub_id
>> http://cekirdek.pardus.org.tr/~ismail/ffmpeg-docs/mpegaudio__parser_8c-source.html#l00047
>> avctx->sub_id = s->layer;
>>
>>
>> When av_find_stream_info() is given an MPEG Audio Layer II elementary
>> stream, it returns CODEC_ID_MP3 with sub_id=2
>>
>> This seems ambiguous to me.
>>
>> What was the original intent ? How is one supposed to determine the
>> layer of an MPEG Audio elementary stream ?
>>
>> I can see two possibilities.
>>
>> 1.) Use codec_id
>>
>> i.e. libavformat returns codec_id = one of CODEC_ID_MP1, CODEC_ID_MP2,
>> CODEC_ID_MP3 according to the layer.
>>
>> Then ff_mpa_decode_header() should set the field to CODEC_ID_MP2 when
>> appropriate.
>>
>> Something along the lines of
>>
>> $ svn diff
>> Index: mpegaudio_parser.c
>> ===================================================================
>> --- mpegaudio_parser.c  (revision 16054)
>> +++ mpegaudio_parser.c  (working copy)
>> @@ -62,6 +62,7 @@
>>           break;
>>       case 2:
>>           avctx->frame_size = 1152;
>> +        avctx->codec_id = CODEC_ID_MP2;
>>           break;
>>       default:
>>       case 3:
> 
> I suspect this patch is ok given it is tested and the indention isn't off

I suppose you meant "on condition that it is tested" ?

> and codec_id is also set for the MP3 case similarly

AFAIU, mp3_read_header() initializes codec_id to CODEC_ID_MP3. But it 
won't hurt performance to set it again in ff_mpa_decode_header()

>> However, what happens with layer I ES ?
>> CODEC_ID_MP1 might have to be defined ?
> 
> yes
> 
> [...]

You snipped the second option.
Does that mean you prefer the first option ?

In that case, setting sub_id becomes redundant, doesn't it ?

Moreover, the comments on sub_id mention

http://cekirdek.pardus.org.tr/~ismail/ffmpeg-docs/structAVCodecContext.html#3c0b6857bb530b32157ec6f922939424

"""
Some codecs need additional format info.

It is stored here. If any muxer uses this then ALL demuxers/parsers AND 
encoders for the specific codec MUST set it correctly otherwise stream 
copy breaks. In general use of this field by muxers is not recommended.
"""

However, avcodec_string() treats CODEC_ID_MP3 as a generic "MPEG Audio" 
codec ID with the layer in sub_id :

         if (!encode && enc->codec_id == CODEC_ID_MP3) {
             if (enc->sub_id == 2)
                 codec_name = "mp2";
             else if (enc->sub_id == 1)
                 codec_name = "mp1";
         }

If you give the green light on the first option, I'll fix that code too.

-- 
Regards.