| Message ID | 20200505141657.10787-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/10] libavformat/nutenc: Remove redundant function parameter | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Tue, May 05, 2020 at 04:16:56PM +0200, Andreas Rheinhardt wrote: > When writing the header, the NUT muxer allocates an array with as many > entries as there are chapters containing information about the used > timebase. This information is used when writing the headers and also > when resending the headers (as the NUT muxer does from time to time). > > When the NUT muxer writes or resends the headers, it simply presumes > that there are enough entries in its array for each chapter in the > AVFormatContext. Yet users are allowed to add chapters during the muxing > process, so this presumption is wrong and may lead to segfaults. > > So explicitly store the number of entries of the chapter array and refer > to this number whenever headers are written. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > This patch presumes that the user may not change or remove the chapters > available during writing the header (if there were chapters available > when writing the header at all). I hope this is ok. > > libavformat/nut.h | 1 + > libavformat/nutenc.c | 3 ++- > 2 files changed, 3 insertions(+), 1 deletion(-) how do i apply this (for testing) ? on its own it fails and it seems the previous patchset doesnt like applying anymore either > > diff --git a/libavformat/nut.h b/libavformat/nut.h > index a4409ee23d..52225fed93 100644 > --- a/libavformat/nut.h > +++ b/libavformat/nut.h > @@ -115,6 +115,7 @@ typedef struct NUTContext { > int flags; > int version; // version currently in use > int minor_version; > + unsigned nb_chapters; > } NUTContext; > > extern const AVCodecTag ff_nut_subtitle_tags[]; > diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c > index 5071278835..2d35c44b79 100644 > --- a/libavformat/nutenc.c > +++ b/libavformat/nutenc.c > @@ -675,7 +675,7 @@ static int write_headers(AVFormatContext *avctx, AVIOContext *bc) > goto fail; > } > > - for (i = 0; i < nut->avf->nb_chapters; i++) { > + for (i = 0; i < nut->nb_chapters; i++) { > write_chapter(nut, dyn_bc, i, prelude, &prelude_size); also if i read this correctly, this would not write all chapters. That seems not ideal Thanks [...]
Michael Niedermayer: > On Tue, May 05, 2020 at 04:16:56PM +0200, Andreas Rheinhardt wrote: >> When writing the header, the NUT muxer allocates an array with as many >> entries as there are chapters containing information about the used >> timebase. This information is used when writing the headers and also >> when resending the headers (as the NUT muxer does from time to time). >> >> When the NUT muxer writes or resends the headers, it simply presumes >> that there are enough entries in its array for each chapter in the >> AVFormatContext. Yet users are allowed to add chapters during the muxing >> process, so this presumption is wrong and may lead to segfaults. >> >> So explicitly store the number of entries of the chapter array and refer >> to this number whenever headers are written. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> This patch presumes that the user may not change or remove the chapters >> available during writing the header (if there were chapters available >> when writing the header at all). I hope this is ok. >> >> libavformat/nut.h | 1 + >> libavformat/nutenc.c | 3 ++- >> 2 files changed, 3 insertions(+), 1 deletion(-) > > how do i apply this (for testing) ? When you test this, you should be aware that the first time the header is repeated is after 16 TiB of output (see patch #12 for the reason), so you should better change the following line in nut_write_packet() to reduce said interval: if (1LL << (20 + 3 * nut->header_count) <= avio_tell(bc)) > on its own it fails and it seems the previous patchset doesnt like applying > anymore either > I have already applied patches 1-4 + 9 (that you said were ok) from this patchset. Patches 5-8 and 10-11 apply cleanly on top of master. Did you try to also apply the patches that have already been merged? (Patch #12 does not apply cleanly any more, because of the changes to the ref files in b4967fc71c63eae8cd96f9c46cd3e1fbd705bbf9. Furthermore my patch as sent only updated a subset of fate-tests, because my build was not configured with enable-gpl. I already fixed this. Normally I'd resend the mail, but given its huge size I'd like to avoid that this time. Should I send it?) > >> >> diff --git a/libavformat/nut.h b/libavformat/nut.h >> index a4409ee23d..52225fed93 100644 >> --- a/libavformat/nut.h >> +++ b/libavformat/nut.h >> @@ -115,6 +115,7 @@ typedef struct NUTContext { >> int flags; >> int version; // version currently in use >> int minor_version; >> + unsigned nb_chapters; >> } NUTContext; >> >> extern const AVCodecTag ff_nut_subtitle_tags[]; >> diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c >> index 5071278835..2d35c44b79 100644 >> --- a/libavformat/nutenc.c >> +++ b/libavformat/nutenc.c >> @@ -675,7 +675,7 @@ static int write_headers(AVFormatContext *avctx, AVIOContext *bc) >> goto fail; >> } >> >> - for (i = 0; i < nut->avf->nb_chapters; i++) { >> + for (i = 0; i < nut->nb_chapters; i++) { >> write_chapter(nut, dyn_bc, i, prelude, &prelude_size); > > also if i read this correctly, this would not write all chapters. > That seems not ideal It's indeed not ideal; it is designed to fix a potential segfault, not to add functionality. (Without this patch patch #12 would run into this bug more often.) I don't know NUT very well, but I see two ways how this could be implemented: 1. One adds the timebases of these chapters to the arrays of timebases that already exist and writes the chapters with their timebases. This would involve moving code from nut_write_header() to write_headers(). I don't know whether adding a new timebase mid-stream is even allowed by the spec. 2. One writes the chapters with the best timebase available for them, even when it is not an exact match. - Andreas
On Thu, May 07, 2020 at 05:49:06PM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > On Tue, May 05, 2020 at 04:16:56PM +0200, Andreas Rheinhardt wrote: > >> When writing the header, the NUT muxer allocates an array with as many > >> entries as there are chapters containing information about the used > >> timebase. This information is used when writing the headers and also > >> when resending the headers (as the NUT muxer does from time to time). > >> > >> When the NUT muxer writes or resends the headers, it simply presumes > >> that there are enough entries in its array for each chapter in the > >> AVFormatContext. Yet users are allowed to add chapters during the muxing > >> process, so this presumption is wrong and may lead to segfaults. > >> > >> So explicitly store the number of entries of the chapter array and refer > >> to this number whenever headers are written. > >> > >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > >> --- > >> This patch presumes that the user may not change or remove the chapters > >> available during writing the header (if there were chapters available > >> when writing the header at all). I hope this is ok. > >> > >> libavformat/nut.h | 1 + > >> libavformat/nutenc.c | 3 ++- > >> 2 files changed, 3 insertions(+), 1 deletion(-) > > > > how do i apply this (for testing) ? > > When you test this, you should be aware that the first time the header > is repeated is after 16 TiB of output (see patch #12 for the reason), so > you should better change the following line in nut_write_packet() to > reduce said interval: > if (1LL << (20 + 3 * nut->header_count) <= avio_tell(bc)) > > > on its own it fails and it seems the previous patchset doesnt like applying > > anymore either > > > I have already applied patches 1-4 + 9 (that you said were ok) from this > patchset. Patches 5-8 and 10-11 apply cleanly on top of master. Did you > try to also apply the patches that have already been merged? > > (Patch #12 does not apply cleanly any more, because of the changes to > the ref files in b4967fc71c63eae8cd96f9c46cd3e1fbd705bbf9. Furthermore > my patch as sent only updated a subset of fate-tests, because my build > was not configured with enable-gpl. I already fixed this. Normally I'd > resend the mail, but given its huge size I'd like to avoid that this > time. Should I send it?) > > > > >> > >> diff --git a/libavformat/nut.h b/libavformat/nut.h > >> index a4409ee23d..52225fed93 100644 > >> --- a/libavformat/nut.h > >> +++ b/libavformat/nut.h > >> @@ -115,6 +115,7 @@ typedef struct NUTContext { > >> int flags; > >> int version; // version currently in use > >> int minor_version; > >> + unsigned nb_chapters; > >> } NUTContext; > >> > >> extern const AVCodecTag ff_nut_subtitle_tags[]; > >> diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c > >> index 5071278835..2d35c44b79 100644 > >> --- a/libavformat/nutenc.c > >> +++ b/libavformat/nutenc.c > >> @@ -675,7 +675,7 @@ static int write_headers(AVFormatContext *avctx, AVIOContext *bc) > >> goto fail; > >> } > >> > >> - for (i = 0; i < nut->avf->nb_chapters; i++) { > >> + for (i = 0; i < nut->nb_chapters; i++) { > >> write_chapter(nut, dyn_bc, i, prelude, &prelude_size); > > > > also if i read this correctly, this would not write all chapters. > > That seems not ideal > > It's indeed not ideal; it is designed to fix a potential segfault, not > to add functionality. (Without this patch patch #12 would run into this > bug more often.) how hard is it to fix this correctly ? if its really alot of code sure its ok to do a simple fix for something like backporting ... > > I don't know NUT very well, but I see two ways how this could be > implemented: > 1. One adds the timebases of these chapters to the arrays of timebases > that already exist and writes the chapters with their timebases. This > would involve moving code from nut_write_header() to write_headers(). I > don't know whether adding a new timebase mid-stream is even allowed by > the spec. no i dont think you can add timebases mid stream currently.Iam not even sure how much sense that makes when you cannot add streams mid stream > 2. One writes the chapters with the best timebase available for them, > even when it is not an exact match. i would be interrested in cases where an exact match is not available that said i dont care much about what happens when the user provided chapters are not representable in the format. Error out, drop, approximate. thx [...]
diff --git a/libavformat/nut.h b/libavformat/nut.h index a4409ee23d..52225fed93 100644 --- a/libavformat/nut.h +++ b/libavformat/nut.h @@ -115,6 +115,7 @@ typedef struct NUTContext { int flags; int version; // version currently in use int minor_version; + unsigned nb_chapters; } NUTContext; extern const AVCodecTag ff_nut_subtitle_tags[]; diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c index 5071278835..2d35c44b79 100644 --- a/libavformat/nutenc.c +++ b/libavformat/nutenc.c @@ -675,7 +675,7 @@ static int write_headers(AVFormatContext *avctx, AVIOContext *bc) goto fail; } - for (i = 0; i < nut->avf->nb_chapters; i++) { + for (i = 0; i < nut->nb_chapters; i++) { write_chapter(nut, dyn_bc, i, prelude, &prelude_size); ret = put_packet(nut, bc, dyn_bc, prelude, prelude_size, INFO_STARTCODE); if (ret < 0) @@ -719,6 +719,7 @@ static int nut_write_header(AVFormatContext *s) nut->chapter = av_calloc(s->nb_chapters, sizeof(*nut->chapter)); if (!nut->chapter) return AVERROR(ENOMEM); + nut->nb_chapters = s->nb_chapters; } for (i = 0; i < s->nb_streams; i++) {
When writing the header, the NUT muxer allocates an array with as many entries as there are chapters containing information about the used timebase. This information is used when writing the headers and also when resending the headers (as the NUT muxer does from time to time). When the NUT muxer writes or resends the headers, it simply presumes that there are enough entries in its array for each chapter in the AVFormatContext. Yet users are allowed to add chapters during the muxing process, so this presumption is wrong and may lead to segfaults. So explicitly store the number of entries of the chapter array and refer to this number whenever headers are written. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- This patch presumes that the user may not change or remove the chapters available during writing the header (if there were chapters available when writing the header at all). I hope this is ok. libavformat/nut.h | 1 + libavformat/nutenc.c | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-)