5
\$\begingroup\$

I have a stream of YUV422 encoded images coming in from a camera using Direct Show. I then run this byte array through a conversion method and return the RGB array to C# to display in a WPF app.

My problem is that running the application for a 10 minute period and profiling it, the CPU usage is too high for my usage, and it spends around 70-80% of the time in the YUV422 to RGB conversion method.

The method works really well but is just too slow.

Are there any optimizations I am missing here? (I am mainly a C# programmer so I can easily miss C++ optimizations)

*nb this method is in a C++ dll that is "PInvoked" from C# - the reason for C++ is that a similar method takes twice as long in C# than C++

const int STANDARD_SIZE = 1036800;

// Clamp out of range values
#define CLAMP(t) (((t)>255)?255:(((t)<0)?0:(t)))

// Color space conversion for RGB
#define GET_R_FROM_YUV(y, u, v) ((298*y+409*v+128)>>8)
#define GET_G_FROM_YUV(y, u, v) ((298*y-100*u-208*v+128)>>8)
#define GET_B_FROM_YUV(y, u, v) ((298*y+516*u+128)>>8)


BOOL __stdcall YUV422toRGB888(unsigned char *d, unsigned char *s)
{
    for (unsigned int i = 0; i < STANDARD_SIZE; ++i)
    {
        int y0 = *s++ - 16;
        int u0 = *s++ - 128;
        int y2 = *s++ - 16;
        int v = *s++ - 128;

        // BGR
        *d++ = CLAMP(GET_B_FROM_YUV(y0, u0, v));
        *d++ = CLAMP(GET_G_FROM_YUV(y0, u0, v));
        *d++ = CLAMP(GET_R_FROM_YUV(y0, u0, v));

        // BGR
        *d++ = CLAMP(GET_B_FROM_YUV(y2, u0, v));
        *d++ = CLAMP(GET_G_FROM_YUV(y2, u0, v));
        *d++ = CLAMP(GET_R_FROM_YUV(y2, u0, v));
    }

    return true;
}
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Looping STANDARD_SIZE times? Fortunately it can be made parallel. Better, you can rewrite it to use SIMD instructions...for example this (just random pick) and if performance are still a problem then you can make it parallel (it's easy to parallelize per blocks) \$\endgroup\$ Commented Apr 11, 2017 at 13:28
  • \$\begingroup\$ could you give an example of how you would use SIMD instructions here \$\endgroup\$ Commented Apr 12, 2017 at 7:45
  • \$\begingroup\$ Steven, see the linked Stack Overflow post. To use that code (or better code from the answer) you need to resort to assembly but I'm sure you might do something also with intrinsic. Oh, I forgot to be pedantic! Why do you use your own CLAMP macro instead of std::clamp()? Also...well...compiler may do it for you (check generated code) but 298 * y might be calculated just once instead of six times. Hmmmm also sure about conversion formula? Quick check on wikipedia gave different values. \$\endgroup\$ Commented Apr 12, 2017 at 8:12
  • \$\begingroup\$ The method works well so yes I am sure about the formula, isnt std:clamp() a C++17 only method? \$\endgroup\$ Commented Apr 12, 2017 at 8:16
  • \$\begingroup\$ Yes, c++17 only but without it I'd std::max(min, std::min(max, value)). Not that generated code will be different in any way... \$\endgroup\$ Commented Apr 12, 2017 at 8:31

2 Answers 2

2
\$\begingroup\$

Be mindful of endianess, and also, some systems are UYUV instead of YUVU.

Sometimes it just takes some trial and error if your docs aren't very good :-(. For myself, the following ordering of YUYV worked great with the rest of the code:

int u0 = *yuv_in++ - 128;
int y0 = *yuv_in++ - 16;
int v = *yuv_in++ - 128;
int y2 = *yuv_in++ - 16;
\$\endgroup\$
2
\$\begingroup\$
const int STANDARD_SIZE = 1036800;

Don't use all-caps for identifiers; the convention is that we reserve that for macros. Macros need extra attention: being text substition, they don't obey the rules of scope or precedence.

#define CLAMP(t) (((t)>255)?255:(((t)<0)?0:(t)))

Simpler to use std::clamp() here. And no reason to be a macro rather than an (inlineable) function.

#define GET_R_FROM_YUV(y, u, v) ((298*y+409*v+128)>>8)
#define GET_G_FROM_YUV(y, u, v) ((298*y-100*u-208*v+128)>>8)
#define GET_B_FROM_YUV(y, u, v) ((298*y+516*u+128)>>8)

These should all be functions too, and it's probably easier to clamp them at this point rather than at the call sites. Also, we could reorder the expressions to put the offset of 128 first, to allow the compiler to extract the common subexpression 128+298*y. Also, right shift isn't defined for negative values, so use division instead and/or clamp before shifting.

for (unsigned int i = 0; i < STANDARD_SIZE; ++i)

Why is i unsigned, and being compared with a (signed) int?

Instead of introducing a new variable, consider using one of the existing variables (s or d) to count the loop. Perhaps

auto *const e = d + 6 * STANDARD_SIZE;

while (d < e)

On the other hand, it may be worth keeping the loop iterator, and using that to divide the work between threads, using OpenMP or similar.

    int y0 = *s++ - 16;
    int u0 = *s++ - 128;
    int y2 = *s++ - 16;
    int v = *s++ - 128;

Instead of subtracting here, why not leave it until the calculation functions, where we are adding a constant to the result?

    return true;

This seems useless, since we never return any other value. We should declare the function to return void.


Modified code

Re-written in more modern C++:

#include <algorithm>

constexpr unsigned int image_length = 1036800;

namespace {
    void write_bgr(unsigned char *d, int y, int u, int v) {
        *d   = std::clamp(298*y + 516*u - 70688, 0, 65535) / 256;
        *++d = std::clamp(298*y - 100*u - 208*v - 44064, 0, 65535) / 256;
        *++d = std::clamp(298*y + 409*v - 56992, 0, 65535) / 256;
    }
}

void yuv422_to_bgr888(unsigned char *dest, unsigned char *src)
{
#pragma omp parallel for
    for (unsigned int i = 0u;  i < image_length;  ++i) {
        auto *s = src + 4 * i;
        int y0 = *s;
        int u0 = *++s;
        int y2 = *++s;
        int v =  *++s;

        auto *d = dest + 6 * i;
        write_bgr(d,   y0, u0, v);
        write_bgr(d+3, y2, u0, v);
    }
}
\$\endgroup\$
5
  • \$\begingroup\$ I think using all-capital identifiers for constexpr constants is fine (like a macro, you can't take the address of it for example). I guess it depends on the code style. As for the name image_pixels, I think it's not clear that it is the number of pixels. So n_image_pixels, image_pixel_count or image_size might be better. \$\endgroup\$
    – G. Sliepen
    Commented Jun 9, 2021 at 18:39
  • \$\begingroup\$ I suspect we'll always disagree on that - personally, I find it's shouting "wolf" too much. \$\endgroup\$ Commented Jun 9, 2021 at 19:45
  • 1
    \$\begingroup\$ @G.Sliepen All-capital identifiers shout preprocessor in C and C++. In languages without, yes, they only mean compiled-in constant. \$\endgroup\$ Commented Jun 9, 2021 at 20:38
  • 1
    \$\begingroup\$ Why not make write_bgr() a lambda? It's not like anyone else should call it. I would expect the type of i to be auto, or the initializer to be plain 0. Both is a bit much. And finally, auto* is rarely more appropriate than auto. It might be useful to mention restrict most C++ implementations borrow from standard C. Not to mention, introduce a bit more const. Anyway, much improved over the original post. \$\endgroup\$ Commented Jun 9, 2021 at 20:46
  • \$\begingroup\$ auto* is a habit provoked by the style-checker in $DAY_JOB. It looks to be one it invented, rather than copied from CERT or MISRA. I was never really a fan of it, but it's become, well, automatic now. Other points spot-on. \$\endgroup\$ Commented Jun 10, 2021 at 5:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.