[FFmpeg-devel] [PATCH v2 02/19] swscale: rename SwsContext to SwsInternal
Niklas Haas
ffmpeg at haasn.xyz
Sun Oct 20 14:21:02 EEST 2024
On Sat, 19 Oct 2024 23:37:17 +0200 Michael Niedermayer <michael at niedermayer.cc> wrote:
> On Thu, Oct 17, 2024 at 02:28:54PM +0200, Niklas Haas wrote:
> > On Mon, 14 Oct 2024 20:58:31 +0200 Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > On Mon, Oct 14, 2024 at 08:33:50PM +0200, Niklas Haas wrote:
> > > > On Mon, 14 Oct 2024 19:49:21 +0200 Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > > > On Mon, Oct 14, 2024 at 06:39:21PM +0200, Michael Niedermayer wrote:
> > > > > > On Mon, Oct 14, 2024 at 06:02:45PM +0200, Niklas Haas wrote:
> > > > > > > On Mon, 14 Oct 2024 16:55:39 +0200 Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > > > > > > On Mon, Oct 14, 2024 at 03:37:27PM +0200, Niklas Haas wrote:
> > > > > > > > > From: Niklas Haas <git at haasn.dev>
> > > > > > > > >
> > > > > > > > > And preserve the public SwsContext as separate name. The motivation here
> > > > > > > > > is that I want to turn SwsContext into a public struct, while keeping the
> > > > > > > > > internal implementation hidden. Additionally, I also want to be able to
> > > > > > > > > use multiple internal implementations, e.g. for GPU devices.
> > > > > > > > >
> > > > > > > > > This commit does not include any functional changes. For the most part, it is
> > > > > > > > > a simple rename. The only complications arise from the public facing API
> > > > > > > > > functions, which preserve their current type (and hence require an additional
> > > > > > > > > unwrapping step internally), and the checkasm test framework, which directly
> > > > > > > > > accesses SwsInternal.
> > > > > > > > >
> > > > > > > > > For consistency, the affected functions that need to maintain a distionction
> > > > > > > > > have generally been changed to refer to the SwsContext as *sws, and the
> > > > > > > > > SwsInternal as *c.
> > > > > > > > >
> > > > > > > > > In an upcoming commit, I will provide a backing definition for the public
> > > > > > > > > SwsContext, and update `sws_internal()` to dereference the internal struct
> > > > > > > > > instead of merely casting it.
> > > > > > > > >
> > > > > > > > > Sponsored-by: Sovereign Tech Fund
> > > > > > > > > Signed-off-by: Niklas Haas <git at haasn.dev>
> > > > > > > >
> > > > > > > > does not apply
> > > > > > > >
> > > > > > > >
> > > > > > > > Applying: swscale: rename SwsContext to SwsInternal
> > > > > > > > Using index info to reconstruct a base tree...
> > > > > > > > M libswscale/output.c
> > > > > > > > M libswscale/utils.c
> > > > > > > > Falling back to patching base and 3-way merge...
> > > > > > > > Auto-merging libswscale/utils.c
> > > > > > > > Auto-merging libswscale/output.c
> > > > > > > > CONFLICT (content): Merge conflict in libswscale/output.c
> > > > > > > > error: Failed to merge in the changes.
> > > > > > > > Patch failed at 0001 swscale: rename SwsContext to SwsInternal
> > > > > > > > hint: Use 'git am --show-current-patch=diff' to see the failed patch
> > > > > > > > hint: When you have resolved this problem, run "git am --continue".
> > > > > > > > hint: If you prefer to skip this patch, run "git am --skip" instead.
> > > > > > > > hint: To restore the original branch and stop patching, run "git am --abort".
> > > > > > > > hint: Disable this message with "git config advice.mergeConflict false"
> > > > > > > >
> > > > > > > > thx
> > > > > > >
> > > > > > > I've rebased it here:
> > > > > > >
> > > > > > > https://github.com/haasn/FFmpeg/tree/swscale4
> > > > > >
> > > > > > on x86-32 linux
> > > > > >
> > > > > > make
> > > > > > CC libswscale/alphablend.o
> > > > > > In file included from src/libavutil/internal.h:39:0,
> > > > > > from src/libavutil/common.h:50,
> > > > > > from src/libavutil/avutil.h:301,
> > > > > > from src/libswscale/swscale.h:33,
> > > > > > from src/libswscale/swscale_internal.h:28,
> > > > > > from src/libswscale/alphablend.c:21:
> > > > > > src/libswscale/swscale_internal.h:682:1: error: static assertion failed: "yuv2rgb_y_offset must be updated in x86 asm"
> > > > > > static_assert(offsetof(SwsInternal, yuv2rgb_y_offset) == 40348,
> > > > > > ^
> > > > > > make: *** [src/ffbuild/common.mak:81: libswscale/alphablend.o] Error 1
> > > > >
> > > > > and on qemu-MIPS: (big endian issue maybe)
> > > > >
> > > > > --- src/tests/ref/fate/sws-yuv-colorspace 2024-10-14 18:05:24.495328917 +0200
> > > > > +++ tests/data/fate/sws-yuv-colorspace 2024-10-14 18:41:48.656863487 +0200
> > > > > @@ -3,4 +3,4 @@
> > > > > #codec_id 0: rawvideo
> > > > > #dimensions 0: 352x288
> > > > > #sar 0: 0/1
> > > > > -0, 0, 0, 1, 152064, 0xcbcb97b9
> > > > > +0, 0, 0, 1, 152064, 0x956698d5
> > > > > TEST acodec-pcm-u8
> > > > > Test sws-yuv-colorspace failed. Look at tests/data/fate/sws-yuv-colorspace.err for
> > > >
> > > > Do you have a command line I could copy to reproduce this?
> > >
> > > iam not sure what you ask, what i use to cross build ffmpeg to mips and run it
> > > under qemu is below, but this is not the latest qemu or latest compilers. I tend
> > > to only update these when i have to or when something breaks
> > >
> > > ../configure --target-exec='qemu-mips -cpu 74Kf -L /usr/mips-linux-gnu/ /usr/mips-linux-gnu/lib/ld-2.30.so --inhibit-cache' --samples=fate-suite/ --enable-gpl --cross-prefix=/usr/mips-linux-gnu/bin/ --cc='ccache mips-linux-gnu-gcc-7' --arch=mips --target-os=linux --enable-cross-compile --disable-mipsfpu --disable-iconv --cpu=74Kf
> > >
> > > this is on ubuntu
> >
> > Fixed, thanks. The int64_t flags were being dereferenced as `unsigned int` by
> > the AVOption code, which broke on big endian platforms.
> >
> > Pushed the fix to https://github.com/haasn/FFmpeg/tree/swscale4
>
> Both swscale4 and swscale5 fail if used a bit differently from ffmpeg
> simple testcase with DC color below
> (also feel free to add this to the fate tests, if you like)
>
> no-scale rgb-lim rgb-lim 200 -> 200
> no-scale rgb-lim rgb-ful 200 -> 200
> no-scale rgb-ful rgb-lim 200 -> 200
> no-scale rgb-ful rgb-ful 200 -> 200
> no-scale rgb-lim yuv-lim 200 -> 16 FAIL
> no-scale rgb-lim yuv-ful 200 -> 16 FAIL
> no-scale rgb-ful yuv-lim 200 -> 16 FAIL
> no-scale rgb-ful yuv-ful 200 -> 16 FAIL
> no-scale yuv-lim rgb-lim 188 -> 0 FAIL
> no-scale yuv-lim rgb-ful 188 -> 0 FAIL
> no-scale yuv-ful rgb-lim 200 -> 0 FAIL
> no-scale yuv-ful rgb-ful 200 -> 0 FAIL
> no-scale yuv-lim yuv-lim 188 -> 188
> no-scale yuv-lim yuv-ful 188 -> 188 FAIL
> no-scale yuv-ful yuv-lim 200 -> 200 FAIL
> no-scale yuv-ful yuv-ful 200 -> 200
>
> scale rgb-lim rgb-lim 200 -> 0 FAIL
> scale rgb-lim rgb-ful 200 -> 0 FAIL
> scale rgb-ful rgb-lim 200 -> 0 FAIL
> scale rgb-ful rgb-ful 200 -> 0 FAIL
> scale rgb-lim yuv-lim 200 -> 16 FAIL
> scale rgb-lim yuv-ful 200 -> 16 FAIL
> scale rgb-ful yuv-lim 200 -> 16 FAIL
> scale rgb-ful yuv-ful 200 -> 16 FAIL
> scale yuv-lim rgb-lim 188 -> 0 FAIL
> scale yuv-lim rgb-ful 188 -> 0 FAIL
> scale yuv-ful rgb-lim 200 -> 0 FAIL
> scale yuv-ful rgb-ful 200 -> 0 FAIL
> scale yuv-lim yuv-lim 188 -> 188
> scale yuv-lim yuv-ful 188 -> 188 FAIL
> scale yuv-ful yuv-lim 200 -> 200 FAIL
> scale yuv-ful yuv-ful 200 -> 200
>
> vs:
> no-scale rgb-lim rgb-lim 200 -> 200
> no-scale rgb-lim rgb-ful 200 -> 200
> no-scale rgb-ful rgb-lim 200 -> 200
> no-scale rgb-ful rgb-ful 200 -> 200
> no-scale rgb-lim yuv-lim 200 -> 188
> no-scale rgb-lim yuv-ful 200 -> 200
> no-scale rgb-ful yuv-lim 200 -> 188
> no-scale rgb-ful yuv-ful 200 -> 200
> no-scale yuv-lim rgb-lim 188 -> 200
> no-scale yuv-lim rgb-ful 188 -> 200
> no-scale yuv-ful rgb-lim 200 -> 200
> no-scale yuv-ful rgb-ful 200 -> 200
> no-scale yuv-lim yuv-lim 188 -> 188
> no-scale yuv-lim yuv-ful 188 -> 200
> no-scale yuv-ful yuv-lim 200 -> 188
> no-scale yuv-ful yuv-ful 200 -> 200
>
> scale rgb-lim rgb-lim 200 -> 200
> scale rgb-lim rgb-ful 200 -> 200
> scale rgb-ful rgb-lim 200 -> 200
> scale rgb-ful rgb-ful 200 -> 200
> scale rgb-lim yuv-lim 200 -> 188
> scale rgb-lim yuv-ful 200 -> 200
> scale rgb-ful yuv-lim 200 -> 188
> scale rgb-ful yuv-ful 200 -> 200
> scale yuv-lim rgb-lim 188 -> 200
> scale yuv-lim rgb-ful 188 -> 200
> scale yuv-ful rgb-lim 200 -> 200
> scale yuv-ful rgb-ful 200 -> 200
> scale yuv-lim yuv-lim 188 -> 188
> scale yuv-lim yuv-ful 188 -> 200
> scale yuv-ful yuv-lim 200 -> 188
> scale yuv-ful yuv-ful 200 -> 200
>
> #include <inttypes.h>
> #include <stdbool.h>
> #include <stdio.h>
> #include <string.h>
> #include <assert.h>
> #include "libswscale/swscale.h"
> #include "libavutil/opt.h"
> #include "libavutil/pixfmt.h"
>
> #define W 128
> #define H 128
> #define STRIDE_YUV W
> #define STRIDE_RGBA (W * 4)
>
> void scale(uint8_t *dst[3], uint8_t *src[3], int s_yuv, int s_range,
> int d_yuv, int d_range, int scale)
> {
> struct SwsContext *sws = sws_alloc_context();
>
> int exact = SWS_FULL_CHR_H_INT | SWS_FULL_CHR_H_INP |
> SWS_ACCURATE_RND | SWS_BITEXACT;
> av_opt_set_int(sws, "sws_flags", exact | SWS_POINT /* | SWS_PRINT_INFO*/, 0);
>
> av_opt_set_int(sws, "srcw", W, 0);
> av_opt_set_int(sws, "srch", H, 0);
> av_opt_set_int(sws, "src_format", s_yuv ? AV_PIX_FMT_YUV444P : AV_PIX_FMT_RGBA, 0);
>
> av_opt_set_int(sws, "dstw", scale ? W/2 : W, 0);
> av_opt_set_int(sws, "dsth", scale ? H/2 : H, 0);
> av_opt_set_int(sws, "dst_format", d_yuv ? AV_PIX_FMT_YUV444P : AV_PIX_FMT_RGBA, 0);
>
> sws_setColorspaceDetails(sws, sws_getCoefficients(SWS_CS_ITU709), !!s_range,
> sws_getCoefficients(SWS_CS_ITU709), !!d_range,
> 0, 1 << 16, 1 << 16);
>
> int res = sws_init_context(sws, NULL, NULL);
> assert(res >= 0);
>
> int s_stride = s_yuv ? STRIDE_YUV : STRIDE_RGBA;
> int d_stride = d_yuv ? STRIDE_YUV : STRIDE_RGBA;
> int s_stride_a[3] = {s_stride, s_stride, s_stride};
> int d_stride_a[3] = {d_stride, d_stride, d_stride};
> sws_scale(sws, (const unsigned char *const *)src, s_stride_a, 0, H, dst, d_stride_a);
> sws_freeContext(sws);
> }
>
> void set_white(uint8_t *c[3], bool yuv, int val)
> {
> if (yuv) {
> memset(c[0], val, STRIDE_YUV * H);
> memset(c[1], 128, STRIDE_YUV * H);
> memset(c[2], 128, STRIDE_YUV * H);
> } else {
> int i;
> memset(c[0], val, STRIDE_RGBA * H);
> for (i = 3; i<STRIDE_RGBA*H; i+=4)
> c[0][i] = 0xFF;
> }
> }
>
> int get_white(uint8_t *c[3], bool yuv)
> {
> if (yuv) {
> return c[0][W/4 + STRIDE_YUV * (H/4)];
> } else {
> return c[0][W/4 * 4 + STRIDE_RGBA * (H/4) + 1];
> }
> }
>
> uint8_t *a[3], *b[3];
>
> void test(unsigned int mode)
> {
> bool d_range = mode & 1;
> bool d_yuv = mode & 4;
> bool s_range = mode & 2;
> bool s_yuv = mode & 8;
> bool doscale = mode & 16;
> bool lower = mode & 32;
>
>
> int ival = (s_yuv && !s_range) ? 188 : 200;
>
> set_white(a, s_yuv, ival);
> scale(b, a, s_yuv, s_range, d_yuv, d_range, doscale);
> int oval = get_white(b, d_yuv);
> printf("%-9s", doscale ? "scale" : "no-scale");
> printf("%s", s_yuv ? "yuv" : "rgb");
> printf("-%s ", s_range ? "ful" : "lim");
> printf("%s", d_yuv ? "yuv" : "rgb");
> printf("-%s ", d_range ? "ful" : "lim");
> printf(" %d -> %d", ival, oval);
>
> if( oval != ((d_yuv && !d_range) ? 188 : 200) )
> printf(" FAIL");
> printf("\n");
> }
>
> int main(int argc, char **argv)
> {
> int i;
> for (i = 0; i < 3; i++) {
> a[i] = av_malloc(STRIDE_RGBA * H);
> b[i] = av_malloc(STRIDE_RGBA * H);
> }
> for (i = 0; i < 32; i++) {
> test(i);
> if (i % 16 == 15)
> printf("\n");
> }
> }
>
> thx
Found the issue, it's caused by calling sws_setColorspaceDetails() before
sws_init_context(). Seems a lot of downstream code relies on this behavior,
so we have to continue supporting it.
I'll just make both init_context *and* setColorspaceDetails() partially
fix the relevant fields, and also error out explicitly if they've changed
in the meantime. (Since the status quo atm is that some fields may no longer
be modified after sws_setColorspaceDetails() has been called)
Will push a fix later today.
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> I know you won't believe me, but the highest form of Human Excellence is
> to question oneself and others. -- Socrates
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list