[FFmpeg-devel] MPEG Audio elementary streams and layers

[Michael Niedermayer]

On Tue, Dec 16, 2008 at 10:00:40AM +0100, Marc Mason wrote:
> 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 ?

svn blame will tell you who and when that comment was added ...


> 
> >> 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" ?

yes
but looking again, the codec_id likely should be handled like the sample
rate, this should be more robust in presence of errors that might look like
valid headers.


> 
> > and codec_id is also set for the MP3 case similarly
> 
> AFAIU, mp3_read_header() initializes codec_id to CODEC_ID_MP3. But it 

if mp3_read_header() has been called, that is not certain though


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

if you remove its only use from utils.c ...

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081216/dfd051ba/attachment.pgp>