| Message ID | 20201108195043.210799-1-andriy.gelman@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avformat/nutenc: don't use header_count to store different variables | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
Andriy Gelman: > From: Andriy Gelman <andriy.gelman@gmail.com> > > Currently, header_count is used to store both the elision header count > and the header repetition count (number of times headers have been written > to output). Fix this by using a separate variable to store repetition > count. > > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> > --- > libavformat/nut.h | 3 ++- > libavformat/nutenc.c | 4 ++-- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/libavformat/nut.h b/libavformat/nut.h > index a4409ee23d..a990d3832e 100644 > --- a/libavformat/nut.h > +++ b/libavformat/nut.h > @@ -103,7 +103,8 @@ typedef struct NUTContext { > unsigned int time_base_count; > int64_t last_syncpoint_pos; > int64_t last_resync_pos; > - int header_count; > + int header_count; // elision header count > + int header_rep_count; // number of times headers written > AVRational *time_base; > struct AVTreeNode *syncpoints; > int sp_count; > diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c > index 1dcb2be1b1..87adef6f7e 100644 > --- a/libavformat/nutenc.c > +++ b/libavformat/nutenc.c > @@ -684,7 +684,7 @@ static int write_headers(AVFormatContext *avctx, AVIOContext *bc) > } > > nut->last_syncpoint_pos = INT_MIN; > - nut->header_count++; > + nut->header_rep_count++; > > ret = 0; > fail: > @@ -988,7 +988,7 @@ static int nut_write_packet(AVFormatContext *s, AVPacket *pkt) > data_size += sm_size; > } > > - if (1LL << (20 + 3 * nut->header_count) <= avio_tell(bc)) > + if (1LL << (20 + 3 * nut->header_rep_count) <= avio_tell(bc)) > write_headers(s, bc); > > if (key_frame && !(nus->last_flags & FLAG_KEY)) > 1. You are not the first to notice this: https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200505141657.10787-2-andreas.rheinhardt@gmail.com/ 2. Your patch is incomplete: header_count is also used in write_trailer. If you use the correct value there, you will have to update lots of fate-tests (see the above commit). (The fate updates of that old patch need to be updated (and were incomplete anyway because I did not ran tests for gpl-components*). 3. The reason the above patch (or rather patchset) has not been applied is that there is actually a subtle bug with chapters: Users are allowed to add new chapters after writing the header (some muxer then write the chapters when writing the trailer). Yet this doesn't work with nut right now: See the commit message of the preceding patch (https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200505141657.10787-1-andreas.rheinhardt@gmail.com/) for details. - Andreas *: I only noticed this through patchwork, so thank you again for this.
On Mon, 09. Nov 00:04, Andreas Rheinhardt wrote: > Andriy Gelman: > > From: Andriy Gelman <andriy.gelman@gmail.com> > > > > Currently, header_count is used to store both the elision header count > > and the header repetition count (number of times headers have been written > > to output). Fix this by using a separate variable to store repetition > > count. > > > > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> > > --- > > libavformat/nut.h | 3 ++- > > libavformat/nutenc.c | 4 ++-- > > 2 files changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/libavformat/nut.h b/libavformat/nut.h > > index a4409ee23d..a990d3832e 100644 > > --- a/libavformat/nut.h > > +++ b/libavformat/nut.h > > @@ -103,7 +103,8 @@ typedef struct NUTContext { > > unsigned int time_base_count; > > int64_t last_syncpoint_pos; > > int64_t last_resync_pos; > > - int header_count; > > + int header_count; // elision header count > > + int header_rep_count; // number of times headers written > > AVRational *time_base; > > struct AVTreeNode *syncpoints; > > int sp_count; > > diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c > > index 1dcb2be1b1..87adef6f7e 100644 > > --- a/libavformat/nutenc.c > > +++ b/libavformat/nutenc.c > > @@ -684,7 +684,7 @@ static int write_headers(AVFormatContext *avctx, AVIOContext *bc) > > } > > > > nut->last_syncpoint_pos = INT_MIN; > > - nut->header_count++; > > + nut->header_rep_count++; > > > > ret = 0; > > fail: > > @@ -988,7 +988,7 @@ static int nut_write_packet(AVFormatContext *s, AVPacket *pkt) > > data_size += sm_size; > > } > > > > - if (1LL << (20 + 3 * nut->header_count) <= avio_tell(bc)) > > + if (1LL << (20 + 3 * nut->header_rep_count) <= avio_tell(bc)) > > write_headers(s, bc); > > > > if (key_frame && !(nus->last_flags & FLAG_KEY)) > > > 1. You are not the first to notice this: > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200505141657.10787-2-andreas.rheinhardt@gmail.com/ The goal of my set was to help user in [1] by adding option to insert periodic headers (see patch 3). http://ffmpeg.org/pipermail/ffmpeg-user/2020-November/050690.html > 2. Your patch is incomplete: header_count is also used in write_trailer. > If you use the correct value there, you will have to update lots of > fate-tests (see the above commit). (The fate updates of that old patch > need to be updated (and were incomplete anyway because I did not ran > tests for gpl-components*). It has been dead code since the header elision support was added. Would it make sense to remove that code in write_trailer()? This would simplify your patch. > 3. The reason the above patch (or rather patchset) has not been applied > is that there is actually a subtle bug with chapters: Users are allowed > to add new chapters after writing the header (some muxer then write the > chapters when writing the trailer). Yet this doesn't work with nut right > now: See the commit message of the preceding patch > (https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200505141657.10787-1-andreas.rheinhardt@gmail.com/) > for details. I'd have to look at your set in more detail, but I thought repeated headers had to be the same as original ones. From the spec: "Headers may be repeated, but if they are, then all mandatory headers MUST be repeated and repeated headers MUST be identical."
Andriy Gelman: > On Mon, 09. Nov 00:04, Andreas Rheinhardt wrote: >> Andriy Gelman: >>> From: Andriy Gelman <andriy.gelman@gmail.com> >>> >>> Currently, header_count is used to store both the elision header count >>> and the header repetition count (number of times headers have been written >>> to output). Fix this by using a separate variable to store repetition >>> count. >>> >>> Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> >>> --- >>> libavformat/nut.h | 3 ++- >>> libavformat/nutenc.c | 4 ++-- >>> 2 files changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/libavformat/nut.h b/libavformat/nut.h >>> index a4409ee23d..a990d3832e 100644 >>> --- a/libavformat/nut.h >>> +++ b/libavformat/nut.h >>> @@ -103,7 +103,8 @@ typedef struct NUTContext { >>> unsigned int time_base_count; >>> int64_t last_syncpoint_pos; >>> int64_t last_resync_pos; >>> - int header_count; >>> + int header_count; // elision header count >>> + int header_rep_count; // number of times headers written >>> AVRational *time_base; >>> struct AVTreeNode *syncpoints; >>> int sp_count; >>> diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c >>> index 1dcb2be1b1..87adef6f7e 100644 >>> --- a/libavformat/nutenc.c >>> +++ b/libavformat/nutenc.c >>> @@ -684,7 +684,7 @@ static int write_headers(AVFormatContext *avctx, AVIOContext *bc) >>> } >>> >>> nut->last_syncpoint_pos = INT_MIN; >>> - nut->header_count++; >>> + nut->header_rep_count++; >>> >>> ret = 0; >>> fail: >>> @@ -988,7 +988,7 @@ static int nut_write_packet(AVFormatContext *s, AVPacket *pkt) >>> data_size += sm_size; >>> } >>> >>> - if (1LL << (20 + 3 * nut->header_count) <= avio_tell(bc)) >>> + if (1LL << (20 + 3 * nut->header_rep_count) <= avio_tell(bc)) >>> write_headers(s, bc); >>> >>> if (key_frame && !(nus->last_flags & FLAG_KEY)) >>> >> 1. You are not the first to notice this: >> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200505141657.10787-2-andreas.rheinhardt@gmail.com/ > > The goal of my set was to help user in [1] by adding option to insert periodic > headers (see patch 3). > http://ffmpeg.org/pipermail/ffmpeg-user/2020-November/050690.html > >> 2. Your patch is incomplete: header_count is also used in write_trailer. >> If you use the correct value there, you will have to update lots of >> fate-tests (see the above commit). (The fate updates of that old patch >> need to be updated (and were incomplete anyway because I did not ran >> tests for gpl-components*). > > It has been dead code since the header elision support was added. > Would it make sense to remove that code in write_trailer()? This would simplify > your patch. > It would simplify the patch, but it is not how it is supposed to be. >> 3. The reason the above patch (or rather patchset) has not been applied >> is that there is actually a subtle bug with chapters: Users are allowed >> to add new chapters after writing the header (some muxer then write the >> chapters when writing the trailer). Yet this doesn't work with nut right >> now: See the commit message of the preceding patch >> (https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200505141657.10787-1-andreas.rheinhardt@gmail.com/) >> for details. > > I'd have to look at your set in more detail, but I thought repeated headers had > to be the same as original ones. From the spec: > > "Headers may be repeated, but if they are, then all mandatory headers MUST be > repeated and repeated headers MUST be identical." > For the specs, chapters are info_packets and not part of a mandatory header, so the above is no restriction. - Andreas
diff --git a/libavformat/nut.h b/libavformat/nut.h index a4409ee23d..a990d3832e 100644 --- a/libavformat/nut.h +++ b/libavformat/nut.h @@ -103,7 +103,8 @@ typedef struct NUTContext { unsigned int time_base_count; int64_t last_syncpoint_pos; int64_t last_resync_pos; - int header_count; + int header_count; // elision header count + int header_rep_count; // number of times headers written AVRational *time_base; struct AVTreeNode *syncpoints; int sp_count; diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c index 1dcb2be1b1..87adef6f7e 100644 --- a/libavformat/nutenc.c +++ b/libavformat/nutenc.c @@ -684,7 +684,7 @@ static int write_headers(AVFormatContext *avctx, AVIOContext *bc) } nut->last_syncpoint_pos = INT_MIN; - nut->header_count++; + nut->header_rep_count++; ret = 0; fail: @@ -988,7 +988,7 @@ static int nut_write_packet(AVFormatContext *s, AVPacket *pkt) data_size += sm_size; } - if (1LL << (20 + 3 * nut->header_count) <= avio_tell(bc)) + if (1LL << (20 + 3 * nut->header_rep_count) <= avio_tell(bc)) write_headers(s, bc); if (key_frame && !(nus->last_flags & FLAG_KEY))