espressif / esp32-camera

Apache License 2.0
1.9k stars 638 forks source link

RBG vs. BGR in conversion functions #379

Closed AIWintermuteAI closed 2 years ago

AIWintermuteAI commented 2 years ago

I'm using OV2640 on ESP-EYE(ESP32). When collecting images I noticed that the colors are off, example here https://github.com/espressif/esp32-camera/issues/343 image I went on to have a look at the source code of converter functions, specifically fmt2rgb888() to see this https://github.com/espressif/esp32-camera/blob/86a4951f507e86034725b04f2428c98c49496dfe/conversions/to_bmp.c#L290

             yuv2rgb(y0, u, v, &r, &g, &b);
            *rgb_buf++ = b;
            *rgb_buf++ = g;
            *rgb_buf++ = r;

            yuv2rgb(y1, u, v, &r, &g, &b);
            *rgb_buf++ = b;
            *rgb_buf++ = g;
            *rgb_buf++ = r;
        }

And here similar swapping done for JPEG decoding: https://github.com/espressif/esp32-camera/blob/86a4951f507e86034725b04f2428c98c49496dfe/conversions/to_bmp.c#L113

Could someone please clarify the intent behind swapping red and blue channels in the resulting image? I understand it might have to do with endianness of the platform, but in even in that case, two things are very much desired: 1) Notify the user about the fact they're going to be getting BGR image 2) Provide a way to have either RGB or BGR image.

AIWintermuteAI commented 2 years ago

Addressing the question to @me-no-dev, since it was him who originally submitted the code https://github.com/espressif/esp32-camera/blame/86a4951f507e86034725b04f2428c98c49496dfe/conversions/to_bmp.c#L290

me-no-dev commented 2 years ago

I can not recall exactly the reason, but this is what was needed for face detection/recognition, bmp and jpeg. You did not share much of your configuration, but I would guess that you are running in YUV mode. What is the resolution? I do experience color issues at low resolutions. Resolutions above CIF look very different.

AIWintermuteAI commented 2 years ago

Actually I am getting JPEG image from OV2640 module on ESP32. I tried YUV too, to see if it has similar issue. The R and G channel is swapped for JPEG here https://github.com/espressif/esp32-camera/blob/86a4951f507e86034725b04f2428c98c49496dfe/conversions/to_bmp.c#L113 It is definitely intentional - and perhaps justified by some application. However, it is important to note that in documentation or at least in the code as a comment - and ideally to provide a method to get RGB or BGR image, depending on user's needs. I'll make my code public a bit later today - it's official Edge Impulse firmware for ESP32 (ESP-EYE) for you to have a look.

me-no-dev commented 2 years ago

I have not had any such issue so far. I even ran ESP-EYE with different settings to look for issues. There were none. Color were correct with any format I tried. Do you have another sensor that you can try? Can you confirm this with more than one camera board?

AIWintermuteAI commented 2 years ago

Here is the usage in my code: https://github.com/edgeimpulse/firmware-espressif-esp32/blob/e411b519a538509fe8549c495ad37857d36fc9d1/edge-impulse/ingestion-sdk-platform/sensors/ei_camera.cpp#L180 I have two boards, both with OV2640 sensor - ESP-EYE and ESP-CAM. Both exhibit the same problem.

github-actions[bot] commented 2 years ago

This issue appears to be stale. Please close it if its no longer valid.

AIWintermuteAI commented 7 months ago

@me-no-dev the problem clearly still exists and have been reported by many users: https://github.com/espressif/esp32-camera/issues/422 https://github.com/espressif/esp32-camera/issues/343 Including the latest report by one of our users on the forum here https://forum.edgeimpulse.com/t/problems-code-error-with-the-built-in-case-of-the-fomo-rgb-image-and-esp-eye/8625

We are using forked version of your camera library in our pre-built firmware, but for Arduino deployments users have to rely on your official esp32-camera, where the issue is not fixed.

Would it be acceptable to add this as an argument with a default value, so it would not change the default behavior now, but there would be a way to restore R and B ordering? If yes, I'll make a PR about that.

me-no-dev commented 7 months ago

@AIWintermuteAI please go ahead :)

AIWintermuteAI commented 7 months ago

@me-no-dev when trying to implement this I realized it's not likely to work, since C does not support default arguments:

.../components/esp32-camera/conversions/include/img_converters.h:122:112: error: expected ';', ',' or ')' before '=' token
  122 | bool fmt2rgb888(const uint8_t *src_buf, size_t src_len, pixformat_t format, uint8_t * rgb_buf, bool reverse_rb = false);

Do you have any ideas on how to resolve this with minimal impact?

Also see these two pieces of code _rgb_write vs. _rgb565_write: https://github.com/espressif/esp32-camera/blob/bae46be5eb126c46c692a29a953991117f68336b/conversions/to_bmp.c#L105 https://github.com/espressif/esp32-camera/blob/bae46be5eb126c46c692a29a953991117f68336b/conversions/to_bmp.c#L150 The R and B positions are different - so one of them MUST be wrong?

Since you mentioned you tested it, can you tell which example have you used for testing? Perhaps that example swaps the R and B somewhere down the line - or does not use _rgb_write at all.