[FFmpeg-devel] [PATCH 2/3] avformat: remove more unneeded avio_flush() calls

[Marton Balint]

On Fri, 3 Jan 2020, Martin Storsjö wrote:

> On Fri, 3 Jan 2020, Marton Balint wrote:
>
>> Throughout libavformat there are lots of avio_flush() calls which are 
> unneeded:
>> - Instances found at the end of write_header, write_packet and 
> write_trailer
>>  callbacks. These are handled by the generic code in libavformat/mux.c.
>
> They are only handled by the generic code, if the flush_packets flag is 
> set. If it isn't, and the flush was there for a reason (I'm sure not all 
> of them are, but I'm also quite sure a few of them are there for very 
> specific reasons), things will now break.

For write_packet, you are right, it removes the explicit flush and fall 
backs to the default behaviour of the flush_packets flag, which is to 
flush if the output is streamed, and not flush otherwise. Can you a think 
of a case which breaks when the output is not streamed?

For write_header there is an explicit flush in avio_write_marker if the 
marker type is AVIO_DATA_MARKER_HEADER and if the type is different to the 
existing marker, so it should not make a difference.

>
> One such case that you're removing comes from 
> 8ad5124b7ecf7f727724e270a7b4bb8c7bcbf6a4 which was added for a specific 
> reason.

I guess this predates your avio_write_marker addition.

>
>> - Instances in the middle of write_header and write_trailer which are
>>  redundant or are present becuase avio_flush() used to be required before
>>  doing a seekback. That is no longer the case, aviobuf code does the flush
>>  automatically on seek.
>
> It's not necessarily about flushing before doing seekback, it's also about 
> ensuring that seekback can be done within the current buffer.
>
> For streaming output (where we can't seek back once data has been 
> flushed), it's cruicial to flush _before_ a point you want to seek to. 
> Consider if we have a 32k avio buffer, and it's filled up to 30k at the 
> moment. We're going to write a 8k structure which requires seeks back and 
> forth. If we remove the pre-write-flush, we'll write 2k of data, flush it 
> out, and later seeks to the beginning of this block will fail.
>
> If we explicitly flush before starting to write one block where we know 
> we'll need to seek to, we maximize the chances of it working. (If we need 
> to seek across a larger area than the avio buffer, then it still won't 
> work of course.)
>
> I didn't check yet all of the ones you are removing, but I'd say at least 
> that movenc.c has cases of intentional flushing in this style.

Ok, this can be a problem indeed. A proper solution would be to use a 
dynbuf in these cases if streaming is of importance because it is wrong to 
assume any particular output buffer size in a muxer and rely on this 
behaviour.

>> So this patch removes those cases. Removing explicit avio_flush() calls 
> helps
>> us to buffer more data and avoid flushing the IO context too often which 
> causes
>> reduced IO throughput for non-streamed file output.
>
> So if you're arguing that some can be removed because the generic code in 
> mux.c does the same (although it only does so if a nondefault flag is 
> set?), this benefit can only be attributed to the other ones, that are 
> removed from the middle of functions.

Yes. I will rework the patchset, sepearte the cases better and maybe keep 
the ones in the middle write_header/write_trailer for now.

Thanks,
Marton