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

[Martin Storsjö]

On Fri, 3 Jan 2020, Marton Balint wrote:

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

Hmm, not really

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

Ah, I overlooked that - then it might be fine.

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

Yeah, I guess so.


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

Yes, that would be better indeed. For the case with movenc, I think I 
think I might have taken this route in order to reduce the amount of 
needed refactoring as this method felt good enough for the cases at hand 
at the time.

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

Awesome, that'd make me more comfortable with it!

// Martin