[FFmpeg-devel] [PATCH] Set AVStream duration in several demuxers

[Michael Niedermayer]

On Thu, Jan 21, 2010 at 04:46:39AM -0500, David Conrad wrote:
> On Jan 20, 2010, at 1:24 PM, Michael Niedermayer wrote:
> 
> > On Wed, Jan 20, 2010 at 06:02:48PM +0100, Aurelien Jacobs wrote:
> >> On Mon, Jan 18, 2010 at 10:43:06PM -0500, David Conrad wrote:
> >>> Hi,
> >>> 
> >>> Several demuxers set duration and start_time in AVFormatContext, despite the comment to never set them directly. This sets them in AVStream instead, fixing "Estimating duration from bitrate, this may be inaccurate" messages.
> >>> 
> >>> iavs/ivas AVIs still set AVFormatContext duration, I didn't find a sample to test.
> >>> 
> >> 
> >>> commit d1ec4470be04fba3ff10fbecd455a14ab322d14e
> >>> Author: David Conrad <lessen42 at gmail.com>
> >>> Date:   Mon Jan 18 22:33:25 2010 -0500
> >>> 
> >>>    Set start_time and duration in AVStream not AVFormatContext.
> >>>    The latter is deduced from the former.
> >>> 
> >>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> >>> index 59dc166..7fcef04 100644
> >>> --- a/libavformat/matroskadec.c
> >>> +++ b/libavformat/matroskadec.c
> >>> @@ -1151,9 +1151,6 @@ static int matroska_read_header(AVFormatContext *s, AVFormatParameters *ap)
> >>>         return -1;
> >>>     matroska_execute_seekhead(matroska);
> >>> 
> >>> -    if (matroska->duration)
> >>> -        matroska->ctx->duration = matroska->duration * matroska->time_scale
> >>> -                                  * 1000 / AV_TIME_BASE;
> >>>     av_metadata_set(&s->metadata, "title", matroska->title);
> >>> 
> >>>     tracks = matroska->tracks.elem;
> >>> @@ -1338,6 +1335,9 @@ static int matroska_read_header(AVFormatContext *s, AVFormatParameters *ap)
> >>>             track->time_scale = 1.0;
> >>>         av_set_pts_info(st, 64, matroska->time_scale*track->time_scale, 1000*1000*1000); /* 64 bit pts in ns */
> >>> 
> >>> +        if (matroska->duration)
> >>> +            st->duration = matroska->duration;
> >>> +
> >>>         st->codec->codec_id = codec_id;
> >>>         st->start_time = 0;
> >>>         if (strcmp(track->language, "und"))
> >> 
> >> Strictly speeking this don't really seem correct. The segment duration
> >> stored in matroska file is (IIUC) the total duration of the segment.
> >> IOW it is the duration of the longest track. So using this same duration
> >> for every streams is not excatly correct. But provided the current avformat
> >> API, it seems to be the best we can do :-(
> >> 
> >> Maybe it would be better to improve the avformat API instead, to allow
> >> demuxers to write directly AVFormatContext.duration when they only know
> >> the total duration and not individual streams duration.
> >> I guess this (untested) patch would fix your original issue. It would
> >> also require an update of AVFormatContext.duration documentation.
> > 
> > iam not against this patch but it would have to be tested
> 
> Works for me. I went ahead and changed ape, mpc, and wavpack anyway since they're only one stream and it allows ffmpeg to calculate bitrate as well.
> 
> Here's the documentation update, does it look good to apply?
> 

> commit 3c198d5c78d102c88154cd4ebc3b2d9b6fc9ec7a
> Author: David Conrad <lessen42 at gmail.com>
> Date:   Thu Jan 21 04:34:11 2010 -0500
> 
>     Update documentation for AVFormatContext start_time and duration
> 
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index e4dba36..ec097b1 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -560,12 +560,13 @@ typedef struct AVFormatContext {
>      struct AVPacketList *packet_buffer;
>  
>      /** Decoding: position of the first frame of the component, in
> -       AV_TIME_BASE fractional seconds. NEVER set this value directly:
> -       It is deduced from the AVStream values.  */
> +       AV_TIME_BASE fractional seconds. If at least one AVStream
> +       start_time is set, this field is ignored and derived from
> +       those values instead */
>      int64_t start_time;
>      /** Decoding: duration of the stream, in AV_TIME_BASE fractional
> -       seconds. NEVER set this value directly: it is deduced from the
> -       AVStream values.  */
> +       seconds. If at least one AVStream start_time+duration is set,
> +       this field is ignored and derived from those values instead */
>      int64_t duration;

you are documenting the implementation dont do this please.
This is a public header, we likely want to change the implementation
once in a while. Also its read by 2 different kind of people
application developers and demuxer developers thetext must make sense to
both

What i would say is:
PTS of the first frame of the component, in AV_TIME_BASE fractional seconds.
demuxers should NEVER set this value directly if they set any start_time or
duration of any stream.

duration of the stream, in AV_TIME_BASE fractional seconds.
demuxers should NEVER set this value directly if they set any start_time or
duration of any stream.


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100121/d5b40913/attachment.pgp>