[FFmpeg-devel] [PATCH 12/12] avformat/nutenc: Fix repeating headers

[Andreas Rheinhardt]

Michael Niedermayer:
> On Tue, May 05, 2020 at 04:16:57PM +0200, Andreas Rheinhardt wrote:
>> Commit 14b3f9961f3cf9173c43c936eb0cfb5a35fb8419 introduced header
>> repetition (used to improve resilience) in the NUT muxer; a variable
>> header_count was added for this to the NUTContext. It contained the
>> number of times the header has already been sent.
>>
>> Yet a few months later a712d725c0d466cc3672d626883319ef828ca8d6 and
>> 3b4f69ae8ceac45dd815d26e17d83a7dda4c4057 were merged which added support
>> for header elision in the NUT muxer resp. demuxer. Both of these commits
>> used the header_count variable to denote the number of stored headers.
>> This effectively initialized header_count to seven; it was still
>> incremented when writing a header (in particular, it was eight after
>> writing the first header at the beginning of the file), yet given that
>> it started from such a high level the first repeated header would only
>> be inserted after 16TiB.
>>
>> (The NUTContext supports up to 128 stored headers in order to satisfy
>> the needs of the demuxer (which shares this context with the muxer).
>> If the muxer had its own context that only contained seven entries for
>> headers, then this would have been very dangerous, because after writing
>> the initial header the number of actual stored headers and header_count
>> differ.)
>>
>> This commit fixes this by separating the two variables. The number of
>> written headers is now stored in header_written.
>>
>> Given that NUT output is used in a large number of FATE tests that have
>> nothing to do with NUT (like the filter-pixdesc tests that use NUT with
>> the md5 output protocol) a large number of tests had to be updated. The
>> only difference is that now repeated headers are written when writing
>> the trailer. Notice that the part of the tests that actually test the
>> file's contents did not need to be modified as seen with the acodec-pcm
>> tests.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>> ---
>> This commit is simply about fixing the regression caused by a712d725 and
> 
>> restoring the previous behaviour. In particular, it potentially repeats
>> the header twice when writing the trailer, although I don't know why.
> 
> if there is damage to one of 2 headers it is not neccessarily always clear
> what is damaged, with 3 one can simply take the majority.
> 
Makes sense.
> 
>> I am also unsure why the size difference between successive headers
>> grows exponentially.
> 
> I think i dont understand what you say here.
> The size of each header should not grow exponentially relativly to the
> previous
> 
I did not mean the size of the header, but the position/offsets of the
headers. I specifically meant this line:
if (1LL << (20 + 3 * nut->header_count) <= avio_tell(bc))
which in its current form that the first repeated header will be written
after 16 TiB, then another one after 128 TiB (i.e. after further 112
TiB) and so on.

> 
>>
>> And maybe we should stop using NUT in all these tests that have nothing
>> to do with NUT.
> 
> One advantage of NUT is that we can extend it whenever we need. So theres
> never a feature we couldnt test with nut.
> That makes nut a simple choice to use universally when the container is
> not the article of the test itself
> 
Maybe a hash muxer would be better suited for this; but unfortunately
none of the current versions seem to be suited for the task: The
framehash/crc muxers generate too much output; the other muxers ignore
too much of the input (they only take the packet's data into account,
ignoring extradata, side data, timestamps etc.).

- Andreas