Re: [FFmpeg-devel] [PATCH v1 3/6] swscale: Add explicit rgb24->yv12 conversion

[Michael Niedermayer <michael@niedermayer.cc>]

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

On Sun, Aug 20, 2023 at 07:28:40PM +0100, John Cox wrote:
> On Sun, 20 Aug 2023 19:45:11 +0200, you wrote:
> 
> >On Sun, Aug 20, 2023 at 07:16:14PM +0200, Michael Niedermayer wrote:
> >> On Sun, Aug 20, 2023 at 03:10:19PM +0000, John Cox wrote:
> >> > Add a rgb24->yuv420p conversion. Uses the same code as the existing
> >> > bgr24->yuv converter but permutes the conversion array to swap R & B
> >> > coefficients.
> >> > 
> >> > Signed-off-by: John Cox <jc@kynesim.co.uk>
> >> > ---
> >> >  libswscale/rgb2rgb.c          |  5 +++++
> >> >  libswscale/rgb2rgb.h          |  7 +++++++
> >> >  libswscale/rgb2rgb_template.c | 38 ++++++++++++++++++++++++++++++-----
> >> >  libswscale/swscale_unscaled.c | 24 +++++++++++++++++++++-
> >> >  4 files changed, 68 insertions(+), 6 deletions(-)
> >> > 
> >> > diff --git a/libswscale/rgb2rgb.c b/libswscale/rgb2rgb.c
> >> > index 8707917800..de90e5193f 100644
> >> > --- a/libswscale/rgb2rgb.c
> >> > +++ b/libswscale/rgb2rgb.c
> >> > @@ -83,6 +83,11 @@ void (*ff_bgr24toyv12)(const uint8_t *src, uint8_t *ydst,
> >> >                         int width, int height,
> >> >                         int lumStride, int chromStride, int srcStride,
> >> >                         int32_t *rgb2yuv);
> >> > +void (*ff_rgb24toyv12)(const uint8_t *src, uint8_t *ydst,
> >> > +                       uint8_t *udst, uint8_t *vdst,
> >> > +                       int width, int height,
> >> > +                       int lumStride, int chromStride, int srcStride,
> >> > +                       int32_t *rgb2yuv);
> >> >  void (*planar2x)(const uint8_t *src, uint8_t *dst, int width, int height,
> >> >                   int srcStride, int dstStride);
> >> >  void (*interleaveBytes)(const uint8_t *src1, const uint8_t *src2, uint8_t *dst,
> >> > diff --git a/libswscale/rgb2rgb.h b/libswscale/rgb2rgb.h
> >> > index 305b830920..f7a76a92ba 100644
> >> > --- a/libswscale/rgb2rgb.h
> >> > +++ b/libswscale/rgb2rgb.h
> >> > @@ -79,6 +79,9 @@ void    rgb12to15(const uint8_t *src, uint8_t *dst, int src_size);
> >> >  void ff_bgr24toyv12_c(const uint8_t *src, uint8_t *ydst, uint8_t *udst,
> >> >                        uint8_t *vdst, int width, int height, int lumStride,
> >> >                        int chromStride, int srcStride, int32_t *rgb2yuv);
> >> > +void ff_rgb24toyv12_c(const uint8_t *src, uint8_t *ydst, uint8_t *udst,
> >> > +                      uint8_t *vdst, int width, int height, int lumStride,
> >> > +                      int chromStride, int srcStride, int32_t *rgb2yuv);
> >> >  
> >> >  /**
> >> >   * Height should be a multiple of 2 and width should be a multiple of 16.
> >> > @@ -128,6 +131,10 @@ extern void (*ff_bgr24toyv12)(const uint8_t *src, uint8_t *ydst, uint8_t *udst,
> >> >                                int width, int height,
> >> >                                int lumStride, int chromStride, int srcStride,
> >> >                                int32_t *rgb2yuv);
> >> > +extern void (*ff_rgb24toyv12)(const uint8_t *src, uint8_t *ydst, uint8_t *udst, uint8_t *vdst,
> >> > +                              int width, int height,
> >> > +                              int lumStride, int chromStride, int srcStride,
> >> > +                              int32_t *rgb2yuv);
> >> >  extern void (*planar2x)(const uint8_t *src, uint8_t *dst, int width, int height,
> >> >                          int srcStride, int dstStride);
> >> >  
> >> > diff --git a/libswscale/rgb2rgb_template.c b/libswscale/rgb2rgb_template.c
> >> > index 8ef4a2cf5d..e57bfa6545 100644
> >> > --- a/libswscale/rgb2rgb_template.c
> >> > +++ b/libswscale/rgb2rgb_template.c
> >> 
> >> 
> >> > @@ -646,13 +646,14 @@ static inline void uyvytoyv12_c(const uint8_t *src, uint8_t *ydst,
> >> >   * others are ignored in the C version.
> >> >   * FIXME: Write HQ version.
> >> >   */
> >> > -void ff_bgr24toyv12_c(const uint8_t *src, uint8_t *ydst, uint8_t *udst,
> >> > +static void rgb24toyv12_x(const uint8_t *src, uint8_t *ydst, uint8_t *udst,
> >> 
> >> this probably should be inline
> >> 
> >> also i see now "FIXME: Write HQ version." above here. Do you really want to
> >> add a low quality rgb24toyv12 ?
> >> (it is vissible on the diagonal border (cyan / red )) in
> >>  ./ffmpeg -f lavfi -i testsrc=size=5632x3168 -pix_fmt yuv420p -vframes 1 -qscale 1 -strict -1 new.jpg
> >> 
> >>  also on smaller sizes but for some reason its clearer on the big one zoomed in 400% with gimp
> >> (the gimp test was done with the whole patchset not after this patch)
> >
> >Also the reason why its LQ and looks like it does is because
> >1. half the RGB samples are ignored in computing the chroma samples
> 
> I thought it was a bit light but it is what the existing code did
> 
> >2. the chroma sample locations are ignored, the locations for yuv420 are reaonable standard
> 
> As I recall MPEG-1 has chroma at (0.5, 0.5), MPEG-II defaults to (0.5,
> 0),

yes


> H.265 defaults to (0,0).

hmm
    When the value of chroma_format_idc is equal to 1, the nominal vertical and horizontal relative locations of luma and
    chroma samples in pictures are shown in Figure 6-1. Alternative chroma sample relative locations may be indicated in
    video usability information (see Annex E).

    X  X  X  X  X  X
    O     O     O    ...
    X  X  X  X  X  X

    X  X  X  X  X  X
    O     O     O
    X  X  X  X  X  X

    X  X  X  X  X  X
    O     O     O
    X  X  X  X  X  X
    .                .
    :                 ´.
    X Location of luma sample
    O Location of chroma sample

    Figure 6-1 – Nominal vertical and horizontal locations of 4:2:0 luma and chroma samples in a picture



> Printing out dst_h_chr_pos, dst_v_chr_pos
> in the setup of your example yields -513, 128 which I'm guessing means
> (unset, 0.5) - am I looking at the correct vars?
> 
> >this needs some simple filter to get from a few RGB samples to the RGB sample co-located
> >with ths UV sample before RGB->UV
> 

> I can get to simple bilinear without adding so much complexity that I
> lose the speed I need - would that be OK?

Not sure simple bilinear is 100% clearly defined
I think it could mean 3 things

1 2 1
  C
1 2 1

or

  1
  C
  1

  or

1 2 1

3 6 3
  C
3 6 3

1 2 1

I think the 6 and 12 tap cases would produce ok results teh 2 tap not
Also maybe there are more finetuned filters for this specific case, i dont
know / didnt look.
Testing these probably would not be a bad idea before implementation

I think users in 2023 expect the default to be better than what the
existing code was doing by default
so feel free to replace the existing "identical" code too

[...]

thx

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The day soldiers stop bringing you their problems is the day you have stopped 
leading them. They have either lost confidence that you can help or concluded 
you do not care. Either case is a failure of leadership. - Colin Powell

[-- 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".