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.
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
