[Ffmpeg-devel] Matroska Patch

[Måns Rullgård]

Steve Lhomme <slhomme at divxcorp.com> writes:

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

> -                matroska->duration = num * matroska->time_scale;
> +                matroska->ctx->duration = num * matroska->time_scale * 1000 / AV_TIME_BASE;

Would it not make sense to set the stream time base from the matroska
time scale?

>              av_set_pts_info(st, 24, 1, 1000); /* 24 bit pts in ms */
>
>              st->codec->codec_id = codec_id;
>
> +            if (track->default_duration)
> +                av_reduce(&st->codec->time_base.num, &st->codec->time_base.den, track->default_duration, 1000000000, 30000);

Why 30000?  Also, av_set_pts_info() should be called with values
compatible with time_base.

> +                // pixel aspect ratio
> +                if (videotrack->display_width != 0 || videotrack->display_height != 0) {
> +                    int64_t dis_width, dis_height;
> +                    if (videotrack->display_width != 0)
> +                        dis_width = videotrack->display_width;
> +                    else
> +                        dis_width = videotrack->pixel_width;
> +                    if (videotrack->display_height != 0)
> +                        dis_height = videotrack->display_height;
> +                    else
> +                        dis_height = videotrack->pixel_height;
> +                    av_reduce(&st->codec->sample_aspect_ratio.num,
> +                              &st->codec->sample_aspect_ratio.den,
> +                              st->codec->height * dis_width,
> +                              st->codec->width * dis_height,
> +                              1000);

Why 1000?

>  static int
>  matroska_parse_blockgroup (MatroskaDemuxContext *matroska,
> -                           uint64_t              cluster_time)
> +                           int64_t              cluster_time)

Cluster time is defined in the spec as unsigned.

>  {
>      int res = 0;
>      uint32_t id;
> @@ -2339,7 +2401,7 @@
>              case MATROSKA_ID_BLOCK: {
>                  uint8_t *data, *origdata;
>                  int size;
> -                uint64_t time;
> +                int64_t block_time;

Why rename the variable?

The rest looks OK.

Could you please send independent changes as separate patches?  It
makes reviewing much easier.

-- 
M?ns Rullg?rd
mru at inprovide.com