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

Stefano Sabatini stefano.sabatini-lala
Sun Dec 19 20:43:16 CET 2010


On date Saturday 2010-12-18 14:07:47 +0100, Michael Niedermayer encoded:
> On Sat, Dec 18, 2010 at 11:46:16AM +0100, Stefano Sabatini wrote:
> > 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...
> 
> To me it looks like its bigger than needed and messy if not buggy
> 
> 
> > 
> > > > 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
> 
> ok, so your mail starts to make more sense to me
> 
> 
> > 
> > > >    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.
> 
> yes, so far so good, makes sense and i agree, we dont want that
> 
> 
> >  
> > > 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).
> 
> ok so far
> 
> 
> > 
> > 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.
> 
> No, vflip flips the linesize.
> This fliped buffer is passed on by start_frame to the next filter and if that
> doesnt support it the existing code copies it
> The previous filter is not affected by start_frame messing with line sizes,
> It cant be affected because that would be a bug. A filter can have a static
> buffer that it passes on and it can alraedy have rendered into this buffer
> theres no way start_frame could tell the previous filter render fliped at that
> time. Only though get_video_buffer() can teh previous filter be brought to
> render upside down.
> 
> Thus if the previous filter uses get_video_buffer() it receives frames with
> negative linesize (if supported) and start_frame then also receives these
> buffers with negative linesize and it changes them to positive by fliping
> linesize and vissual image
> If OTOH the previous filter does not use get_video_buffer() or negative
> linesizes arent supported somewhere along the path then start_frame gets
> positive linesize frames and flips them to negative and (its the identical
> operation as previous) if now a downstream filter cant handle negative linesizes
> it gets copied before in that filters start_frame

OK, this explanation was useful, thanks.

Now I still can't find a way to avoid the copy in the filter, anyway
the new approach is indeed much simpler.
-- 
FFmpeg = Foolish and Fast Marvellous Pure Elastic God



More information about the ffmpeg-devel mailing list