[Ffmpeg-devel] Matroska Patch

[Michael Niedermayer]

Hi

On Tue, Mar 21, 2006 at 05:20:33PM +0100, Steve Lhomme wrote:
> Hi,
> 
> Here is a new patch to support more features/codec from Matroska. They 
> have all been tested on DrDivX. They include:
> 
> - support for RealVideo in matroska
> - support for the real file duration
> - support for codec_private data (extradata)
> - set the discard flag for unsupported codecs
> - support a framerate (informational) when the track has a default duration
> - support for correct aspect ratio
> - support for Subtitle codec type (instead of being marked as video)
> - support for negative timecodes
> 
> There are more things than the previous patch that was never accepted. 
> But it doesn't include the correct support for (HE-)AAC.
> 
> Steve

[...]
> @@ -2178,6 +2182,50 @@
>                               MATROSKA_CODEC_ID_VIDEO_MPEG2))
>                  codec_id = CODEC_ID_MPEG2VIDEO;
>  
> +            else if (!strcmp(track->codec_id,
> +                             MATROSKA_CODEC_ID_VIDEO_RVIDEO40)) {
> +                extradata_size = track->codec_priv_size;
> +                if(extradata_size) {
> +                    extradata = av_malloc(extradata_size);
> +                    if(extradata == NULL)
> +                        return AVERROR_NOMEM;
> +                    memcpy(extradata, track->codec_priv, extradata_size);
> +                }
> +                codec_id = CODEC_ID_RV40;
> +            }
> +            else if (!strcmp(track->codec_id,
> +                             MATROSKA_CODEC_ID_VIDEO_RVIDEO30)) {
> +                extradata_size = track->codec_priv_size;
> +                if(extradata_size) {
> +                    extradata = av_malloc(extradata_size);
> +                    if(extradata == NULL)
> +                        return AVERROR_NOMEM;
> +                    memcpy(extradata, track->codec_priv, extradata_size);
> +                }
> +                codec_id = CODEC_ID_RV30;
> +            }
> +            else if (!strcmp(track->codec_id,
> +                             MATROSKA_CODEC_ID_VIDEO_RVIDEO20)) {
> +                extradata_size = track->codec_priv_size;
> +                if(extradata_size) {
> +                    extradata = av_malloc(extradata_size);
> +                    if(extradata == NULL)
> +                        return AVERROR_NOMEM;
> +                    memcpy(extradata, track->codec_priv, extradata_size);
> +                }
> +                codec_id = CODEC_ID_RV20;
> +            }
> +            else if (!strcmp(track->codec_id,
> +                             MATROSKA_CODEC_ID_VIDEO_RVIDEO10)) {
> +                extradata_size = track->codec_priv_size;
> +                if(extradata_size) {
> +                    extradata = av_malloc(extradata_size);
> +                    if(extradata == NULL)
> +                        return AVERROR_NOMEM;
> +                    memcpy(extradata, track->codec_priv, extradata_size);
> +                }
> +                codec_id = CODEC_ID_RV10;
> +            }

highly redundant, not ok ...

1. add a table with codec_id and strings for all codecs (this will also
   come in handy for muxing ...)
2. convert from string to id
3. add a switch(codec_id) 
    case CODEC_ID_RV10: 
    case CODEC_ID_RV20: 
    case CODEC_ID_RV30: 
    ...
        extradata_si...


[...]
> @@ -2248,10 +2296,16 @@
>              st = av_new_stream(s, track->stream_index);
>              if (st == NULL)
>                  return AVERROR_NOMEM;
> +            if (codec_id == CODEC_ID_NONE)
> +                matroska->ctx->streams[matroska->num_streams-1]->discard = AVDISCARD_ALL;

not acceptable
1. why should data by a unsupported codec be discarded, the user app might
   very well be able to make more sense out of it maybe by using a binary
   codec
2. AVStream->discard is a parameter set by the user you may not set it


[...]
>  
> +            if (track->default_duration)
> +                av_reduce(&st->codec->time_base.num, &st->codec->time_base.den, track->default_duration, 1000000000, 30000);

not acceptable


[...]
> -                uint64_t time;
> +                int64_t block_time;
>                  uint32_t *lace_size = NULL;
>                  int n, track, flags, laces = 0;
>                  uint64_t num;
> @@ -2372,8 +2434,8 @@
>                      break;
>                  }
>  
> -                /* time (relative to cluster time) */
> -                time = ((data[0] << 8) | data[1]) * matroska->time_scale;
> +                /* block_time (relative to cluster time) */
> +                block_time = ((data[0] << 8) | data[1]) * matroska->time_scale;
>                  data += 2;
>                  size -= 2;
>                  flags = *data;
> @@ -2470,10 +2532,10 @@
>                              break;
>                          }
>                          if (cluster_time != (uint64_t)-1) {
> -                            if (time < 0 && (-time) > cluster_time)
> +                            if (block_time < 0 && (-block_time) > cluster_time)
>                                  timecode = cluster_time;
>                              else
> -                                timecode = cluster_time + time;
> +                                timecode = cluster_time + block_time;

cosmetical changes are not accpetable in functional patches

[...]

-- 
Michael