diff mbox series

[FFmpeg-devel,04/20] avformat/matroskaenc: Reuse random seed

Message ID 20200405155928.9323-5-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
This commit reuses the random seed generated in mkv_init() (to determine
the TrackUIDs) for the SegmentUID in order to avoid a potentially
expensive call to av_get_random_seed().

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

Comments

Steve Lhomme April 19, 2020, 7:03 a.m. UTC | #1
Wouldn't it be better to set the AVLFG in the MatroskaMuxContext in mkv_init and keep using it when needed ?

Then you can still update UIDs in the location you really need to create them.

> On April 5, 2020 5:59 PM Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
> 
>  
> This commit reuses the random seed generated in mkv_init() (to determine
> the TrackUIDs) for the SegmentUID in order to avoid a potentially
> expensive call to av_get_random_seed().
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavformat/matroskaenc.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index d2046193ee..b93c50cb01 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -161,6 +161,8 @@ typedef struct MatroskaMuxContext {
>      int wrote_chapters;
>  
>      int allow_raw_vfw;
> +
> +    uint32_t segment_uid[4];
>  } MatroskaMuxContext;
>  
>  /** 2 bytes * 7 for EBML IDs, 7 1-byte EBML lengths, 6 1-byte uint,
> @@ -1821,15 +1823,7 @@ static int mkv_write_header(AVFormatContext *s)
>              put_ebml_string(pb, MATROSKA_ID_WRITINGAPP, LIBAVFORMAT_IDENT);
>  
>          if (mkv->mode != MODE_WEBM) {
> -            uint32_t segment_uid[4];
> -            AVLFG lfg;
> -
> -            av_lfg_init(&lfg, av_get_random_seed());
> -
> -            for (i = 0; i < 4; i++)
> -                segment_uid[i] = av_lfg_get(&lfg);
> -
> -            put_ebml_binary(pb, MATROSKA_ID_SEGMENTUID, segment_uid, 16);
> +            put_ebml_binary(pb, MATROSKA_ID_SEGMENTUID, mkv->segment_uid, 16);
>          }
>      } else {
>          const char *ident = "Lavf";
> @@ -2661,9 +2655,14 @@ static int mkv_init(struct AVFormatContext *s)
>          return AVERROR(ENOMEM);
>      }
>  
> -    if (!(s->flags & AVFMT_FLAG_BITEXACT))
> +    if (!(s->flags & AVFMT_FLAG_BITEXACT)) {
>          av_lfg_init(&c, av_get_random_seed());
>  
> +        // Calculate the SegmentUID now in order not to waste our random seed.
> +        for (i = 0; i < 4; i++)
> +            mkv->segment_uid[i] = av_lfg_get(&c);
> +    }
> +
>      for (i = 0; i < s->nb_streams; i++) {
>          mkv_track *track = &mkv->tracks[i];
>  
> -- 
> 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:11 a.m. UTC | #2
Steve Lhomme:
> Wouldn't it be better to set the AVLFG in the MatroskaMuxContext in mkv_init and keep using it when needed ?
> 
> Then you can still update UIDs in the location you really need to create them.
> 
What other UIDs do I need? UIDs for tracks and attachments are created
in init as is the SegmentUID. The EditionUID is not written as we only
support ordinary chapters for now (FFmpeg does not have nested chapters
or multiple editions or hierarchical metadata -- I really don't know how
this could be supported). And the ChapterUIDs are not created randomly.

- Andreas

>> On April 5, 2020 5:59 PM Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>>
>>  
>> This commit reuses the random seed generated in mkv_init() (to determine
>> the TrackUIDs) for the SegmentUID in order to avoid a potentially
>> expensive call to av_get_random_seed().
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>>  libavformat/matroskaenc.c | 19 +++++++++----------
>>  1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>> index d2046193ee..b93c50cb01 100644
>> --- a/libavformat/matroskaenc.c
>> +++ b/libavformat/matroskaenc.c
>> @@ -161,6 +161,8 @@ typedef struct MatroskaMuxContext {
>>      int wrote_chapters;
>>  
>>      int allow_raw_vfw;
>> +
>> +    uint32_t segment_uid[4];
>>  } MatroskaMuxContext;
>>  
>>  /** 2 bytes * 7 for EBML IDs, 7 1-byte EBML lengths, 6 1-byte uint,
>> @@ -1821,15 +1823,7 @@ static int mkv_write_header(AVFormatContext *s)
>>              put_ebml_string(pb, MATROSKA_ID_WRITINGAPP, LIBAVFORMAT_IDENT);
>>  
>>          if (mkv->mode != MODE_WEBM) {
>> -            uint32_t segment_uid[4];
>> -            AVLFG lfg;
>> -
>> -            av_lfg_init(&lfg, av_get_random_seed());
>> -
>> -            for (i = 0; i < 4; i++)
>> -                segment_uid[i] = av_lfg_get(&lfg);
>> -
>> -            put_ebml_binary(pb, MATROSKA_ID_SEGMENTUID, segment_uid, 16);
>> +            put_ebml_binary(pb, MATROSKA_ID_SEGMENTUID, mkv->segment_uid, 16);
>>          }
>>      } else {
>>          const char *ident = "Lavf";
>> @@ -2661,9 +2655,14 @@ static int mkv_init(struct AVFormatContext *s)
>>          return AVERROR(ENOMEM);
>>      }
>>  
>> -    if (!(s->flags & AVFMT_FLAG_BITEXACT))
>> +    if (!(s->flags & AVFMT_FLAG_BITEXACT)) {
>>          av_lfg_init(&c, av_get_random_seed());
>>  
>> +        // Calculate the SegmentUID now in order not to waste our random seed.
>> +        for (i = 0; i < 4; i++)
>> +            mkv->segment_uid[i] = av_lfg_get(&c);
>> +    }
>> +
>>      for (i = 0; i < s->nb_streams; i++) {
>>          mkv_track *track = &mkv->tracks[i];
>>  
>> -- 
>> 2.20.1
>>
Steve Lhomme April 19, 2020, 8:06 a.m. UTC | #3
> On April 19, 2020 9:11 AM Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
> 
>  
> Steve Lhomme:
> > Wouldn't it be better to set the AVLFG in the MatroskaMuxContext in mkv_init and keep using it when needed ?
> > 
> > Then you can still update UIDs in the location you really need to create them.
> > 
> What other UIDs do I need? UIDs for tracks and attachments are created
> in init as is the SegmentUID. The EditionUID is not written as we only
> support ordinary chapters for now (FFmpeg does not have nested chapters
> or multiple editions or hierarchical metadata -- I really don't know how
> this could be supported). And the ChapterUIDs are not created randomly.

That's a general rule so you don't need to adapt everytime you need a new UIDs. And it's easier/cleaner to generate UIDs when the item they relate to is created.

Anyway, it's not a blocker, just a general remark.

As for other UIDs they should be randomized (hence the Unique in UIDs) unless you're remuxing or taking the values from other sources which is sometimes needed for advanced chapter handling or referencing a part of the same mux via a UID.
 
> - Andreas
> 
> >> On April 5, 2020 5:59 PM Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
> >>
> >>  
> >> This commit reuses the random seed generated in mkv_init() (to determine
> >> the TrackUIDs) for the SegmentUID in order to avoid a potentially
> >> expensive call to av_get_random_seed().
> >>
> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> >> ---
> >>  libavformat/matroskaenc.c | 19 +++++++++----------
> >>  1 file changed, 9 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> >> index d2046193ee..b93c50cb01 100644
> >> --- a/libavformat/matroskaenc.c
> >> +++ b/libavformat/matroskaenc.c
> >> @@ -161,6 +161,8 @@ typedef struct MatroskaMuxContext {
> >>      int wrote_chapters;
> >>  
> >>      int allow_raw_vfw;
> >> +
> >> +    uint32_t segment_uid[4];
> >>  } MatroskaMuxContext;
> >>  
> >>  /** 2 bytes * 7 for EBML IDs, 7 1-byte EBML lengths, 6 1-byte uint,
> >> @@ -1821,15 +1823,7 @@ static int mkv_write_header(AVFormatContext *s)
> >>              put_ebml_string(pb, MATROSKA_ID_WRITINGAPP, LIBAVFORMAT_IDENT);
> >>  
> >>          if (mkv->mode != MODE_WEBM) {
> >> -            uint32_t segment_uid[4];
> >> -            AVLFG lfg;
> >> -
> >> -            av_lfg_init(&lfg, av_get_random_seed());
> >> -
> >> -            for (i = 0; i < 4; i++)
> >> -                segment_uid[i] = av_lfg_get(&lfg);
> >> -
> >> -            put_ebml_binary(pb, MATROSKA_ID_SEGMENTUID, segment_uid, 16);
> >> +            put_ebml_binary(pb, MATROSKA_ID_SEGMENTUID, mkv->segment_uid, 16);
> >>          }
> >>      } else {
> >>          const char *ident = "Lavf";
> >> @@ -2661,9 +2655,14 @@ static int mkv_init(struct AVFormatContext *s)
> >>          return AVERROR(ENOMEM);
> >>      }
> >>  
> >> -    if (!(s->flags & AVFMT_FLAG_BITEXACT))
> >> +    if (!(s->flags & AVFMT_FLAG_BITEXACT)) {
> >>          av_lfg_init(&c, av_get_random_seed());
> >>  
> >> +        // Calculate the SegmentUID now in order not to waste our random seed.
> >> +        for (i = 0; i < 4; i++)
> >> +            mkv->segment_uid[i] = av_lfg_get(&c);
> >> +    }
> >> +
> >>      for (i = 0; i < s->nb_streams; i++) {
> >>          mkv_track *track = &mkv->tracks[i];
> >>  
> >> -- 
> >> 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, 8:30 a.m. UTC | #4
Steve Lhomme:
>> On April 19, 2020 9:11 AM Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>>
>>  
>> Steve Lhomme:
>>> Wouldn't it be better to set the AVLFG in the MatroskaMuxContext in mkv_init and keep using it when needed ?
>>>
>>> Then you can still update UIDs in the location you really need to create them.
>>>
>> What other UIDs do I need? UIDs for tracks and attachments are created
>> in init as is the SegmentUID. The EditionUID is not written as we only
>> support ordinary chapters for now (FFmpeg does not have nested chapters
>> or multiple editions or hierarchical metadata -- I really don't know how
>> this could be supported). And the ChapterUIDs are not created randomly.
> 
> That's a general rule so you don't need to adapt everytime you need a new UIDs. And it's easier/cleaner to generate UIDs when the item they relate to is created.
> 
> Anyway, it's not a blocker, just a general remark.
> 
Several of these patches (including this one) have already been applied;
the Cues patch has not (the reason for this is that I wanted to wait for
this until I have added a test for DASH audio just in order to show that
this patch does not affect the Cues for DASH audio at all).


> As for other UIDs they should be randomized (hence the Unique in UIDs) unless you're remuxing or taking the values from other sources which is sometimes needed for advanced chapter handling or referencing a part of the same mux via a UID.

I know. I will implement preserving the TrackUID during remuxing
(streamcopying) some day. But advanced chapter handling is very far off
given the fundamental differences of FFmpeg's chapter handling and
Matroska's. (The current handling of Tags leaves much to be desired, too.)

- Andreas

>  
>> - Andreas
>>
>>>> On April 5, 2020 5:59 PM Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>>>>
>>>>  
>>>> This commit reuses the random seed generated in mkv_init() (to determine
>>>> the TrackUIDs) for the SegmentUID in order to avoid a potentially
>>>> expensive call to av_get_random_seed().
>>>>
>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>>> ---
>>>>  libavformat/matroskaenc.c | 19 +++++++++----------
>>>>  1 file changed, 9 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>>>> index d2046193ee..b93c50cb01 100644
>>>> --- a/libavformat/matroskaenc.c
>>>> +++ b/libavformat/matroskaenc.c
>>>> @@ -161,6 +161,8 @@ typedef struct MatroskaMuxContext {
>>>>      int wrote_chapters;
>>>>  
>>>>      int allow_raw_vfw;
>>>> +
>>>> +    uint32_t segment_uid[4];
>>>>  } MatroskaMuxContext;
>>>>  
>>>>  /** 2 bytes * 7 for EBML IDs, 7 1-byte EBML lengths, 6 1-byte uint,
>>>> @@ -1821,15 +1823,7 @@ static int mkv_write_header(AVFormatContext *s)
>>>>              put_ebml_string(pb, MATROSKA_ID_WRITINGAPP, LIBAVFORMAT_IDENT);
>>>>  
>>>>          if (mkv->mode != MODE_WEBM) {
>>>> -            uint32_t segment_uid[4];
>>>> -            AVLFG lfg;
>>>> -
>>>> -            av_lfg_init(&lfg, av_get_random_seed());
>>>> -
>>>> -            for (i = 0; i < 4; i++)
>>>> -                segment_uid[i] = av_lfg_get(&lfg);
>>>> -
>>>> -            put_ebml_binary(pb, MATROSKA_ID_SEGMENTUID, segment_uid, 16);
>>>> +            put_ebml_binary(pb, MATROSKA_ID_SEGMENTUID, mkv->segment_uid, 16);
>>>>          }
>>>>      } else {
>>>>          const char *ident = "Lavf";
>>>> @@ -2661,9 +2655,14 @@ static int mkv_init(struct AVFormatContext *s)
>>>>          return AVERROR(ENOMEM);
>>>>      }
>>>>  
>>>> -    if (!(s->flags & AVFMT_FLAG_BITEXACT))
>>>> +    if (!(s->flags & AVFMT_FLAG_BITEXACT)) {
>>>>          av_lfg_init(&c, av_get_random_seed());
>>>>  
>>>> +        // Calculate the SegmentUID now in order not to waste our random seed.
>>>> +        for (i = 0; i < 4; i++)
>>>> +            mkv->segment_uid[i] = av_lfg_get(&c);
>>>> +    }
>>>> +
>>>>      for (i = 0; i < s->nb_streams; i++) {
>>>>          mkv_track *track = &mkv->tracks[i];
>>>>  
>>>> -- 
>>>> 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 d2046193ee..b93c50cb01 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -161,6 +161,8 @@  typedef struct MatroskaMuxContext {
     int wrote_chapters;
 
     int allow_raw_vfw;
+
+    uint32_t segment_uid[4];
 } MatroskaMuxContext;
 
 /** 2 bytes * 7 for EBML IDs, 7 1-byte EBML lengths, 6 1-byte uint,
@@ -1821,15 +1823,7 @@  static int mkv_write_header(AVFormatContext *s)
             put_ebml_string(pb, MATROSKA_ID_WRITINGAPP, LIBAVFORMAT_IDENT);
 
         if (mkv->mode != MODE_WEBM) {
-            uint32_t segment_uid[4];
-            AVLFG lfg;
-
-            av_lfg_init(&lfg, av_get_random_seed());
-
-            for (i = 0; i < 4; i++)
-                segment_uid[i] = av_lfg_get(&lfg);
-
-            put_ebml_binary(pb, MATROSKA_ID_SEGMENTUID, segment_uid, 16);
+            put_ebml_binary(pb, MATROSKA_ID_SEGMENTUID, mkv->segment_uid, 16);
         }
     } else {
         const char *ident = "Lavf";
@@ -2661,9 +2655,14 @@  static int mkv_init(struct AVFormatContext *s)
         return AVERROR(ENOMEM);
     }
 
-    if (!(s->flags & AVFMT_FLAG_BITEXACT))
+    if (!(s->flags & AVFMT_FLAG_BITEXACT)) {
         av_lfg_init(&c, av_get_random_seed());
 
+        // Calculate the SegmentUID now in order not to waste our random seed.
+        for (i = 0; i < 4; i++)
+            mkv->segment_uid[i] = av_lfg_get(&c);
+    }
+
     for (i = 0; i < s->nb_streams; i++) {
         mkv_track *track = &mkv->tracks[i];