diff mbox series

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

Message ID 20200405155928.9323-2-andreas.rheinhardt@gmail.com
State Accepted
Headers show
Series Matroska muxer patches
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt April 5, 2020, 3:59 p.m. UTC
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.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/matroskaenc.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Steve Lhomme April 19, 2020, 6:52 a.m. UTC | #1
> On April 5, 2020 5:59 PM Andreas Rheinhardt <andreas.rheinhardt@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@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Andreas Rheinhardt April 19, 2020, 7:02 a.m. UTC | #2
Steve Lhomme:
>> On April 5, 2020 5:59 PM Andreas Rheinhardt <andreas.rheinhardt@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.
> 
The tags are (and were) also offset so that they still match the
chapter. That's also why I had to adapt mkv_write_tag().

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

The problem here is that the chapter id field is only 32bits, whereas
Matroska UIDs are 64bit. With my patch the lower 32bits are preserved
(provided that none of them are zero). The earlier version treated the
lower 32bits as a signed number in itself and offset it in order to make
it > 0 (and there was an edge case (see commit description) where it
nevertheless could turn out as zero).

- Andreas

> 
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
diff mbox series

Patch

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)