| Message ID | 20200405155928.9323-5-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Matroska muxer patches | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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".
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 >>
> 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".
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 --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];
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(-)