use ASF preroll as st->start_time

Message ID 4D80C29C.6020706@gmail.com
State Superseded
Headers show

Commit Message

Vladimir Pantelic March 16, 2011, 2:01 p.m.
./ffmpeg -y -i asf_with_chapters.wmv -vcodec copy -acodec copy test.asf
(the file is in the samples collection)

current git:

is: dts:3000
                 os: dts:-57
is: dts:3040
                 os: dts:-17
is: dts:3057
                 os: dts:0
is: dts:3057
is: dts:3080
                 os: dts:23
is: dts:3120
                 os: dts:63
is: dts:3160
                 os: dts:103
is: dts:3200
                 os: dts:143
is: dts:3215
                 os: dts:158

thus negative timestamps are written into the output asf file which
according to asf specs is not allowed.

reverting svn r9963:

------------------------------------------------------------------------
r9963 | michael | 2007-08-06 22:36:55 +0200 (Mon, 06 Aug 2007) | 2 lines

ignore preroll, it is generally not what AVStream.start_time should contain
------------------------------------------------------------------------

results in:

is: dts:3000
                 os: dts:0
is: dts:3040
                 os: dts:40
is: dts:3057
                 os: dts:57
is: dts:3057
is: dts:3080
                 os: dts:80
is: dts:3120
                 os: dts:120
is: dts:3160
                 os: dts:160
is: dts:3200
                 os: dts:200
is: dts:3215
                 os: dts:215


the ASF spec says about preroll:

Specifies the amount of time to buffer data before starting to play the file,
in millisecond units. If this value is nonzero, the Play Duration field and all
of the payload Presentation Time fields have been offset by this amount. Therefore,
player software must subtract the value in the preroll field from the play duration
and presentation times to calculate their actual values. It follows that all payload
Presentation Time fields need to be at least this value.

yet libavformat/avformat.h says:

     /**
      * Decoding: pts of the first frame of the stream, in stream time base.
      * Only set this if you are absolutely 100% sure that the value you set
      * it to really is the pts of the first frame.
      * This may be undefined (AV_NOPTS_VALUE).
      * @note The ASF header does NOT contain a correct start_time the ASF
      * demuxer must NOT set this.
      */
     int64_t start_time;

with a special note for ASF.

So, maybe using preroll as start_time is bad, but so is not doing it as we can see
above. I tend to rather follow the spec.

Comments

Mans Rullgard March 16, 2011, 2:17 p.m. | #1
Vladimir Pantelic <vladoman@gmail.com> writes:

> ./ffmpeg -y -i asf_with_chapters.wmv -vcodec copy -acodec copy test.asf
> (the file is in the samples collection)
>
> current git:
>
> is: dts:3000
>                 os: dts:-57
> is: dts:3040
>                 os: dts:-17
> is: dts:3057
>                 os: dts:0
> is: dts:3057
> is: dts:3080
>                 os: dts:23
> is: dts:3120
>                 os: dts:63
> is: dts:3160
>                 os: dts:103
> is: dts:3200
>                 os: dts:143
> is: dts:3215
>                 os: dts:158
>
> thus negative timestamps are written into the output asf file which
> according to asf specs is not allowed.
>
> reverting svn r9963:
>
> ------------------------------------------------------------------------
> r9963 | michael | 2007-08-06 22:36:55 +0200 (Mon, 06 Aug 2007) | 2 lines
>
> ignore preroll, it is generally not what AVStream.start_time should contain
> ------------------------------------------------------------------------
>
> results in:
>
> is: dts:3000
>                 os: dts:0
> is: dts:3040
>                 os: dts:40
> is: dts:3057
>                 os: dts:57
> is: dts:3057
> is: dts:3080
>                 os: dts:80
> is: dts:3120
>                 os: dts:120
> is: dts:3160
>                 os: dts:160
> is: dts:3200
>                 os: dts:200
> is: dts:3215
>                 os: dts:215
>
> the ASF spec says about preroll:
>
> Specifies the amount of time to buffer data before starting to play
> the file, in millisecond units. If this value is nonzero, the Play
> Duration field and all of the payload Presentation Time fields have
> been offset by this amount. Therefore, player software must subtract
> the value in the preroll field from the play duration and presentation
> times to calculate their actual values. It follows that all payload
> Presentation Time fields need to be at least this value.
>
> yet libavformat/avformat.h says:
>
>     /**
>      * Decoding: pts of the first frame of the stream, in stream time base.
>      * Only set this if you are absolutely 100% sure that the value you set
>      * it to really is the pts of the first frame.
>      * This may be undefined (AV_NOPTS_VALUE).
>      * @note The ASF header does NOT contain a correct start_time the ASF
>      * demuxer must NOT set this.
>      */
>     int64_t start_time;
>
> with a special note for ASF.
>
> So, maybe using preroll as start_time is bad, but so is not doing it
> as we can see above. I tend to rather follow the spec.

The alternative would be to subtract the preroll value from all
timestamps in the demuxer and add it back in the muxer.  Your patch
seems much simpler.

> From 60731793612f2f4800d1b93a959ba35f3fd7e829 Mon Sep 17 00:00:00 2001
> From: Vladimir Pantelic <vladoman@gmail.com>
> Date: Wed, 16 Mar 2011 14:53:05 +0100
> Subject: [PATCH] revert SVN r9963 and use the ASF preroll value as st->start_time again
>
> when not doing so, even a simple ASF to ASF stream copy results in
> an ASF file that has negative time stamps which is not allowed.
> ---
>  libavformat/asfdec.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/libavformat/asfdec.c b/libavformat/asfdec.c
> index cbcd576..ff1f166 100644
> --- a/libavformat/asfdec.c
> +++ b/libavformat/asfdec.c
> @@ -218,7 +218,7 @@ static int asf_read_stream_properties(AVFormatContext *s, int64_t size)
>      int type_specific_size, sizeX;
>      uint64_t total_size;
>      unsigned int tag1;
> -    int64_t pos1, pos2, start_time;
> +    int64_t pos1, pos2;
>      int test_for_ext_stream_audio, is_dvr_ms_audio=0;
>  
>      if (s->nb_streams == ASF_MAX_STREAMS) {
> @@ -236,13 +236,13 @@ static int asf_read_stream_properties(AVFormatContext *s, int64_t size)
>      if (!asf_st)
>          return AVERROR(ENOMEM);
>      st->priv_data = asf_st;
> -    start_time = asf->hdr.preroll;
> +    st->start_time = asf->hdr.preroll;
>  
>      asf_st->stream_language_index = 128; // invalid stream index means no language info
>  
>      if(!(asf->hdr.flags & 0x01)) { // if we aren't streaming...
>          st->duration = asf->hdr.play_time /
> -            (10000000 / 1000) - start_time;
> +            (10000000 / 1000) - st->start_time;
>      }
>      ff_get_guid(pb, &g);
>  
> -- 
> 1.6.0.2

Timestamp changes by michael tend to be wrong, so reverting it seems right...
I'd remove the note in the start_time doxy as well.
Vladimir Pantelic March 16, 2011, 8:43 p.m. | #2
Måns Rullgård wrote:
> Vladimir Pantelic<vladoman@gmail.com>  writes:

>>  So, maybe using preroll as start_time is bad, but so is not doing it
>>  as we can see above. I tend to rather follow the spec.
>
> The alternative would be to subtract the preroll value from all
> timestamps in the demuxer and add it back in the muxer.  Your patch
> seems much simpler.

Reading the spec again, I think we should rather subtract the preroll
in the demuxer:

"...player software *must* subtract the value in the preroll field
from the play duration and presentation times to calculate their
actual values..."

Patch

From 60731793612f2f4800d1b93a959ba35f3fd7e829 Mon Sep 17 00:00:00 2001
From: Vladimir Pantelic <vladoman@gmail.com>
Date: Wed, 16 Mar 2011 14:53:05 +0100
Subject: [PATCH] revert SVN r9963 and use the ASF preroll value as st->start_time again

when not doing so, even a simple ASF to ASF stream copy results in
an ASF file that has negative time stamps which is not allowed.
---
 libavformat/asfdec.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/libavformat/asfdec.c b/libavformat/asfdec.c
index cbcd576..ff1f166 100644
--- a/libavformat/asfdec.c
+++ b/libavformat/asfdec.c
@@ -218,7 +218,7 @@  static int asf_read_stream_properties(AVFormatContext *s, int64_t size)
     int type_specific_size, sizeX;
     uint64_t total_size;
     unsigned int tag1;
-    int64_t pos1, pos2, start_time;
+    int64_t pos1, pos2;
     int test_for_ext_stream_audio, is_dvr_ms_audio=0;
 
     if (s->nb_streams == ASF_MAX_STREAMS) {
@@ -236,13 +236,13 @@  static int asf_read_stream_properties(AVFormatContext *s, int64_t size)
     if (!asf_st)
         return AVERROR(ENOMEM);
     st->priv_data = asf_st;
-    start_time = asf->hdr.preroll;
+    st->start_time = asf->hdr.preroll;
 
     asf_st->stream_language_index = 128; // invalid stream index means no language info
 
     if(!(asf->hdr.flags & 0x01)) { // if we aren't streaming...
         st->duration = asf->hdr.play_time /
-            (10000000 / 1000) - start_time;
+            (10000000 / 1000) - st->start_time;
     }
     ff_get_guid(pb, &g);
 
-- 
1.6.0.2