[FFmpeg-devel] [Patch] [avfilter] refactor floating point based interpolation and introduce in vf_lenscorrection

Michael Niedermayer michaelni at gmx.at
Wed Aug 20 12:51:49 CEST 2014

On Wed, Aug 20, 2014 at 12:20:06PM +0200, Daniel Oberhoff wrote:
> ---
>  Daniel Oberhoff 
>  daniel.oberhoff at gmail.com
> On Aug 20, 2014, at 12:15 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Wed, Aug 20, 2014 at 11:54:08AM +0200, Daniel Oberhoff wrote:
> >> 
> >> ---
> >> Daniel Oberhoff 
> >> daniel.oberhoff at gmail.com
> >> 
> >> 
> >> 
> >> On Aug 20, 2014, at 11:47 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> 
> >>> On Wed, Aug 20, 2014 at 08:14:31AM +0200, Daniel Oberhoff wrote:
> >>>> 
> >>>> 
> >>>> Von meinem iPhone gesendet
> >>>> 
> >>>>> Am 20.08.2014 um 03:16 schrieb Michael Niedermayer <michaelni at gmx.at>:
> >>>>> 
> >>>>>> On Wed, Aug 20, 2014 at 12:12:49AM +0200, Daniel Oberhoff wrote:
> >>>>>> Hello,
> >>>>>> 
> >>>>>> As a follow-up to my last patch I now factored out the floating point based interpolation from transform.h/transform.c and applied it in the vf_lenscorrection filter I introduced in my last patch. What I did not do is also factor out fixed point based interpolation as used in vf_rotate and vf_perspective and maybe others as could probably also be done. Also I did not look further for uses of floating point based interpolation. Basically I just wanted to introduce interpolation in vf_lenscorrection without code duplication. As a side note I also tried to introduce fixed point calculations to vf_lenscorrection but found myself effectively doing floating point “by hand” since due to the high order of the algorithm (up to 4th order) it is very hard to keep track of the right amount of pre/post-comma digits for a given step in the algorithm and given parameters and it felt rather futile after a while.
> >>>>> 
> >>>>>> Looking forward to reviews :).
> >>>>> 
> >>>>> why did you use the deshake code and not the vf_perspective code ?!
> >>>>> i suspect this will be quite a bit harder to get into shape for a
> >>>>> common API
> >>>>> 
> >>>> 
> >>>> As i Said, perspective does fixed point calculations. Also would you care to ekaborate what exactly are your whoes with this API?
> >>> 
> >>> there where further commments further down in the reply, that list
> >>> some problems
> >>> 
> >>> In addition, being float based is already a problem on its own,
> >>> we dont really want it to be float based. Its very likely slower and
> >>> its not bitexact making tests harder.
> >> 
> >> Well, as I had outlined fixed point is not a viable option for my algorithm.
> > 
> > blending 4 or 16 8bit samples linearly together works fine in fixed
> > point. Please see the vf_perspective code, its exactly doing
> > that.
> > 
> > If you have a problem with fixed point, please show exactly where this
> > problem is and ill try to help
> > 
> > 
> >> 
> >>> besides a API that works 1 sample at a time redundantly recalculates
> >>> the Coefficients for each color plane, also doing 1 sample per call
> >>> isnt very SIMD friendly
> >>> 
> >>> or to see this from another point of view
> >>> what we would like to have is a SSE*/AVX* based generic resampling
> >>> code using fixed point (with 16bit coeffs for 8bit samples for example)
> >>> 
> >>> Theres no need to implement SSE/AVX but the API should be a step
> >>> toward this, and the patch feels like its going the opposit direction
> >>> this is even worse as this is common API, so other filters could
> >>> start using this increasing the amount of code that would eventually
> >>> be changed/updated/rewritten
> >> 
> >> There is some contradiction in your comments. You want to move towards vectorization (btw. how would you do that in ffmpeg? I had previously proposed an sse version using intrinsics and builtin operators, but that was dismissed as not being portable and I was asked to write assembly instead, which I am not proficient in), but you want to do all planes simultanously, even though they are far apart in memory. Imho the gain by being memory local (which is also what you want for vectorization) far outweights the calculation cost for the coefficients.
> > 
> > I think you misunderstand how cpu caches work
> > 
> > theres no reason why reading from 2 or 3 planes sequentially would
> > be significnatly slower
> > also i wonder if this filter shouldnt be working in RGB space.
> Well, that would mean I would have to significantly rewrite my algorithm, leading to the question why this was not brought up before.

The only difference between a packed RGB plane and planar YUV is
the step between pixels, which would be 1 for YUV planar and
3 or 4 for packed RGB

thats not significantly rewriting anything
the whole yuv filter code is
*out++ =  isvalid ? indata[y * inlinesize + x] : 0;

in rgb you do
*out++ =  isvalid ? indata[y * inlinesize + 3*x+0] : 0;
*out++ =  isvalid ? indata[y * inlinesize + 3*x+1] : 0;
*out++ =  isvalid ? indata[y * inlinesize + 3*x+2] : 0;

thats 3 lines

> > camera sensors work in RGB not YUV and correct gamma correction
> > for interpolation also is a RGB space thing.
> > now RGB is generally packed pixel which would avoid the multiple
> > planes
> But the codecs work in YUV planar, and filtering is most efficiently done there, betwen the decoder and the encoder, right? Also modern networked cameras all stream h246 and not packed RGB.

yes, its best to support both, high quality needs RGB, and some
low quality with incorrect gamma interpolation can be done in yuv

> > of course if someone wants to use the filter as a quick correction
> > for a random video downloaded somewhere thats likely in YUV space
> For me going to YUV made everyting around 4x faster..
> Anyhow, this starts to feel a little like “perfect is the enemy of good” and would lead me to drop this refactoring and instead hack interpolation into vf_lenscorrection directly...

what you write sounds a bit like you try to avoid making any
improvments to the filter. Not sure i understand that

Of course its work to add rgb support, of course its work to get
gamma correction right, of course its work to implement a fixed point
bicubic filter, ...
you dont have to do any but it should be kind of a maintainers goal
to improve the code while reading your mails it makes me belive
your goal is to keep the filter as lowest quality as possible ;)

Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140820/ed905b0b/attachment.asc>

More information about the ffmpeg-devel mailing list