[FFmpeg-devel] Understanding lavfi's permissions system (was: lavfi/audio: fix size of copied samples.)

[Michael Niedermayer]

On Fri, Jul 27, 2012 at 07:17:40PM +0200, Nicolas George wrote:
> Le decadi 10 thermidor, an CCXX, Michael Niedermayer a écrit :
> > > The permissions that a filter has on the buffer pointed by a reference are
> > > exactly the permissions it asked for: the value of min_perms if the buffer
> > > comes from an input, the second argument of ff_get_<type>_buffer if the
> > > buffer is just created. The perms field in the structure are only there to
> > > help lavfi keep track of when a copy is needed.
> > this is not how the API was or is supposed to work nor is it what
> > is implemented even with all the bugs that are there
> 
> I am very surprised to read that. I do not see any filter checking the
> permissions, they just assume they have what they asked for.

what i meant was that a filter is guranteed to get the permissions it
asks for but it can get more permissions and if it does it has every
right to use the additional permissions.


> 
> > and it causes issues as you yourself describe below
> 
> Any solution with the current data structures will cause the same problems,
> I'll explain below.
> 
> > my question is still open, where is the problem with the permissions
> > as i interpret them ?
> 
> After more careful examination, I believe we are both right: the thing is
> that PRESERVE and WRITE are currently completely redundant, because all
> non-buggy filters that want PRESERVE also do not give WRITE to the next and
> all non-buggy filters that need WRITE reject PRESERVE.
> 
> >		       Permissions should only be droped when theres
> > need to drop them.
> 
> Why? My intuition with permissions is quite the contrary: permissions should
> be kept at minimum and dropped as soon as conveniently possible.

because if a reference has too few permissions it has to be copied
if they are droped as early as possibly this leads to more copying

consider a filter that needs just read permissions if you drop write
you will always have to copy after the filter if the next needs
write permissions. If you dont drop the write permissions you dont
need to copy



> 
> > heres a patch to fix this
> 
> That will not work, as  you pointed out.

it fixes the min/rej permissions, there sure are more bugs in there


> 
> > rereading and rethinking this, i see a shortcomming of the current API
> > (any interpretation of it probably) with fps+fifo+drawtext
> > fps would have to drop write because it keeps a preserve buffer for
> > duplication while after fifo there may be individual references
> > that are unshared and could be drawn into saftely by drawtext while
> > they lost the write rights
> > This would indeed need a API change (write reference counting for
> > example)
> > without the fifo though at the time drawtext gets the buffer, either
> > theres a reference still in fps so a copy will be needed or if no
> > reference is kept in fps then theres also no need for fps to have
> > droped write rights.
> 
> The problem I am thinking of is not there. Consider this filter graph:
> 
> fps needs to be sure that the frame it is currently duplicating is not being
> written by anyone; in other words, it needs a writer's lock. Now, let us
> look at the bigger picture:
> 
>                   ,-> showinfo -> X -> out1
>     in -> split -<
>                   `-> showinfo -> fps -> showinfo -> drawtext -> out2
> 
> We already know that a copy will happen in drawtext (except for the last
> copy of a frame, but that is an optimisation), so the question is: what does
> X? If X writes in the buffer (maybe ass), a copy is necessary, if not (maybe
> scale), it is not necessary.
> 
> The problem is that fps can not know what X does: they have each a reference
> on the same buffer, but the references themselves are totally unrelated.
> Since the permissions live in the reference, there is no communication
> possible.

The problem you describe sounds like a description of the bugs in the
filters not like any issue with the API

let us wildly assume "in" produces a buffer with Read+Write+Preserve
but you can pick anything.
split now has a varity of choices, it could copy the buffer and
pass 2 seperate buffers with R+W+P on or it could drop Write from
both and pass 2 R+P reference pointing to the same buffer on.
It also could do less symetric things but lets ignore that ATM
You seem to assume that split has the right to pass a 2
read+write+preserve refs pointing to the same buffer to the 2 outputs
thats not allowed, it would be a direct violation of the definition
of the permissions, namely that Preserve on A implicates no other
reference with write permission exists.
Back to the example, if 2 seperate buffers are passed on no problem
could arrise, OTOH if 2 read+preserve buffers are passed on then fps
does indeed know what not only X but anyone does to the buffer, namely
due to Preserve being on it noone else can write and X would need to
copy implicitly or explicitly if it wants to write to it.

I hope this clarifies how the API is supposed to work


> 
> And you are right, the solution would certainly involve counting the number
> of read and write references.

write references would allow avoiding some copies in some corner cases
they would be a API change and may add some moderate burden on the
person doing merges from libav.

So what do you suggest we do to move forward here ?
Iam happy to fix the filters to how the current API is defined (which
is likely not how most filters implement it) and continue to do
merges from libav.
do you have some other suggestion ?

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

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- 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/20120727/16175714/attachment.asc>