[Ffmpeg-devel] Matroska Patch

[Måns Rullgård]

Steve Lhomme <steve.lhomme at free.fr> writes:

> M?ns Rullg?rd wrote:
>>> -                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?
>
> Sorry, I don't understand. But the duration is not related to any
> other values. It's always in ?s. (at least that's what other
> containers give).

Check how some other demuxers do it.

>>>              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.
>
> Well, adding a feature doesn't mean I'm going to fix all the bugs in
> that demuxer.

If you're messing with the time_base things, you need to keep it
consistent.  If it was already broken, no point changing it into being
broken in a different way.

>>> +                // 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?
>
> Because that's likely to be a value big enough so that it's never
> used. If you have a nicer solution it's welcome. But at least it
> worked with all the files I have.

INT_MAX?

>>>  static int
>>>  matroska_parse_blockgroup (MatroskaDemuxContext *matroska,
>>> -                           uint64_t              cluster_time)
>>> +                           int64_t              cluster_time)
>> Cluster time is defined in the spec as unsigned.
>
> I know, but adding a negative value to a unsigned value that can be 0
> is never a good idea in the code. So all timecodes are handled as
> signed.

The cluster time is unsigned.  You can't treat it as signed.

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