[FFmpeg-devel] [PATCH 01/20] avformat/matroskaenc: Ensure that ChapterUID are != 0

[Steve Lhomme]

> On April 5, 2020 5:59 PM Andreas Rheinhardt <andreas.rheinhardt at gmail.com> wrote:
> 
>  
> AVChapters have an int as id field and therefore this value can appear
> <= 0. When remuxing from Matroska, this value actually contains
> the lower 32 bits of the original ChapterUID (which can be 64 bits).
> 
> In order to ensure that the ChapterUID is always > 0, they were offset
> as follows (since 07704c61): First max(0, 1LL - chapter[i].id) was computed
> and stored in an uint32_t. And then the IDs were offset using this value.
> 
> This has two downsides:
> 1. It does not ensure that the UID is actually != 0: Namely if there is
> a chapter with id == INT_MIN, then the offset will be 2^31 + 1 and a
> chapter with id == INT_MAX will become 2^31 - 1 + 2^31 + 1 = 2^32 = 0,
> because the actual calculation was performed in 32 bits.
> 2. As soon as a chapter id appears to be negative, a nontrivial offset
> is used, so that not even a ChapterUID that only uses 32 bits is
> preserved.
> 
> So change this by treating the id as an unsigned value internally and
> only offset (by 1) if an id vanishes. The actual offsetting then has to
> be performed in 64 bits in order to make sure that no UINT32_MAX wraps
> around.

That means you are changing the chapter UIDs of the source when remuxing (if for some reason a chapter with no id was added in the process). If tags were referencing the chapter UIDs (TagChapterUID) they don't match the chapter anymore.

I think silently changing the IDs is wrong. But its was already like that before, so that's not breaking your patch. If anything your patch is less likely to break remuxing.

> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
>  libavformat/matroskaenc.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 060e8b7816..a377d092df 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -1422,7 +1422,8 @@ static int mkv_write_chapters(AVFormatContext *s)
>          }
>  
>          chapteratom = start_ebml_master(dyn_cp, MATROSKA_ID_CHAPTERATOM, 0);
> -        put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERUID, c->id + mkv->chapter_id_offset);
> +        put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERUID,
> +                      (uint32_t)c->id + (uint64_t)mkv->chapter_id_offset);
>          put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERTIMESTART, chapterstart);
>          put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERTIMEEND, chapterend);
>          if (mkv->mode != MODE_WEBM) {
> @@ -1479,7 +1480,7 @@ static int mkv_write_simpletag(AVIOContext *pb, AVDictionaryEntry *t)
>  }
>  
>  static int mkv_write_tag_targets(AVFormatContext *s, uint32_t elementid,
> -                                 unsigned int uid, ebml_master *tag)
> +                                 uint64_t uid, ebml_master *tag)
>  {
>      AVIOContext *pb;
>      MatroskaMuxContext *mkv = s->priv_data;
> @@ -1518,7 +1519,7 @@ static int mkv_check_tag_name(const char *name, uint32_t elementid)
>  }
>  
>  static int mkv_write_tag(AVFormatContext *s, AVDictionary *m, uint32_t elementid,
> -                         unsigned int uid)
> +                         uint64_t uid)
>  {
>      MatroskaMuxContext *mkv = s->priv_data;
>      ebml_master tag;
> @@ -1612,7 +1613,8 @@ static int mkv_write_tags(AVFormatContext *s)
>              if (!mkv_check_tag(ch->metadata, MATROSKA_ID_TAGTARGETS_CHAPTERUID))
>                  continue;
>  
> -            ret = mkv_write_tag(s, ch->metadata, MATROSKA_ID_TAGTARGETS_CHAPTERUID, ch->id + mkv->chapter_id_offset);
> +            ret = mkv_write_tag(s, ch->metadata, MATROSKA_ID_TAGTARGETS_CHAPTERUID,
> +                                (uint32_t)ch->id + (uint64_t)mkv->chapter_id_offset);
>              if (ret < 0)
>                  return ret;
>          }
> @@ -1882,7 +1884,10 @@ static int mkv_write_header(AVFormatContext *s)
>          return ret;
>  
>      for (i = 0; i < s->nb_chapters; i++)
> -        mkv->chapter_id_offset = FFMAX(mkv->chapter_id_offset, 1LL - s->chapters[i]->id);
> +        if (!s->chapters[i]->id) {
> +            mkv->chapter_id_offset = 1;
> +            break;
> +        }
>  
>      ret = mkv_write_chapters(s);
>      if (ret < 0)
> -- 
> 2.20.1
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".