[Ffmpeg-devel] Matroska Patch

Steve Lhomme steve.lhomme
Tue Mar 21 21:07:26 CET 2006


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

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

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

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

Steve






More information about the ffmpeg-devel mailing list