[FFmpeg-devel] [PATCH/RFC]av_codec_get_tag() cannot work for Matroska (Ticket 8)

[Aurelien Jacobs]

On Wed, Aug 24, 2011 at 02:37:33PM +0200, Carl Eugen Hoyos wrote:
> Hi!
> 
> ffmpeg.c sets codec_tag for the output stream in the stream copy case if 
> codec_tag was set by the demuxer, and no default codec_tag for the codec can 
> be found in the muxers' list of codec_tags using av_codec_get_tag (and also if 
> no such list was defined and also if the codec_tag is known to be correct).
> (line 2231 - 2235)
> 
> The ts demuxer always sets codec_tag, it was discussed here before that this 
> is intended (third party applications may like the value).
> 
> Matroska cannot provide a codec_tags list compatible with av_codec_get_tag and 
> therefore provides the wav (and the bmp) list.
> 
> Some codecs that work in Matroska are not listed in the wav (and bmp) list, 
> making stream copy impossible if a codec_tag was set because the set codec_tag 
> is rejected later (it is for example not set for TrueHD, so it is possible 
> that only the EAC3 in ts case - at least for some samples - triggers) in 
> avformat_write_header() / validate_codec_tag().
> 
> Attached is a try to fix this by adding the missing audio codec_tags (I will 
> do similar for subtitles and video if needed), I would be glad if a simpler 
> solution is possible like not to try to validate the codec_tag for matroska.
> 
> Carl Eugen

> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 0910da5..0bc995f 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -1214,6 +1214,25 @@ static int mkv_query_codec(enum CodecID codec_id, int std_compliance)
>      return 0;
>  }
>  
> +/* The following tags are not listed in ff_codec_wav_tags,
> + * so av_codec_get_id() and av_codec_get_tag() would incorrectly fail
> + * if they are not listed here. */
> +const AVCodecTag missing_audio_tags[] = {
> +    { CODEC_ID_EAC3,      0xFFFFFFFF },
> +    { CODEC_ID_MLP,       0xFFFFFFFF },
> +    { CODEC_ID_PCM_S16BE, 0xFFFFFFFF },
> +    { CODEC_ID_PCM_S24BE, 0xFFFFFFFF },
> +    { CODEC_ID_PCM_S32BE, 0xFFFFFFFF },
> +    { CODEC_ID_QDM2,      0xFFFFFFFF },
> +    { CODEC_ID_RA_144,    0xFFFFFFFF },
> +    { CODEC_ID_RA_288,    0xFFFFFFFF },
> +    { CODEC_ID_COOK,      0xFFFFFFFF },
> +    { CODEC_ID_TRUEHD,    0xFFFFFFFF },
> +    { CODEC_ID_TTA,       0xFFFFFFFF },
> +    { CODEC_ID_WAVPACK,   0xFFFFFFFF },
> +    { CODEC_ID_NONE,      0xFFFFFFFF }
> +};
> +
>  #if CONFIG_MATROSKA_MUXER
>  AVOutputFormat ff_matroska_muxer = {
>      .name              = "matroska",
> @@ -1235,7 +1254,7 @@ AVOutputFormat ff_matroska_muxer = {
>      .write_packet      = mkv_write_packet,
>      .write_trailer     = mkv_write_trailer,
>      .flags             = AVFMT_GLOBALHEADER | AVFMT_VARIABLE_FPS,
> -    .codec_tag         = (const AVCodecTag* const []){ff_codec_bmp_tags, ff_codec_wav_tags, 0},
> +    .codec_tag         = (const AVCodecTag* const []){ff_codec_bmp_tags, ff_codec_wav_tags, missing_audio_tags, 0},
>      .subtitle_codec    = CODEC_ID_SSA,
>      .query_codec       = mkv_query_codec,
>  };
> @@ -1274,6 +1293,6 @@ AVOutputFormat ff_matroska_audio_muxer = {
>      .write_packet      = mkv_write_packet,
>      .write_trailer     = mkv_write_trailer,
>      .flags = AVFMT_GLOBALHEADER,
> -    .codec_tag = (const AVCodecTag* const []){ff_codec_wav_tags, 0},
> +    .codec_tag = (const AVCodecTag* const []){ff_codec_wav_tags, missing_audio_tags, 0},
>  };
>  #endif

This patch looks OK, but I think I prefer your third patch.

> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index ef1de94..8fac96e 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -2986,7 +2986,7 @@ int avformat_write_header(AVFormatContext *s, AVDictionary **options)
>              break;
>          }
>  
> -        if(s->oformat->codec_tag){
> +        if(s->oformat->codec_tag && strcmp(s->oformat->name, "matroska")){
>              if(st->codec->codec_tag && st->codec->codec_id == CODEC_ID_RAWVIDEO && av_codec_get_tag(s->oformat->codec_tag, st->codec->codec_id) == 0 && !validate_codec_tag(s, st)){
>                  //the current rawvideo encoding system ends up setting the wrong codec_tag for avi, we override it here
>                  st->codec->codec_tag= 0;

Adding a special case for matroska in common code is not OK.

Aurel