Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 help / color / mirror / Atom feed
From: John Cox <jc@kynesim.co.uk>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH v1 3/6] swscale: Add explicit rgb24->yv12 conversion
Date: Sun, 20 Aug 2023 19:09:34 +0100
Message-ID: <tti4eiljf93fkavc2i2m3rnihn99ksejq6@4ax.com> (raw)
In-Reply-To: <20230820171614.GC7802@pb2>

On Sun, 20 Aug 2023 19:16:14 +0200, you 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

Could do, and I will if you deem it important, but the only bit that
inline is going to help is the matrix coefficient loading and that
happens once outside the main loops.

>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)

On the whole - yes - in the encode path on the Pi that I'm writing this
for speed is more important than quality - the existing path is too slow
to be usable. And honestly - using your example above comparing (Windows
photo viewer zoomed in s.t. pixels are clearly individually visible) the
general (bitexact), presumably HQ, output vs the new code I grant that
the new is slightly muckier but not by a huge amount - sharp chroma
transitions in 420 are always nasty.

>[...]
>> diff --git a/libswscale/swscale_unscaled.c b/libswscale/swscale_unscaled.c
>> index 32e0d7f63c..751bdcb2e4 100644
>> --- a/libswscale/swscale_unscaled.c
>> +++ b/libswscale/swscale_unscaled.c
>> @@ -1654,6 +1654,23 @@ static int bgr24ToYv12Wrapper(SwsContext *c, const uint8_t *src[],
>>      return srcSliceH;
>>  }
>>  
>> +static int rgb24ToYv12Wrapper(SwsContext *c, const uint8_t *src[],
>> +                              int srcStride[], int srcSliceY, int srcSliceH,
>> +                              uint8_t *dst[], int dstStride[])
>> +{
>> +    ff_rgb24toyv12(
>> +        src[0],
>> +        dst[0] +  srcSliceY       * dstStride[0],
>> +        dst[1] + (srcSliceY >> 1) * dstStride[1],
>> +        dst[2] + (srcSliceY >> 1) * dstStride[2],
>> +        c->srcW, srcSliceH,
>> +        dstStride[0], dstStride[1], srcStride[0],
>> +        c->input_rgb2yuv_table);
>> +    if (dst[3])
>> +        fillPlane(dst[3], dstStride[3], c->srcW, srcSliceH, srcSliceY, 255);
>> +    return srcSliceH;
>> +}
>> +
>>  static int yvu9ToYv12Wrapper(SwsContext *c, const uint8_t *src[],
>>                               int srcStride[], int srcSliceY, int srcSliceH,
>>                               uint8_t *dst[], int dstStride[])
>
>> @@ -2035,8 +2052,13 @@ void ff_get_unscaled_swscale(SwsContext *c)
>>      /* bgr24toYV12 */
>>      if (srcFormat == AV_PIX_FMT_BGR24 &&
>>          (dstFormat == AV_PIX_FMT_YUV420P || dstFormat == AV_PIX_FMT_YUVA420P) &&
>> -        !(flags & SWS_ACCURATE_RND) && !(dstW&1))
>> +        !(flags & (SWS_ACCURATE_RND | SWS_BITEXACT)) && !(dstW&1))
>
>this doesnt belong in this patch

So should it go in its own patch, or attached to some other patch?

Ta
_______________________________________________
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".

  parent reply	other threads:[~2023-08-20 18:09 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-20 15:10 [FFmpeg-devel] [PATCH v1 0/6] swscale: Add dedicated RGB->YUV unscaled functions & aarch64 asm John Cox
2023-08-20 15:10 ` [FFmpeg-devel] [PATCH v1 1/6] fate-filter-fps: Set swscale bitexact for tests that do conversions John Cox
2023-08-20 15:10 ` [FFmpeg-devel] [PATCH v1 2/6] swscale: Rename BGR24->YUV conversion functions as bgr John Cox
2023-08-20 15:10 ` [FFmpeg-devel] [PATCH v1 3/6] swscale: Add explicit rgb24->yv12 conversion John Cox
2023-08-20 17:16   ` Michael Niedermayer
2023-08-20 17:45     ` Michael Niedermayer
2023-08-20 18:28       ` John Cox
2023-08-21 19:15         ` Michael Niedermayer
2023-08-22 14:24           ` John Cox
2023-08-22 18:03             ` Michael Niedermayer
2023-08-20 18:09     ` John Cox [this message]
2023-08-20 15:10 ` [FFmpeg-devel] [PATCH v1 4/6] swscale: RGB24->YUV allow odd widths & improve C rounding John Cox
2023-08-20 15:10 ` [FFmpeg-devel] [PATCH v1 5/6] swscale: Add unscaled XRGB->YUV420P functions John Cox
2023-08-20 15:10 ` [FFmpeg-devel] [PATCH v1 6/6] swscale: Add aarch64 functions for RGB24->YUV420P John Cox

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=tti4eiljf93fkavc2i2m3rnihn99ksejq6@4ax.com \
    --to=jc@kynesim.co.uk \
    --cc=ffmpeg-devel@ffmpeg.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
		ffmpegdev@gitmailbox.com
	public-inbox-index ffmpegdev

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git