[FFmpeg-devel] [PATCH] lavfi: new colorspace conversion filter.

[Clément Bœsch]

On Wed, Apr 06, 2016 at 11:44:22AM -0400, Ronald S. Bultje wrote:
[...]
> > Note: a cleaner way to do this (IMO) is to do put the settings into the
> > template file, and do something like:
> >
> >    #define TEMPLATE_444
> >    #include "colorspacedsp_template.c"
> >    #undef TEMPLATE_444
> >
> >    #define TEMPLATE_422
> >    #include "colorspacedsp_template.c"
> >    #undef TEMPLATE_422
> >
> >    #define TEMPLATE_420
> >    #include "colorspacedsp_template.c"
> >    #undef TEMPLATE_420
> >
> > it's simpler for the caller, and in the template you see the exact
> > settings in use for each "profile".
> >
> > See libswresample/rematrix{,_template}.c for a complete example.
> >
> 
> Hm, I based this off of how lavc/bit_depth_template.c is typically used. I
> guess there's various way to do it. How much do you want me to change it?
> 

The question is: if you ignore the changing effort and the consistency,
which version do you actually prefer? It's your filter so it's up to you.
I just believe this is much more readable that way, but it's an opinion :)

> [...]
> > > +typedef void (*yuv2rgb_fn)(int16_t *rgb[3], ptrdiff_t rgb_stride,
> > > +                           uint8_t *yuv[3], ptrdiff_t yuv_stride[3],
> > > +                           int w, int h, const int16_t
> > yuv2rgb_coeffs[3][3][8],
> > > +                           const int16_t yuv_offset[8]);
> > > +typedef void (*rgb2yuv_fn)(uint8_t *yuv[3], ptrdiff_t yuv_stride[3],
> > > +                           int16_t *rgb[3], ptrdiff_t rgb_stride,
> > > +                           int w, int h, const int16_t
> > rgb2yuv_coeffs[3][3][8],
> > > +                           const int16_t yuv_offset[8]);
> > > +typedef void (*yuv2yuv_fn)(uint8_t *yuv_out[3], ptrdiff_t
> > yuv_out_stride[3],
> > > +                           uint8_t *yuv_in[3], ptrdiff_t
> > yuv_in_stride[3],
> > > +                           int w, int h, const int16_t
> > yuv2yuv_coeffs[3][3][8],
> > > +                           const int16_t yuv_offset[2][8]);
> >
> > I suppose you didn't make use of const for the input because of the
> > pain of the multiple dimensions?
> >
> 
> Right.
> 
> uint8_t * const in[N] doesn't do the trick?
> >
> 
> Well, it's incomplete const, right? I want to indicate that both the plane
> pointers as well as the data in the plane pointers should be considered
> immutable for the dsp function, but as you pointed out, that's not easy
> with C multi-dimensional arrays... I can add half-baked const if you like...
> 

It's fine, I just like to hint about the constness. Knowning that it's not
trashing or permuting the input is kinda helpful in these cases.

[...]
> > > +        yuv0 += (yuv_stride[0] * (1 << SS_H)) / sizeof(pixel);
> > > +        yuv1 += yuv_stride[1] / sizeof(pixel);
> > > +        yuv2 += yuv_stride[2] / sizeof(pixel);
> > > +        rgb0 += rgb_stride * (1 << SS_H);
> > > +        rgb1 += rgb_stride * (1 << SS_H);
> > > +        rgb2 += rgb_stride * (1 << SS_H);
> >
> > i know we will have some asm for this, but compiler will probably like to
> > have these increment variables in some const before the loop instead of
> > computing them every time.
> >
> > I don't think we will have ASM for all the architectures soon so having a
> > fast C is still relevant for such important code.
> >
> 
> But this is used in various lavc templating schemes also, right? These all
> revert back to single instructions, since assembly operates on pointer
> values as bytes, not types, so int16_t *something; something += stride / 2;
> actually becomes "add something, stride" in assembly... Which part do you
> feel will be calculated at runtime inside the loop?
> 

If they are able to do that then that's fine. They weren't so smart in the
past.

[...]
> I've uploaded a new version addressing these concerns, adding some
> refactoring and adding SIMD for x86 (SSE2, 64bit only) on github:
> https://github.com/rbultje/ffmpeg/commits/colorspace
> Would you like me to squash everything in a single big patch and re-send?
> Or something else?

I'd say 2 patches if possible; one for the filter, one adding the SIMD. If
too complex, just one is OK.

> 
> Ronald

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160406/c0ebc809/attachment.sig>