[FFmpeg-devel] [PATCH 2/3] avformat/avienc: add reserve_index_space option

[Tobias Rapp]

On 31.01.2017 04:33, Michael Niedermayer wrote:
> On Mon, Jan 30, 2017 at 04:40:17PM +0100, Tobias Rapp wrote:
>> On 25.01.2017 22:49, Michael Niedermayer wrote:
>>> On Mon, Jan 23, 2017 at 11:12:13AM +0100, Tobias Rapp wrote:
>>> [...]
>>>> libavformat/avi.h                       |    1
>>>> libavformat/avienc.c                    |   77 +++++++++++++++++++++++++++++---
>>>> libavformat/version.h                   |    2
>>>> tests/ref/fate/mpeg4-bsf-unpack-bframes |    2
>>>> tests/ref/lavf-fate/avi_cram            |    2
>>>> 5 files changed, 74 insertions(+), 10 deletions(-)
>>>> 8dfa5b4ff26ee67c6772bb257a772672bb91aa34  0002-avformat-avienc-add-reserve_index_space-option.patch
>>> >From 4cc70ffdeb7eeb60e7bbb725bd5885dcacf968d2 Mon Sep 17 00:00:00 2001
>>>> From: Tobias Rapp <t.rapp at noa-archive.com>
>>>> Date: Wed, 4 Jan 2017 15:31:29 +0100
>>>> Subject: [PATCH 2/3] avformat/avienc: add reserve_index_space option
>>>>
>>>> Allows the user to reserve space for the ODML master index. A sufficient
>>>> sized master index in the AVI header avoids storing follow-up master
>>>> indexes within the 'movi' data later.
>>>>
>>>> If the option is omitted or zero the index size is estimated from output
>>>> duration and bitrate. A worst-case bitrate for video streams is assumed
>>>> in case it is not available.
>>>>
>>>
>>>> Note: fate reference files changed because the video stream had zero
>>>> bitrate before and is guessed now.
>>>
>>> I dont think this is good
>>> if the user app says bitrate is 0 the muxer should not replace that by
>>> the bitrate for a rawvideo stream (when its not rawvideo)
>>>
>>> This could be ok for the reserved space calculation but for the file
>>> bitrate, especially with lossy compressed streams this will likely
>>> give values that are less correct than before
>>
>
>> My assumption was that bitrate=0 corresponds to "unknown" and that a
>> large value for MaxBytesPerSec would do less harm than a small/zero
>> value.
>
> That can be argued about but that should not be in this patch, changing
> some bitrates in the header and changing the index space are 2
> differnt thigs

I agree. And it would be even better if no guessing would be needed on 
muxer level at all because even loss-less encoders would signal the 
estimated stream bitrate.

>> Find attached an updated version of the patch with "worst-case
>> bitrate guessing" removed.
>
> patch LGTM

Pushed, thanks for the review.

Regards,
Tobias