[FFmpeg-devel] [RFC] How to fix DR+lavfi+vflip crash

[Stefano Sabatini]

On date Saturday 2010-12-18 02:29:04 +0100, Michael Niedermayer encoded:
> On Fri, Dec 17, 2010 at 12:07:50PM +0100, Stefano Sabatini wrote:
> > On date Wednesday 2010-12-15 13:22:24 +0100, Michael Niedermayer encoded:
> > > On Tue, Dec 14, 2010 at 11:17:47PM +0100, Stefano Sabatini wrote:
> > [...]
> > > > Have another look at my last patch. You see that most code is in the
> > > > vflip draw_slice() function. Now avfilter_draw_slice() has similar
> > > > code which copies the input slice to the output slice when this is
> > > > required for permission reasons.
> > > > 
> > > > Now we could try to move the vflipping code to
> > > > avfilter_draw_slice(). Now this is a problem because this way
> > > > avfilter_draw_slice() has to know *when* to vflip the frame, which is
> > > > not required when the code is in the vflip filter.
> > > 
> > > the permission code in start_frame needs 1 line to be added for the current
> > > copying code in draw_slice to copy the frame.
> > > 
> > > i dont understand the rest of your convoluted ideas.
> > > 
> > > 1. We need copying code if buffer permissions differ, this exists and i assume
> > >    it works, if it doesnt work it needs to be fixed
> > > 2. We need to copy if we have a negative linesize and a filter not supporting
> > >    it
> > > i see nothing in your mails which would even hint on why the permission copy
> > > could not handle the case where linesizes differ
> > 
> > You're confusing different problems.
> 
> If so iam not seeing it, and i fail to be able to see your problem

I could say the same thing, that is I still can't try to understand
what's your problem with my implementation...

> > Here there is a list of some of the lavfi features related to the
> > linesize sign invertion:
> > 
> > 1) having a filter only accept a buffer with non-negative linesizes in
> >    get_video_buffer() (can be done if you don't set
> >    AV_PERM_NEG_LINESIZES in perms, check ffplay.c)
> 
> 
> > 
> > 2) having a filter reject a buffer with positive linesizes in
                                            ^^^^^^^^
Sorry I meant -> negative

> >    start_frame(), and copy it to a newly allocated buffer (can be done
> >    using again the permission system, check the posls test filter)
> 
> rejecting positive linesize, we dont want to do this, posls rejects
> negative linesizes though

yes, as implemented in posls

> > 
> > 3) having a filter explicitely request a buffer with negative
> >    linesizes in get_video_buffer() (cannot be done, it may be done
> >    adding another AV_PERM_POS_LINESIZES flag and changing the
> >    semantics of AV_PERM_NEG_LINESIZES, but I don't think we want this
> >    feature)
> 
> not wanted, agree
> 
> 
> > 
> > 4) having a filter reject a buffer with negative linesizes in
                                            ^^^^^^^^

Here I meant positive (I messed up something after the many re-edits).

> >    start_frame(), and copy it to a newly allocated buffer (cannot be
> >    done, see above)
> 
> What are you talking about?
> There may be a problem but you can copy a buffer whatever linesize in whatever
> function

Yes but with the current scheme you can't make a filter reject a
buffer with positive linesizes (that is you cannot implement negls),
and again I don't think this is a feature that we want to implement.
 
> a few more comments about your code below:
> [...]
> 
> > @@ -63,25 +68,65 @@ static AVFilterBufferRef *get_video_buffer(AVFilterLink *link, int perms,
> >  static void start_frame(AVFilterLink *link, AVFilterBufferRef *picref)
> >  {
> >      FlipContext *flip = link->dst->priv;
> > +    AVFilterBufferRef *picref2 = avfilter_ref_buffer(picref, ~0);
> >      int i;
> >  
> > +    if (!(picref->perms & AV_PERM_NEG_LINESIZES)) {
> > +        picref2->perms |= AV_PERM_NEG_LINESIZES;
> > +        avfilter_default_start_frame(link, picref2);
> > +        return;
> > +    }
> > +
> >      for (i = 0; i < 4; i ++) {
> >          int vsub = i == 1 || i == 2 ? flip->vsub : 0;
> >  
> > -        if (picref->data[i]) {
> > -            picref->data[i] += ((link->h >> vsub)-1) * picref->linesize[i];
> > -            picref->linesize[i] = -picref->linesize[i];
> > +        if (picref2->data[i]) {
> > +            picref2->data[i] += ((link->h >> vsub)-1) * picref->linesize[i];
> > +            picref2->linesize[i] = -picref->linesize[i];
> >          }
> >      }
> >  
> > -    avfilter_start_frame(link->dst->outputs[0], picref);
> > +    avfilter_start_frame(link->dst->outputs[0], picref2);
> >  }
> 
> setting AVFilterBufferRef.perm to or not to AV_PERM_NEG_LINESIZES makes no
> sense, its a piece of memory it can always be referenced both ways.

Let's consider this scenario:

pos_src ----> vflip

pos_src requires a buffer with positive linesizes.  When a buffer is
requested to vflip it won't reverse the linesizes and will allocate a
new buffer and which will have positive linesizes (as in general when
requesting a buffer to the filterchain you have no guarantee that the
returned buffer will have positive linesizes).

Then pos_src invokes start_frame(). Now vflip has to somehow knows if
it can revert the linesizes, or if the buffer was the same one
previously allocated by the vflip filter itself. I'm using the
AV_PERM_NEG_LINESIZES for keeping this bit of info, that means that
in start_frame() the AV_PERM_NEG_LINESIZES bit means:
"the buffer linesizes were inverted".

(Note that in a previous patch I was using a new flag parameter in
start_frame(), which was semantically more clear but was complicating
the API).

The same applies to this other scenario:
src ---> posls ---> vflip

In this case the src buffer request a buffer, which is indeed inverted
by vflip, then posls requests a new non inverted buffer in
start_frame() and send it to vflip, which in this case won't reverse
it again because the sent buffer will have the AV_PERM_NEG_LINESIZES
set to 0.

In other words: the vflip filter works by inverting the requested
buffer in get_video_buffer(), and inverting it back when the same
buffer pass through start_frame(), if the requested buffer linesizes
was not inverted then it has to know this and I'm using the perm
AV_PERM_NEG_LINESIZES flag for storing this information.

If you have suggestions for achieving this result in a cleaner way
they're very welcome.
-- 
FFmpeg = Foolish and Frightening Multimedia Purposeless Encoding/decoding God