Re: [FFmpeg-devel] [PATCH v3 1/4] swscale/input: add rgbaf32 input support

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 5215 bytes --]

On Sun, Nov 13, 2022 at 05:50:37PM -0800, Mark Reid wrote:
> On Sun, Nov 13, 2022 at 1:25 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Wed, Nov 02, 2022 at 09:00:07PM -0700, mindmark@gmail.com wrote:
> > > From: Mark Reid <mindmark@gmail.com>
> > >
> > > ---
> > >  libswscale/input.c | 172 +++++++++++++++++++++++++++++++++++++++++++++
> > >  libswscale/utils.c |   4 ++
> > >  2 files changed, 176 insertions(+)
> > >
> > > diff --git a/libswscale/input.c b/libswscale/input.c
> > > index 7ff7bfaa01..4683284b0b 100644
> > > --- a/libswscale/input.c
> > > +++ b/libswscale/input.c
> > > @@ -1284,6 +1284,136 @@ static void rgbaf16##endian_name##ToA_c(uint8_t
> > *_dst, const uint8_t *_src, cons
> > >  rgbaf16_funcs_endian(le, 0)
> > >  rgbaf16_funcs_endian(be, 1)
> > >
> > > +#define rdpx(src) (is_be ? av_int2float(AV_RB32(&src)):
> > av_int2float(AV_RL32(&src)))
> > > +
> > > +static av_always_inline void rgbaf32ToUV_half_endian(uint16_t *dstU,
> > uint16_t *dstV, int is_be,
> > > +                                                     const float *src,
> > int width,
> > > +                                                     int32_t *rgb2yuv,
> > int comp)
> > > +{
> > > +    int32_t ru = rgb2yuv[RU_IDX], gu = rgb2yuv[GU_IDX], bu =
> > rgb2yuv[BU_IDX];
> > > +    int32_t rv = rgb2yuv[RV_IDX], gv = rgb2yuv[GV_IDX], bv =
> > rgb2yuv[BV_IDX];
> > > +    int i;
> > > +    for (i = 0; i < width; i++) {
> >
> > > +        int r = (lrintf(av_clipf(65535.0f * rdpx(src[i*(comp*2)+0]),
> > 0.0f, 65535.0f)) +
> > > +                 lrintf(av_clipf(65535.0f * rdpx(src[i*(comp*2)+4]),
> > 0.0f, 65535.0f))) >> 1;
> > > +        int g = (lrintf(av_clipf(65535.0f * rdpx(src[i*(comp*2)+1]),
> > 0.0f, 65535.0f)) +
> > > +                 lrintf(av_clipf(65535.0f * rdpx(src[i*(comp*2)+5]),
> > 0.0f, 65535.0f))) >> 1;
> > > +        int b = (lrintf(av_clipf(65535.0f * rdpx(src[i*(comp*2)+2]),
> > 0.0f, 65535.0f)) +
> > > +                 lrintf(av_clipf(65535.0f * rdpx(src[i*(comp*2)+6]),
> > 0.0f, 65535.0f))) >> 1;
> > > +
> > > +        dstU[i] = (ru*r + gu*g + bu*b + (0x10001<<(RGB2YUV_SHIFT-1)))
> > >> RGB2YUV_SHIFT;
> > > +        dstV[i] = (rv*r + gv*g + bv*b + (0x10001<<(RGB2YUV_SHIFT-1)))
> > >> RGB2YUV_SHIFT;
> >
> > I would expect this sort of code to use 2 lrintf() and 2 av_clipf() not 6
> >
> >
> ya it is a bit excessive, I'll just remove the _half conversions for now,
> they aren't strictly necessary as far as I can tell.

do you see a problem with just factorizing them out ?
it shouldnt be hard to reorder the operations


> 
> 
> >
> > > +    }
> > > +}
> > > +
> > > +static av_always_inline void rgbaf32ToUV_endian(uint16_t *dstU,
> > uint16_t *dstV, int is_be,
> > > +                                                const float *src, int
> > width,
> > > +                                                int32_t *rgb2yuv, int
> > comp)
> > > +{
> > > +    int32_t ru = rgb2yuv[RU_IDX], gu = rgb2yuv[GU_IDX], bu =
> > rgb2yuv[BU_IDX];
> > > +    int32_t rv = rgb2yuv[RV_IDX], gv = rgb2yuv[GV_IDX], bv =
> > rgb2yuv[BV_IDX];
> > > +    int i;
> > > +    for (i = 0; i < width; i++) {
> > > +        int r = lrintf(av_clipf(65535.0f * rdpx(src[i*comp+0]), 0.0f,
> > 65535.0f));
> > > +        int g = lrintf(av_clipf(65535.0f * rdpx(src[i*comp+1]), 0.0f,
> > 65535.0f));
> > > +        int b = lrintf(av_clipf(65535.0f * rdpx(src[i*comp+2]), 0.0f,
> > 65535.0f));
> > > +
> > > +        dstU[i] = (ru*r + gu*g + bu*b + (0x10001<<(RGB2YUV_SHIFT-1)))
> > >> RGB2YUV_SHIFT;
> > > +        dstV[i] = (rv*r + gv*g + bv*b + (0x10001<<(RGB2YUV_SHIFT-1)))
> > >> RGB2YUV_SHIFT;
> > > +    }
> > > +}
> > > +
> >
> > > +static av_always_inline void rgbaf32ToY_endian(uint16_t *dst, const
> > float *src, int is_be,
> > > +                                               int width, int32_t
> > *rgb2yuv, int comp)
> > > +{
> > > +    int32_t ry = rgb2yuv[RY_IDX], gy = rgb2yuv[GY_IDX], by =
> > rgb2yuv[BY_IDX];
> > > +    int i;
> > > +    for (i = 0; i < width; i++) {
> > > +        int r = lrintf(av_clipf(65535.0f * rdpx(src[i*comp+0]), 0.0f,
> > 65535.0f));
> > > +        int g = lrintf(av_clipf(65535.0f * rdpx(src[i*comp+1]), 0.0f,
> > 65535.0f));
> > > +        int b = lrintf(av_clipf(65535.0f * rdpx(src[i*comp+2]), 0.0f,
> > 65535.0f));
> > > +
> >
> > > +        dst[i] = (ry*r + gy*g + by*b + (0x2001<<(RGB2YUV_SHIFT-1))) >>
> > RGB2YUV_SHIFT;
> >
> > there is one output so there should be only need for one clip and one
> > float->int
> >
> 
> This is matching the f32 planar version. I think I was paranoid about
> things being bitexact for tests and that's why it's currently being done
> this way.
> I'll see what happens if I introduce more float operations, could I perhaps
> do this in a later patch? some asm might have to change too.

of course can be a seperate patch in a set. Maybe f32 planar can be changed
at the same time

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".