diff mbox series

[FFmpeg-devel,2/2] avformat: Make AVChapter.id an int64_t on next major bump

Message ID 20210316082953.273550-2-andreas.rheinhardt@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,1/2] avformat/matroskaenc: Check chapter ids for duplicates
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt March 16, 2021, 8:29 a.m. UTC
64 bits are needed in order to retain the uid values of Matroska
chapters; the type is kept signed because the semantics of NUT chapters
depend upon whether the id is > 0 or < 0.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
Apologies for being so late.

 doc/APIchanges            | 4 ++++
 libavformat/aadec.c       | 2 +-
 libavformat/avformat.h    | 4 ++++
 libavformat/internal.h    | 4 ++++
 libavformat/matroskaenc.c | 4 ++++
 libavformat/nutdec.c      | 4 ++--
 libavformat/utils.c       | 4 ++++
 libavformat/version.h     | 5 ++++-
 8 files changed, 27 insertions(+), 4 deletions(-)

Comments

Anton Khirnov March 16, 2021, 9:20 a.m. UTC | #1
Quoting Andreas Rheinhardt (2021-03-16 09:29:53)
> 64 bits are needed in order to retain the uid values of Matroska
> chapters; the type is kept signed because the semantics of NUT chapters
> depend upon whether the id is > 0 or < 0.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> Apologies for being so late.
> 
>  doc/APIchanges            | 4 ++++
>  libavformat/aadec.c       | 2 +-
>  libavformat/avformat.h    | 4 ++++
>  libavformat/internal.h    | 4 ++++
>  libavformat/matroskaenc.c | 4 ++++
>  libavformat/nutdec.c      | 4 ++--
>  libavformat/utils.c       | 4 ++++
>  libavformat/version.h     | 5 ++++-
>  8 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index c0d955b1fa..8b93adebe1 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
>  
>  API changes, most recent first:
>  
> +2021-03-16 - xxxxxxxxxx - lavf 58.75.100  - avformat.h
> +  AVChapter.id will be changed from int to int64_t
> +  on the next major version bump.
> +
>  2021-03-12 - xxxxxxxxxx - lavc 58.131.100 - avcodec.h codec.h
>    Add a get_encode_buffer callback to AVCodecContext, similar to
>    get_buffer2 but for encoders.
> diff --git a/libavformat/aadec.c b/libavformat/aadec.c
> index e88cdb89df..80ca2c12d7 100644
> --- a/libavformat/aadec.c
> +++ b/libavformat/aadec.c
> @@ -222,7 +222,7 @@ static int aa_read_header(AVFormatContext *s)
>      c->content_end = start + largest_size;
>  
>      while ((chapter_pos = avio_tell(pb)) >= 0 && chapter_pos < c->content_end) {
> -        int chapter_idx = s->nb_chapters;
> +        unsigned chapter_idx = s->nb_chapters;

unrelated?
Andreas Rheinhardt March 16, 2021, 9:34 a.m. UTC | #2
Anton Khirnov:
> Quoting Andreas Rheinhardt (2021-03-16 09:29:53)
>> 64 bits are needed in order to retain the uid values of Matroska
>> chapters; the type is kept signed because the semantics of NUT chapters
>> depend upon whether the id is > 0 or < 0.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>> Apologies for being so late.
>>
>>  doc/APIchanges            | 4 ++++
>>  libavformat/aadec.c       | 2 +-
>>  libavformat/avformat.h    | 4 ++++
>>  libavformat/internal.h    | 4 ++++
>>  libavformat/matroskaenc.c | 4 ++++
>>  libavformat/nutdec.c      | 4 ++--
>>  libavformat/utils.c       | 4 ++++
>>  libavformat/version.h     | 5 ++++-
>>  8 files changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index c0d955b1fa..8b93adebe1 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
>>  
>>  API changes, most recent first:
>>  
>> +2021-03-16 - xxxxxxxxxx - lavf 58.75.100  - avformat.h
>> +  AVChapter.id will be changed from int to int64_t
>> +  on the next major version bump.
>> +
>>  2021-03-12 - xxxxxxxxxx - lavc 58.131.100 - avcodec.h codec.h
>>    Add a get_encode_buffer callback to AVCodecContext, similar to
>>    get_buffer2 but for encoders.
>> diff --git a/libavformat/aadec.c b/libavformat/aadec.c
>> index e88cdb89df..80ca2c12d7 100644
>> --- a/libavformat/aadec.c
>> +++ b/libavformat/aadec.c
>> @@ -222,7 +222,7 @@ static int aa_read_header(AVFormatContext *s)
>>      c->content_end = start + largest_size;
>>  
>>      while ((chapter_pos = avio_tell(pb)) >= 0 && chapter_pos < c->content_end) {
>> -        int chapter_idx = s->nb_chapters;
>> +        unsigned chapter_idx = s->nb_chapters;
> 
> unrelated?
> 
The chapter ids created by aadec are just 0,1,... And in the
hypothetical scenario that there are more than INT_MAX of them the
INT_MAX+1U chapter would have a negative id if I didn't change it to
unsigned above. (I don't think it can happen, but nb_chapters is an
unsigned, so it is better anyway.)

- Andreas
Anton Khirnov March 16, 2021, 10:06 a.m. UTC | #3
Quoting Andreas Rheinhardt (2021-03-16 10:34:22)
> Anton Khirnov:
> > Quoting Andreas Rheinhardt (2021-03-16 09:29:53)
> >> 64 bits are needed in order to retain the uid values of Matroska
> >> chapters; the type is kept signed because the semantics of NUT chapters
> >> depend upon whether the id is > 0 or < 0.
> >>
> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> >> ---
> >> Apologies for being so late.
> >>
> >>  doc/APIchanges            | 4 ++++
> >>  libavformat/aadec.c       | 2 +-
> >>  libavformat/avformat.h    | 4 ++++
> >>  libavformat/internal.h    | 4 ++++
> >>  libavformat/matroskaenc.c | 4 ++++
> >>  libavformat/nutdec.c      | 4 ++--
> >>  libavformat/utils.c       | 4 ++++
> >>  libavformat/version.h     | 5 ++++-
> >>  8 files changed, 27 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/doc/APIchanges b/doc/APIchanges
> >> index c0d955b1fa..8b93adebe1 100644
> >> --- a/doc/APIchanges
> >> +++ b/doc/APIchanges
> >> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
> >>  
> >>  API changes, most recent first:
> >>  
> >> +2021-03-16 - xxxxxxxxxx - lavf 58.75.100  - avformat.h
> >> +  AVChapter.id will be changed from int to int64_t
> >> +  on the next major version bump.
> >> +
> >>  2021-03-12 - xxxxxxxxxx - lavc 58.131.100 - avcodec.h codec.h
> >>    Add a get_encode_buffer callback to AVCodecContext, similar to
> >>    get_buffer2 but for encoders.
> >> diff --git a/libavformat/aadec.c b/libavformat/aadec.c
> >> index e88cdb89df..80ca2c12d7 100644
> >> --- a/libavformat/aadec.c
> >> +++ b/libavformat/aadec.c
> >> @@ -222,7 +222,7 @@ static int aa_read_header(AVFormatContext *s)
> >>      c->content_end = start + largest_size;
> >>  
> >>      while ((chapter_pos = avio_tell(pb)) >= 0 && chapter_pos < c->content_end) {
> >> -        int chapter_idx = s->nb_chapters;
> >> +        unsigned chapter_idx = s->nb_chapters;
> > 
> > unrelated?
> > 
> The chapter ids created by aadec are just 0,1,... And in the
> hypothetical scenario that there are more than INT_MAX of them the
> INT_MAX+1U chapter would have a negative id if I didn't change it to
> unsigned above. (I don't think it can happen, but nb_chapters is an
> unsigned, so it is better anyway.)

Right, ok then.
Andreas Rheinhardt March 17, 2021, 7:29 p.m. UTC | #4
Andreas Rheinhardt:
> 64 bits are needed in order to retain the uid values of Matroska
> chapters; the type is kept signed because the semantics of NUT chapters
> depend upon whether the id is > 0 or < 0.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> Apologies for being so late.
> 
>  doc/APIchanges            | 4 ++++
>  libavformat/aadec.c       | 2 +-
>  libavformat/avformat.h    | 4 ++++
>  libavformat/internal.h    | 4 ++++
>  libavformat/matroskaenc.c | 4 ++++
>  libavformat/nutdec.c      | 4 ++--
>  libavformat/utils.c       | 4 ++++
>  libavformat/version.h     | 5 ++++-
>  8 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index c0d955b1fa..8b93adebe1 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
>  
>  API changes, most recent first:
>  
> +2021-03-16 - xxxxxxxxxx - lavf 58.75.100  - avformat.h
> +  AVChapter.id will be changed from int to int64_t
> +  on the next major version bump.
> +
>  2021-03-12 - xxxxxxxxxx - lavc 58.131.100 - avcodec.h codec.h
>    Add a get_encode_buffer callback to AVCodecContext, similar to
>    get_buffer2 but for encoders.
> diff --git a/libavformat/aadec.c b/libavformat/aadec.c
> index e88cdb89df..80ca2c12d7 100644
> --- a/libavformat/aadec.c
> +++ b/libavformat/aadec.c
> @@ -222,7 +222,7 @@ static int aa_read_header(AVFormatContext *s)
>      c->content_end = start + largest_size;
>  
>      while ((chapter_pos = avio_tell(pb)) >= 0 && chapter_pos < c->content_end) {
> -        int chapter_idx = s->nb_chapters;
> +        unsigned chapter_idx = s->nb_chapters;
>          uint32_t chapter_size = avio_rb32(pb);
>          if (chapter_size == 0 || avio_feof(pb))
>              break;
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index e3bd01ec7f..765bc3b6f5 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -1184,7 +1184,11 @@ typedef struct AVProgram {
>                                           change dynamically at runtime. */
>  
>  typedef struct AVChapter {
> +#if FF_API_CHAPTER_ID_INT
>      int id;                 ///< unique ID to identify the chapter
> +#else
> +    int64_t id;             ///< unique ID to identify the chapter
> +#endif
>      AVRational time_base;   ///< time base in which the start/end timestamps are specified
>      int64_t start, end;     ///< chapter start/end time in time_base units
>      AVDictionary *metadata;
> diff --git a/libavformat/internal.h b/libavformat/internal.h
> index 0ffdc87b6a..df4918e318 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -554,7 +554,11 @@ void ff_configure_buffers_for_index(AVFormatContext *s, int64_t time_tolerance);
>   *
>   * @return AVChapter or NULL on error
>   */
> +#if FF_API_CHAPTER_ID_INT
>  AVChapter *avpriv_new_chapter(AVFormatContext *s, int id, AVRational time_base,
> +#else
> +AVChapter *avpriv_new_chapter(AVFormatContext *s, int64_t id, AVRational time_base,
> +#endif
>                                int64_t start, int64_t end, const char *title);
>  
>  /**
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 8f29d64e72..02e171593e 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -1669,7 +1669,11 @@ static int mkv_write_chapters(AVFormatContext *s)
>          int64_t chapterstart = av_rescale_q(c->start, c->time_base, scale);
>          int64_t chapterend   = av_rescale_q(c->end,   c->time_base, scale);
>          const AVDictionaryEntry *t;
> +#if FF_API_CHAPTER_ID_INT
>          uint64_t uid = create_new_ids ? i + 1ULL : (uint32_t)c->id;
> +#else
> +        uint64_t uid = create_new_ids ? i + 1ULL : c->id;
> +#endif
>          if (chapterstart < 0 || chapterstart > chapterend || chapterend < 0) {
>              av_log(s, AV_LOG_ERROR,
>                     "Invalid chapter start (%"PRId64") or end (%"PRId64").\n",
> diff --git a/libavformat/nutdec.c b/libavformat/nutdec.c
> index ebb062377d..d1f3496990 100644
> --- a/libavformat/nutdec.c
> +++ b/libavformat/nutdec.c
> @@ -489,8 +489,8 @@ static int decode_info_header(NUTContext *nut)
>      AVIOContext *bc    = s->pb;
>      uint64_t tmp, chapter_start, chapter_len;
>      unsigned int stream_id_plus1, count;
> -    int chapter_id, i, ret = 0;
> -    int64_t value, end;
> +    int i, ret = 0;
> +    int64_t chapter_id, value, end;
>      char name[256], str_value[1024], type_str[256];
>      const char *type;
>      int *event_flags        = NULL;
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 8573117694..8fb5dbbf9d 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -4611,7 +4611,11 @@ AVProgram *av_new_program(AVFormatContext *ac, int id)
>      return program;
>  }
>  
> +#if FF_API_CHAPTER_ID_INT
>  AVChapter *avpriv_new_chapter(AVFormatContext *s, int id, AVRational time_base,
> +#else
> +AVChapter *avpriv_new_chapter(AVFormatContext *s, int64_t id, AVRational time_base,
> +#endif
>                                int64_t start, int64_t end, const char *title)
>  {
>      AVChapter *chapter = NULL;
> diff --git a/libavformat/version.h b/libavformat/version.h
> index e43754b069..ca1cd1a920 100644
> --- a/libavformat/version.h
> +++ b/libavformat/version.h
> @@ -32,7 +32,7 @@
>  // Major bumping may affect Ticket5467, 5421, 5451(compatibility with Chromium)
>  // Also please add any ticket numbers that you believe might be affected here
>  #define LIBAVFORMAT_VERSION_MAJOR  58
> -#define LIBAVFORMAT_VERSION_MINOR  74
> +#define LIBAVFORMAT_VERSION_MINOR  75
>  #define LIBAVFORMAT_VERSION_MICRO 100
>  
>  #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
> @@ -109,6 +109,9 @@
>  #ifndef FF_API_DEMUXER_OPEN
>  #define FF_API_DEMUXER_OPEN             (LIBAVFORMAT_VERSION_MAJOR < 59)
>  #endif
> +#ifndef FF_API_CHAPTER_ID_INT
> +#define FF_API_CHAPTER_ID_INT           (LIBAVFORMAT_VERSION_MAJOR < 59)
> +#endif
>  #ifndef FF_API_LAVF_PRIV_OPT
>  #define FF_API_LAVF_PRIV_OPT            (LIBAVFORMAT_VERSION_MAJOR < 60)
>  #endif
> 
I'll apply this tomorrow unless there are objections.

- Andreas
diff mbox series

Patch

diff --git a/doc/APIchanges b/doc/APIchanges
index c0d955b1fa..8b93adebe1 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,10 @@  libavutil:     2017-10-21
 
 API changes, most recent first:
 
+2021-03-16 - xxxxxxxxxx - lavf 58.75.100  - avformat.h
+  AVChapter.id will be changed from int to int64_t
+  on the next major version bump.
+
 2021-03-12 - xxxxxxxxxx - lavc 58.131.100 - avcodec.h codec.h
   Add a get_encode_buffer callback to AVCodecContext, similar to
   get_buffer2 but for encoders.
diff --git a/libavformat/aadec.c b/libavformat/aadec.c
index e88cdb89df..80ca2c12d7 100644
--- a/libavformat/aadec.c
+++ b/libavformat/aadec.c
@@ -222,7 +222,7 @@  static int aa_read_header(AVFormatContext *s)
     c->content_end = start + largest_size;
 
     while ((chapter_pos = avio_tell(pb)) >= 0 && chapter_pos < c->content_end) {
-        int chapter_idx = s->nb_chapters;
+        unsigned chapter_idx = s->nb_chapters;
         uint32_t chapter_size = avio_rb32(pb);
         if (chapter_size == 0 || avio_feof(pb))
             break;
diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index e3bd01ec7f..765bc3b6f5 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -1184,7 +1184,11 @@  typedef struct AVProgram {
                                          change dynamically at runtime. */
 
 typedef struct AVChapter {
+#if FF_API_CHAPTER_ID_INT
     int id;                 ///< unique ID to identify the chapter
+#else
+    int64_t id;             ///< unique ID to identify the chapter
+#endif
     AVRational time_base;   ///< time base in which the start/end timestamps are specified
     int64_t start, end;     ///< chapter start/end time in time_base units
     AVDictionary *metadata;
diff --git a/libavformat/internal.h b/libavformat/internal.h
index 0ffdc87b6a..df4918e318 100644
--- a/libavformat/internal.h
+++ b/libavformat/internal.h
@@ -554,7 +554,11 @@  void ff_configure_buffers_for_index(AVFormatContext *s, int64_t time_tolerance);
  *
  * @return AVChapter or NULL on error
  */
+#if FF_API_CHAPTER_ID_INT
 AVChapter *avpriv_new_chapter(AVFormatContext *s, int id, AVRational time_base,
+#else
+AVChapter *avpriv_new_chapter(AVFormatContext *s, int64_t id, AVRational time_base,
+#endif
                               int64_t start, int64_t end, const char *title);
 
 /**
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 8f29d64e72..02e171593e 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -1669,7 +1669,11 @@  static int mkv_write_chapters(AVFormatContext *s)
         int64_t chapterstart = av_rescale_q(c->start, c->time_base, scale);
         int64_t chapterend   = av_rescale_q(c->end,   c->time_base, scale);
         const AVDictionaryEntry *t;
+#if FF_API_CHAPTER_ID_INT
         uint64_t uid = create_new_ids ? i + 1ULL : (uint32_t)c->id;
+#else
+        uint64_t uid = create_new_ids ? i + 1ULL : c->id;
+#endif
         if (chapterstart < 0 || chapterstart > chapterend || chapterend < 0) {
             av_log(s, AV_LOG_ERROR,
                    "Invalid chapter start (%"PRId64") or end (%"PRId64").\n",
diff --git a/libavformat/nutdec.c b/libavformat/nutdec.c
index ebb062377d..d1f3496990 100644
--- a/libavformat/nutdec.c
+++ b/libavformat/nutdec.c
@@ -489,8 +489,8 @@  static int decode_info_header(NUTContext *nut)
     AVIOContext *bc    = s->pb;
     uint64_t tmp, chapter_start, chapter_len;
     unsigned int stream_id_plus1, count;
-    int chapter_id, i, ret = 0;
-    int64_t value, end;
+    int i, ret = 0;
+    int64_t chapter_id, value, end;
     char name[256], str_value[1024], type_str[256];
     const char *type;
     int *event_flags        = NULL;
diff --git a/libavformat/utils.c b/libavformat/utils.c
index 8573117694..8fb5dbbf9d 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -4611,7 +4611,11 @@  AVProgram *av_new_program(AVFormatContext *ac, int id)
     return program;
 }
 
+#if FF_API_CHAPTER_ID_INT
 AVChapter *avpriv_new_chapter(AVFormatContext *s, int id, AVRational time_base,
+#else
+AVChapter *avpriv_new_chapter(AVFormatContext *s, int64_t id, AVRational time_base,
+#endif
                               int64_t start, int64_t end, const char *title)
 {
     AVChapter *chapter = NULL;
diff --git a/libavformat/version.h b/libavformat/version.h
index e43754b069..ca1cd1a920 100644
--- a/libavformat/version.h
+++ b/libavformat/version.h
@@ -32,7 +32,7 @@ 
 // Major bumping may affect Ticket5467, 5421, 5451(compatibility with Chromium)
 // Also please add any ticket numbers that you believe might be affected here
 #define LIBAVFORMAT_VERSION_MAJOR  58
-#define LIBAVFORMAT_VERSION_MINOR  74
+#define LIBAVFORMAT_VERSION_MINOR  75
 #define LIBAVFORMAT_VERSION_MICRO 100
 
 #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
@@ -109,6 +109,9 @@ 
 #ifndef FF_API_DEMUXER_OPEN
 #define FF_API_DEMUXER_OPEN             (LIBAVFORMAT_VERSION_MAJOR < 59)
 #endif
+#ifndef FF_API_CHAPTER_ID_INT
+#define FF_API_CHAPTER_ID_INT           (LIBAVFORMAT_VERSION_MAJOR < 59)
+#endif
 #ifndef FF_API_LAVF_PRIV_OPT
 #define FF_API_LAVF_PRIV_OPT            (LIBAVFORMAT_VERSION_MAJOR < 60)
 #endif