use ASF preroll as st->start_time

Message ID 4D80CD61.7050709@gmail.com
State Committed
Headers show

Commit Message

Vladimir Pantelic March 16, 2011, 2:46 p.m.
Måns Rullgård wrote:

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

it is already added back in the muxer, that part is untouched. I see other
demuxers using start_time for the same purpose so I did not make an ASF
specific thing.

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

attached.

Comments

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

> Måns Rullgård wrote:
>
>>>  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.
>
> it is already added back in the muxer, that part is untouched. I see other
> demuxers using start_time for the same purpose so I did not make an ASF
> specific thing.
>
>> 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.
>
> attached.
>
>
> From 008d5ca03e4b725b8e04433da4f54cfaf1ebd0d2 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 +++---
>  libavformat/avformat.h |    2 --
>  2 files changed, 3 insertions(+), 5 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);
>  
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index ce50053..ca179d2 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -534,8 +534,6 @@ typedef struct AVStream {
>       * 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;
>  
> -- 
> 1.6.0.2

Looks good to me.
Vladimir Pantelic March 16, 2011, 8:23 p.m. | #2
Måns Rullgård wrote:
> Vladimir Pantelic<vladoman@gmail.com>  writes:
>
>>  Måns Rullgård wrote:
>>
>>>>   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.
>>
>>  it is already added back in the muxer, that part is untouched. I see other
>>  demuxers using start_time for the same purpose so I did not make an ASF
>>  specific thing.
>>
>>>  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.
>>
>>  attached.
>>
>>
>>   From 008d5ca03e4b725b8e04433da4f54cfaf1ebd0d2 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 +++---
>>   libavformat/avformat.h |    2 --
>>   2 files changed, 3 insertions(+), 5 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);
>>
>>  diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>>  index ce50053..ca179d2 100644
>>  --- a/libavformat/avformat.h
>>  +++ b/libavformat/avformat.h
>>  @@ -534,8 +534,6 @@ typedef struct AVStream {
>>        * 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;
>>
>>  --
>>  1.6.0.2
>
> Looks good to me.

no, please do not queue for now!!

Patch

From 008d5ca03e4b725b8e04433da4f54cfaf1ebd0d2 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 +++---
 libavformat/avformat.h |    2 --
 2 files changed, 3 insertions(+), 5 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);
 
diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index ce50053..ca179d2 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -534,8 +534,6 @@  typedef struct AVStream {
      * 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;
 
-- 
1.6.0.2