search

ermig1979 / Simd

C++ image processing and machine learning library with using of SIMD: SSE, AVX, AVX-512, AMX for x86/x64, VMX(Altivec) and VSX(Power7) for PowerPC, NEON for ARM.
http://ermig1979.github.io/Simd
MIT License
2.03k stars 406 forks source link

Conversion error in SimdYuv444pToBgra #186

Closed mikeversteeg closed 9 months ago

mikeversteeg commented 2 years ago

There appears to be either a bug or a wrong colour space selection in SimdYuv444pToBgra and SimdYuv420pToBgra, and possible other YUV<>RGB conversion routines as well. I noticed this because an HD colour chart in YUV looks slightly different when converted to RGB. If this is not a bug, maybe you are using BT.601 instead of Rec.709? I did not check for that, BT.601 is valid for SD resolutions, for HD (x>720 || y>576) Rec.709 should be used and for UHD Rec.2020 (x>1920 || y>1080). You could either auto detect the resolution and choose the colour space or add a parameter to set it manually. See also https://en.wikipedia.org/wiki/YUV. As I use these functions in the professional broadcast world, even the slightest colour errors are unacceptable. So I hope this is not too much work to fix. Thanks!

Untitled

ermig1979 commented 2 years ago

I looked coefficients which I use to YUV to BGR conversion (https://github.com/ermig1979/Simd/blob/master/src/Simd/SimdConst.h):

    const int BLUE_TO_Y_WEIGHT = int(0.098*(1 << BGR_TO_YUV_AVERAGING_SHIFT) + 0.5);
    const int GREEN_TO_Y_WEIGHT = int(0.504*(1 << BGR_TO_YUV_AVERAGING_SHIFT) + 0.5);
    const int RED_TO_Y_WEIGHT = int(0.257*(1 << BGR_TO_YUV_AVERAGING_SHIFT) + 0.5);

It is BT.601:

 0.098 = 0.114 * 220 / 256;
 0.257 = 0.299 * 220 / 256;

The issue has two way to be solved :

  1. I can add special macros which will switch between formats (BT.601 and BT.709). The solution will be fast but is not universal.
  2. I can add special parameter to all function which convert YUV to BGR and back and make choosing in runtime. It is hard way.
mikeversteeg commented 2 years ago

Thanks for looking into this! If a macro is easy, then isn't it also easy to create 3 functions, one for each colour space? Then user can decide which one is applicable to current resolution. The possibility to change the colour space at runtime is important as the video resolution may change.

ermig1979 commented 2 years ago

My resume:

  1. There are 4 different YUV standards: T-REC-T.871(JPEG), BT.601, BT.709, BT.2020.
  2. All functions which convert YUV to BGR and backward must to have parameter to choose YUV standard.

This transformation requres change of API, architecture and unit tests for at least 21 functions (21*(5 - 7) ~ 100 - 150 implementations for different platforms). Its a big amount of work and can take long time.

mikeversteeg commented 2 years ago

Personally I'm only interested in the Windows implementation (Intel/AMD) and 601, 709 & 2020 colour spaces, with an immediate need for Rec.709 support in SimdYuv444pToBgra (within a month). I understand though that you wish to keep your library universal. This does mean a lot of work for something apparently no one is even aware of. Why not a compromise? Properly document colour space used for all functions (BT.601), and add the colour space parameter to functions gradually? Starting of course with SimdYuv444pToBgra :D

ermig1979 commented 2 years ago

I think in order to save backward compatibility it is reasonable to add new functions with name something like SimdYuv444pToBgraV2. And I stop to implement of UyvyToBgr until have new YUV to BGR conversion.

mikeversteeg commented 2 years ago

Sounds like a plan!

ermig1979 commented 2 years ago

I added base version of SimdYuv444pToBgraV2. It will be great if before I start optimizations you will check functionality.

mikeversteeg commented 2 years ago

Wow, that's fast.. On it.

mikeversteeg commented 2 years ago

BT.709 is perfect! image

mikeversteeg commented 2 years ago

And I can confirm the missing optimisation: it uses considerably more CPU than V1.

ermig1979 commented 2 years ago

I Added SSE2 and AVX2 optimizations.

mikeversteeg commented 2 years ago

BT.709 is perfect! Shall I also test 601 & 2020 now?

ermig1979 commented 2 years ago

Yes it will be great.

mikeversteeg commented 2 years ago

Sorry, I realise I can't. When I receive an e.g. SD video frame I first need to deinterlace it (2 SimdDeinterleaveUv) and then scale it up to my program's (HD) resolution (3 SimdResizerRun) for further processing, and finally to display I need to convert it to RGB (3* SimdResizerRun + SimdYuv444pToBgra). So to have proper colour reproduction either SimdDeinterleaveUv or SimdResizerRun need to be able to include colour space conversion, or an additional colour convert function needs to be added on the input side. Fortunately in broadcast world mixing resolutions is uncommon. There is already a lot of calculation going on 😵

ermig1979 commented 2 years ago

Why you can't convert SD video frame from YUV to BGRA and then perform resizing?

mikeversteeg commented 2 years ago

I have multiple video sources all coming together (think large video switcher with fx buses). So internally all video sources need to be represented in same format. To use RGBA means a lot of data so I chose planar YUV as most of your functions (as well as most video files) use this. In this particular case I am working on a source that uses UYVY.

ermig1979 commented 2 years ago

I don't fully understand what exactly do you need except you need do something faster. Unambiguously I am against mixing of different functionality in the one function. Are you need in UYVY to BGRA conversion?

mikeversteeg commented 2 years ago

I am trying to say that eventually either colour conversion should be part of SimdResizerRun, or a separate SimdColourConvert is required. Because if you resize an YUV from e.g. SD to HD, it should go from BT.601 to BT.709. I know, "standards" are a pain! This does not have high priority for me though. Thanks again for your work, you may close this ticket.

ermig1979 commented 9 months ago

I added functions for YUV <-> BGR conversions for different standards.