[FFmpeg-devel] Buffers Threads and refcounts

[Michael Niedermayer]

On Wed, Jan 09, 2013 at 01:30:37PM +0100, Hendrik Leppkes wrote:
> On Wed, Jan 9, 2013 at 12:01 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > Hi all
> >
> > The fork plans to make some changes on how various buffers work, that
> > is adding thread safe ref counting and doing all kinds of related and
> > unrelated changes.
> > They plan to stash this 5000+ lines diff changing 300+ files into
> > few commits, to quote:
> > "The changes are split into per-codec patches to simplify
> >  review, but will be squashed on push."
> >
> > See:
> > http://lists.libav.org/pipermail/libav-devel/2013-January/041437.html
> > http://lists.libav.org/pipermail/libav-devel/2013-January/041447.html
> > http://lists.libav.org/pipermail/libav-devel/2013-January/041576.html
> >
> > This quite large (and ineviable quite buggy) changeset will cause
> > various problems
> > * Moving AVFrame to libavutil causes ABI/API issues. Currently
> >   if a new field is needed its just added and used, after the move
> >   it will be needed to keep libavcodec, libavutil, libavfilter and
> >   other users of AVFrame in sync or take extra care on accessing some
> >   fields that might not have existed in other libs when they where
> >   build.
> > * The patchsets remove various buffer pools which will cause
> >   significant speedloss.
> >   It should be possible to reimplement these pools though
> > * Interfacing with any application that doesnt have refcounted frames
> >   will likely need a memcpy.
> > * Various fields are removed from public API, these will break
> >   usage of libavcodec with libpostproc, any reuse of motion vectors
> >   or macroblock types on reencoding and probably all attempts to
> >   extract statistics about the internals of a video codec, short of
> >   parsing av_log() debug output.
> >
> > Above issues will hit libav as well as ffmpeg if we merge the patches.
> > ffmpeg will be hit a bit harder because we have more optimizations
> > that are incompatible with the refcounted buffers.
> >
> > The advantages of this patchset are
> > 1. It simplifies the code, this is true, but this is
> >    just because optimizations, features and not understood bugfixes
> >    are removed. The rest is moving compexity around more than removing
> >    it. And i doubt the code will be any simpler once all bugs are
> >    fixed and once it runs at the same speed again.
> >    (also its 5926 insertions(+), 5464 deletions(-)
> >     that is 500 lines more than before the patches even with many
> >     optimizations droped)
> > 2. It simplifies the permissions in libavfilter at the expense of
> >    speed. Currently libavfilter (of ffmpeg) supports almost any kind
> >    of buffer, static, read only, .... After the patchset only the most
> >    common kind of buffers are supported, that IS simpler and slower
> >    because if your buffer doesnt fit in the API you need to copy it.
> >    Which way is better, i dont know, but the permission system is
> >    entirely independant of what these patchsets do. Such change should
> >    have been done independantly
> > 3. decoders dont return refcounted buffers, this patchset changes this
> >    making users happier.
> >    The same effect though can be achived with the current API/ABI by
> >    just putting a av_refcounted_get_buffer() inside lavc and the
> >    user app using that.
> >
> > in some cases it will need fewer memcpies in other cases, it will
> > need more. And it will do the thread safe refcounting in all cases
> > and that will slow all cases down, especially small frames like in
> > some audio decoders/demuxers will suffer from this
> >
> > So in summary, these patchsets are bad, really bad in the short term
> > and less so in the long term once all optimizations are put back
> > and in the distant future it might even be an improvment and lead to
> > slightly cleaner code overall.
> >
> > What to do about that ?
> > If we do not merge them then this means an end to API/ABI
> > compatibility with libav, this would be a big annoyance to
> > applications using libav*, thus their oppinon is quite welcome here.
> >
> > If we do merge them this means alot of work, these are ~ 6000 changed
> > lines done on a 2 year outdated fork. A fork missing many codecs,
> > filter and (de)muxers that we do have. The code is buggy on its own
> > and possible a lesser number of bugs will get introduced from the
> > merge
> >
> > These changes should have been done on top of ffmpeg changing all
> > codecs and filters And should have been reviewed by the authors who
> > wrote the code that is being changed. But the authors are unwelcome in
> > libav. The changes also should stay in a branch until all
> > optimizations are back in place and bugs fixed instead of being
> > commited to libav.
> > But i doubt thats how it will be.
> >
> > If we do merge these patchsets, we must be aware that the code will
> > likely be in not so good shape afterwards (nor will libav be in better)
> > and will need time and lots of work.
> > Also merging these patchsets that where worked on since 3+ months by
> > libav developers will not be possible in 1 day, it will take longer
> >
> > Also consider that not merging them and the API/ABI incompatibility
> > would affect distributions and applications. And that in this case
> > There may be a need for volunteers to package ffmpeg for some
> > distributions. (We either way still need volunteers for official
> > debian & ubuntu packages of ffmpeg)
> >
> > My current plan is to attempt to integrate the patchsets once they hit
> > libav, and fix as many of the bugs they contain as i can. But iam
> > surely not opposed to the alternative if ffmpeg developers and
> > application developers support going a different path.
> >
> > Also it would be great if the fork nonsense would end and the libav
> > developers would join ffmpeg and we could all work together making
> > these changes without bugs and without speedloss instead of the
> > duplication of work this now will become ...
> > Also consider, if i wouldnt have had to work on daily merging of
> > libav, we now probably would have a HEVC decoder and a useable AAC
> > encoder but i rather had to spend my time on that. And now i will
> > have to work on integrating and fixing this instead of more usefull
> > things ...
> 
> 
> I won't go into detail on all the pros and cons, however i would like
> to say that refcounted buffers for both AVPacket and AVFrames is
> something that has been bugging me for years. Not knowing the real
> life-time of the buffers in those structs without studying the code
> very carefully seriously limits the flexibility and basically ensures
> that you start memcpy'ing them, so for my use-case, i would save a lot

so
1. Docs are crap
2. Users unneccessarily memcpy just to be sure as the docs dont
   document when its actually needed
3. Isnt it logic to fix the docs here ?


[...]

> 
> Another thing i never understood was the use of a separate frame
> structure for lavfi, which slowly adopted all kinds of fields from
> AVFrame, and if this would continue for much longer, would become
> equally bloated as AVFrames - this change makes the whole system much
> simpler (with only one bloated struct :p)

If you use libavcodec together with libavfilter then the use of the
same struct simplifies your application. If you use only one of them
chances are it will get harder to setup the more generic struct.
At least that was the case with ogg and vorbis libs which used the
same structs.
Depending on what you do and how its implemented either can be better.
If well designed i agree a single structure should be better.


> 
> Personally i fail to see which use-cases actually get slower (possibly
> except the really hacky ones), because the AVBuffer spec from Anton
> i've seen is quite flexible, and could adapt to a lot of weird/hacky
> internal usage.

droped audio buffer pool, speedloss documented in git log

Quoting Anton: "Missing frame buffer reuse in lavc. Preliminary tests
show that this causes a measurable slowdown, so it should be fixed."

overhead for thread sync is not negligible, I benchmarked the
code i saw from libav, id have to redo it though if you want hard
numbers

If you interface with libavcodec/libavfilter and the buffers you have
from whichever platform specific API (de)uxer, (decoder, display, ...
arent refcounted but instead only valid till the next call or return.
Then you do NOT need a copy now in many cases but will need one with
the new API
(at least thats how i understand it, please correct me
 if i missed something)


> 
> PS:
> Access to internal frame data from outside ala postproc is a horrible


> design to begin with, and exposing private frame decoding data just
> for the purpose of statistics or the like also increase overall
> complexity for very niche use-cases only, which is something that made
> ffmpeg suffer greatly in simplicity over the years.

If the existence of a field in AVFrame that you never read, write,
init or other need to care about increases complexity for you.
Then yes

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130109/4fbd4b5f/attachment.asc>