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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
