diff mbox series

[FFmpeg-devel,1/4] avformat/nutenc: don't use header_count to store different variables

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
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished

Commit Message

Andriy Gelman Nov. 8, 2020, 7:50 p.m. UTC
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(-)

Comments

Andreas Rheinhardt Nov. 8, 2020, 11:04 p.m. UTC | #1
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.
Andriy Gelman Nov. 8, 2020, 11:49 p.m. UTC | #2
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."
Andreas Rheinhardt Nov. 10, 2020, 9:45 p.m. UTC | #3
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 mbox series

Patch

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