[FFmpeg-devel] [RFC] write standard stsd tags and objecttype for mp4/3gp

[Michael Niedermayer]

On Mon, Jan 28, 2008 at 12:44:57PM +0100, Baptiste Coudurier wrote:
> Hi
> 
> I had that in my tree since some time now anyway, and it will avoid
> other people wasting their time.
> 
> This patch should do what's needed, except a few things:
> 
> - stream copying mpeg-2 removes sequence header and puts it in extradata
> -> is the mpegvideo split function actually used ? Can mpeg2 be stored
> in any container without seq header ?
> -> -vglobal 4 makes it working but it still puts extradata, and then
> extradata is written in decodec specific tag, Im pretty sure this is
> should not be done.
> 
> - set correct profiles for mpeg-2.
> 
> One patch to move prefered object types to be be chosen first.
> One other patch to enable muxing.
> 

> This should be open for discussion, and I hope this time it won't turn
> into flamewar.

i hope as well


[...]
> Index: libavformat/movenc.c
> ===================================================================
> --- libavformat/movenc.c	(revision 11646)
> +++ libavformat/movenc.c	(working copy)
> @@ -60,7 +60,7 @@
>      int         hasBframes;
>      int         language;
>      int         trackID;
> -    int         tag;
> +    int         tag; ///< stsd fourcc
>      AVCodecContext *enc;
>  
>      int         vosLen;

ok, commit anytime



> @@ -381,7 +381,7 @@
>          track->enc->codec_id == CODEC_ID_PCM_S24LE ||
>          track->enc->codec_id == CODEC_ID_PCM_S32LE))
>          mov_write_wave_tag(pb, track);
> -    else if(track->enc->codec_id == CODEC_ID_AAC)
> +    else if(track->tag == MKTAG('m','p','4','a'))
>          mov_write_esds_tag(pb, track);
>      else if(track->enc->codec_id == CODEC_ID_AMR_NB)
>          mov_write_amr_tag(pb, track);

ok, commit anytime


[...]
> +static int mov_find_codec_tag(AVFormatContext *s, MOVTrack *track)
>  {
>      int tag = track->enc->codec_tag;
> -    if (!tag) {
> +    if (track->mode == MODE_MP4 || track->mode == MODE_PSP) {
> +        if (!codec_get_tag(ff_mp4_obj_type, track->enc->codec_id))
> +            return 0;

this breaks h.263 muxing (assuming it was spec compliant before)
as ff_mp4_obj_type doesnt have a h263 entry


> +        if (track->enc->codec_id == CODEC_ID_H264)           tag = MKTAG('a','v','c','1');
> +        else if (track->enc->codec_type == CODEC_TYPE_VIDEO) tag = MKTAG('m','p','4','v');
> +        else if (track->enc->codec_type == CODEC_TYPE_AUDIO) tag = MKTAG('m','p','4','a');

this looks ok i think if you checked the spec, otherwise someone should
check to make sure mp4v/mp4a are mandated by the latest spec instead of
samr/sawb/...


> +    } else if (track->mode == MODE_3GP || track->mode == MODE_3G2) {
> +        tag = codec_get_tag(codec_3gp_tags, track->enc->codec_id);

again, depends on what the spec says, if you checked it, iam ok with it


[...]
> +    track->tag = tag;
>      return tag;

i think retunng the tag and vissibly setting it is more readable
that is IMHO
 track->tag = mov_find_video_codec_tag(s, track);
is better than
if (!mov_find_codec_tag(s, track)) {
    av_log(s, AV_LOG_ERROR, "track %d: could not find tag for codec\n", i);
    return -1;
}

where mov_find_codec_tag() has a "hidden" sideeffect of setting track->tag


>  }
>  

> @@ -573,7 +579,7 @@
>  
>      put_be16(pb, 0x18); /* Reserved */
>      put_be16(pb, 0xffff); /* Reserved */
> -    if(track->enc->codec_id == CODEC_ID_MPEG4)
> +    if(track->tag == MKTAG('m','p','4','v'))
>          mov_write_esds_tag(pb, track);
>      else if(track->enc->codec_id == CODEC_ID_H263)
>          mov_write_d263_tag(pb);

ok, commit anytime


[...]
> Index: libavformat/isom.c
> ===================================================================
> --- libavformat/isom.c	(revision 11646)
> +++ libavformat/isom.c	(working copy)
> @@ -26,12 +26,13 @@
>  #include "isom.h"
>  
>  /* http://www.mp4ra.org */
> +/* ordered by muxing preference */
>  const AVCodecTag ff_mp4_obj_type[] = {
>      { CODEC_ID_MPEG4     ,  32 },
>      { CODEC_ID_H264      ,  33 },
>      { CODEC_ID_AAC       ,  64 },
> +    { CODEC_ID_MPEG2VIDEO,  97 }, /* MPEG2 Main */
>      { CODEC_ID_MPEG2VIDEO,  96 }, /* MPEG2 Simple */
> -    { CODEC_ID_MPEG2VIDEO,  97 }, /* MPEG2 Main */
>      { CODEC_ID_MPEG2VIDEO,  98 }, /* MPEG2 SNR */
>      { CODEC_ID_MPEG2VIDEO,  99 }, /* MPEG2 Spatial */
>      { CODEC_ID_MPEG2VIDEO, 100 }, /* MPEG2 High */
> @@ -39,9 +40,9 @@
>      { CODEC_ID_AAC       , 102 }, /* MPEG2 AAC Main */
>      { CODEC_ID_AAC       , 103 }, /* MPEG2 AAC Low */
>      { CODEC_ID_AAC       , 104 }, /* MPEG2 AAC SSR */
> +    { CODEC_ID_MP3       , 107 }, /* 11172-3 */
>      { CODEC_ID_MP3       , 105 }, /* 13818-3 */
>      { CODEC_ID_MPEG1VIDEO, 106 }, /* 11172-2 */
> -    { CODEC_ID_MP3       , 107 }, /* 11172-3 */
>      { CODEC_ID_MJPEG     , 108 }, /* 10918-1 */
>      { CODEC_ID_PNG       , 109 },
>      { CODEC_ID_JPEG2000  , 110 }, /* 15444-1 */

ok, commit anytime

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
-------------- 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/20080128/199469d8/attachment.pgp>