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

[Stefano Sabatini]

On date Thursday 2010-12-02 23:03:45 +0100, Michael Niedermayer encoded:
> On Thu, Dec 02, 2010 at 05:13:51AM -0800, Jason Garrett-Glaser wrote:
> > On Thu, Dec 2, 2010 at 4:53 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > > On Wed, Dec 01, 2010 at 08:41:38PM -0800, Baptiste Coudurier wrote:
> > >> On 12/1/10 4:53 PM, Jason Garrett-Glaser wrote:
> > >> > On Tue, Nov 30, 2010 at 2:26 PM, Stefano Sabatini
> > >> > <stefano.sabatini-lala at poste.it> wrote:
> > >> >> On date Saturday 2010-11-27 16:53:51 +0100, Stefano Sabatini encoded:
> > >> >>> On date Saturday 2010-11-06 18:21:55 +0100, Stefano Sabatini encoded:
> > >> >>>> On date Saturday 2010-11-06 18:10:04 +0100, Stefano Sabatini encoded:
> > >> >>>>> Hi,
> > >> >>>>>
> > >> >>>>> as you may know the command:
> > >> >>>>> ffplay INPUT -vf vflip
> > >> >>>>>
> > >> >>>>> crashes with many video codecs. This is a regression introduced by the
> > >> >>>>> direct rendering feature, since the codec request the frame to use for
> > >> >>>>> putting the decoded frame, it gets a frame with negative linesizes and
> > >> >>>>> crash
> > >> >>>>>
> > >> >>>>> (BTW the smacker regression also seems to depend on diect
> > >> >>>>> rendering, and precisely with the way the palette is initialized in
> > >> >>>>> avfilter_default_get_buffer, which doesn't use ff_systematic_pal()).
> > >> >>>>>
> > >> >>>>> A possible first step would be to define a CODEC_CAP_NEG_LINESIZES
> > >> >>>>> capability (suggest a better name), and set it in all the codecs which
> > >> >>>>> currently support this feature (I have no idea which of them, do
> > >> >>>>> you?).
> > >> >>>>>
> > >> >>>>> At this point I see two solutions. One solution would be to change
> > >> >>>>> get_video_buffer(), and make it invert the buffer when it detects the
> > >> >>>>> negative linesizes && the NEG_LINESIZES capability is not
> > >> >>>>> supported, *or* auto-add another filter just before the ffplay source.
> > >> >>>>>
> > >> >>>>> Such a filter (vflipfix - suggest better name) would work as a null
> > >> >>>>> filter if the frame is not inverted, and would readjust the frame if
> > >> >>>>> the linesizes are inverted.
> > >> >>>>>
> > >> >>>>> The second solution seems simpler and cleaner.
> > >> >>>>
> > >> >>>> To make it even more useful, we may add a capability to the filters,
> > >> >>>> and auto-add the vflip-fix filter when building the filterchain, and
> > >> >>>> fix all the filters which doesn't support negative linesizes.
> > >> >>>
> > >> >>> Patchset attached.
> > >> >>
> > >> >> Ping.
> > >> >
> > >> > I don't think this extra complexity is a good idea. ?I'd rather just
> > >> > modify vflip to do things the slow way (it's NOT an important filter)
> > >> > and just officially drop support for negative linesizes.
> > >> >
> > >> > This extra complexity would be worth it if it affected any use-case
> > >> > that matters. ?It doesn't.
> > >>
> > >> Well, I tend to agree that I like the idea of dropping negative
> > >> linesizes, it has always caused more problems than it fixed IMHO.
> > >
> > > libmpcodecs supports negative linesize
> > > and code crashing with negative linesize is a bug even if we dont support
> > > negative linesizes
> > > also the bugs that have been mentioned above are said to be in codecs.
> > > droping negative linesize in codecs requires a major bump and changes in
> > > applications (mplayer at least).
> > >
> > > also while iam just mildly against droping negative linesizes iam very strongly
> > > against brushing bugs under the carpet without understanding the bugs first
> > > aka id like to know which codec crashes and id like to have backtraces and know
> > > why before droping a random feature that someone claims is the cause of the
> > > crash
> > 
> > Much assembly code doesn't support negative linesizes on 64-bit
> > because it doesn't do a movsxd on the stride.
> 
> ok, that is an argument
> 
> though even if we drop negative linesize support some specific parts of code
> will need to continue to support them, like the rotate/flip/transpose code or
> some codecs which store images upside down.

Note that the ff_avfilter_readjust_graph() is needed anyway for other
things (fifo auto-insertion in case of split, scale auto-insertion for
filters which don't support re-configuration etc.), so the only added
complexity in the lavfi filtering is given by the auto-inserted
vflipfix filter.
-- 
FFmpeg = Fostering and Forgiving Magic Programmable Elected Gadget