[libcamera-devel] [PATCH] libcamera: color_space: Rename Jpeg to Sycc
Umang Jain
umang.jain at ideasonboard.com
Mon Aug 22 17:22:42 CEST 2022
Hi Laurent,
One more comment
On 8/18/22 7:31 PM, Umang Jain via libcamera-devel wrote:
> Hi Laurent
>
> Thank you for the patch and detailed commit message.
>
> On 8/18/22 02:48, Laurent Pinchart wrote:
>> The JPEG color space is badly name, as the JPEG specification (ITU-T
>> T.81) doesn't define any particular color space:
>>
>> The interchange format does not specify a complete coded image
>> representation. Application-dependent information, e.g. colour
>> space, is outside the scope of this Specification.
>>
>> The JFIF specification (ITU-T T.871) is clearer as it requires ITU-R
>> BT.601 YCbCr encoding and a full quantization range:
>>
>> The interpretations of Y, CB, and CR are derived from the E'Y, E'Cb,
>> and E'Cr signals defined in the 625-line specification of Rec. ITU-R
>> BT.601, but these signals are normalized so as to permit the usage of
>> the full range of 256 levels of the 8-bit binary encoding of the Y
>> component.
>>
>> It however doesn't specify color primaries or a transfer function
>> explicitly. It only mentions the latter when describing the conversion
>> from YCbCr to RGB:
>>
>> The inverse relationship for computing full scale 8-bit per colour
>> channel gamma pre-corrected RGB values (following Rec. ITU-R BT.601
>> gamma pre-correction and colour primary specifications) from YCbCr
>> colours (with 256 levels per component) can be computed as follows:
>> [...]
>>
>> Given that ITU-R BT.601-5 (1995) didn't specify color primaries or a
>> transfer function, and that the later ITU-R BT.601-7 (2011) version
>> specifies color primaries for the 625-line variant that do not match
>> sRGB, the JPEG color space in libcamera is badly named.
>>
>> Rename the color space to sYCC, as its definition matches the sYCC
>> standard, and indicate that it is typically used to encode JPEG images.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>
> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
>
> CC'ing David from RPi especially (for heads-up) because this patch
> will affects libcamera-apps.
>
>> ---
>>
>> Hans, following our discussion on IRC, I double-check the JFIF standard,
>> and my findings are described in the commit message above. It seems that
>> naming the color space "JPEG" in V4L2 is actually incorrect, as JFIF
>> expects the 626-line ITU-R BT.601 chromaticities for the primary colors,
>> which are slightly different than sRGB (the 525-line chromaticities
>> match SMPTE 170M and SMPTE 240M, but not sRGB either). It seems that we
>> are missing a V4L2_COLORSPACE_* value to describe the 625-line ITU-R
>> BT.601 chromaticities, is that an oversight ?
>>
>> ---
>> include/libcamera/color_space.h | 2 +-
>> src/libcamera/color_space.cpp | 28 +++++++++----------
>> .../pipeline/raspberrypi/raspberrypi.cpp | 10 +++----
>> src/libcamera/v4l2_device.cpp | 4 +--
>> src/py/libcamera/py_main.cpp | 2 +-
>> 5 files changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/include/libcamera/color_space.h
>> b/include/libcamera/color_space.h
>> index 0d39fbc02220..8030a264c66f 100644
>> --- a/include/libcamera/color_space.h
>> +++ b/include/libcamera/color_space.h
>> @@ -46,8 +46,8 @@ public:
>> }
>> static const ColorSpace Raw;
>> - static const ColorSpace Jpeg;
>> static const ColorSpace Srgb;
>> + static const ColorSpace Sycc;
>> static const ColorSpace Smpte170m;
>> static const ColorSpace Rec709;
>> static const ColorSpace Rec2020;
>> diff --git a/src/libcamera/color_space.cpp
>> b/src/libcamera/color_space.cpp
>> index caf397607b10..6ace40dcb0fb 100644
>> --- a/src/libcamera/color_space.cpp
>> +++ b/src/libcamera/color_space.cpp
>> @@ -121,8 +121,8 @@ namespace libcamera {
>> * \brief Assemble and return a readable string representation of the
>> * ColorSpace
>> *
>> - * If the color space matches a standard ColorSpace (such as
>> ColorSpace::Jpeg)
>> - * then the short name of the color space ("JPEG") is returned.
>> Otherwise
>> + * If the color space matches a standard ColorSpace (such as
>> ColorSpace::Sycc)
>> + * then the short name of the color space ("sYCC") is returned.
>> Otherwise
>> * the four constituent parts of the ColorSpace are assembled into
>> a longer
>> * string.
Below this paragraph, we have
* - <a
href="https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-details.html#col-jpeg">JPEG</a>
as well.
We will probably in tricky situation here. Do we want to drop the JPEG
link entirely, or change to
* - <a
href="https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-details.html#col-jpeg">sYCC</a>
Can you add your thoughts here and update the patch (no need to re-send
I think, but if you decide to push it...)
(It will be more tricky for sRGB too, as we will not be going with
kernel's definition)
>> *
>> @@ -134,8 +134,8 @@ std::string ColorSpace::toString() const
>> static const std::array<std::pair<ColorSpace, const char *>,
>> 6> colorSpaceNames = { {
>> { ColorSpace::Raw, "RAW" },
>> - { ColorSpace::Jpeg, "JPEG" },
>> { ColorSpace::Srgb, "sRGB" },
>> + { ColorSpace::Sycc, "sYCC" },
>> { ColorSpace::Smpte170m, "SMPTE170M" },
>> { ColorSpace::Rec709, "Rec709" },
>> { ColorSpace::Rec2020, "Rec2020" },
>> @@ -242,17 +242,6 @@ const ColorSpace ColorSpace::Raw = {
>> Range::Full
>> };
>> -/**
>> - * \brief A constant representing the JPEG color space used for
>> - * encoding JPEG images
>> - */
>> -const ColorSpace ColorSpace::Jpeg = {
>> - Primaries::Rec709,
>> - TransferFunction::Srgb,
>> - YcbcrEncoding::Rec601,
>> - Range::Full
>> -};
>> -
>> /**
>> * \brief A constant representing the sRGB color space
>> *
>> @@ -266,6 +255,17 @@ const ColorSpace ColorSpace::Srgb = {
>> Range::Limited
>> };
>> +/**
>> + * \brief A constant representing the sYCC color space, typically
>> used for
>> + * encoding JPEG images
>> + */
>> +const ColorSpace ColorSpace::Sycc = {
>> + Primaries::Rec709,
>> + TransferFunction::Srgb,
>> + YcbcrEncoding::Rec601,
>> + Range::Full
>> +};
>> +
>> /**
>> * \brief A constant representing the SMPTE170M color space
>> */
>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> index e895584d4fbc..b4094898ca6c 100644
>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> @@ -593,11 +593,11 @@ CameraConfiguration
>> *PipelineHandlerRPi::generateConfiguration(Camera *camera,
>> fmts = data->isp_[Isp::Output0].dev()->formats();
>> pixelFormat = formats::NV12;
>> /*
>> - * Still image codecs usually expect the JPEG color space.
>> + * Still image codecs usually expect the sYCC color space.
>> * Even RGB codecs will be fine as the RGB we get with the
>> - * JPEG color space is the same as sRGB.
>> + * sYCC color space is the same as sRGB.
>> */
>> - colorSpace = ColorSpace::Jpeg;
>> + colorSpace = ColorSpace::Sycc;
>> /* Return the largest sensor resolution. */
>> size = sensorSize;
>> bufferCount = 1;
>> @@ -628,7 +628,7 @@ CameraConfiguration
>> *PipelineHandlerRPi::generateConfiguration(Camera *camera,
>> case StreamRole::Viewfinder:
>> fmts = data->isp_[Isp::Output0].dev()->formats();
>> pixelFormat = formats::ARGB8888;
>> - colorSpace = ColorSpace::Jpeg;
>> + colorSpace = ColorSpace::Sycc;
>> size = { 800, 600 };
>> bufferCount = 4;
>> outCount++;
>> @@ -835,7 +835,7 @@ int PipelineHandlerRPi::configure(Camera *camera,
>> CameraConfiguration *config)
>> format.size = maxSize;
>> format.fourcc = dev->toV4L2PixelFormat(formats::YUV420);
>> /* No one asked for output, so the color space doesn't
>> matter. */
>> - format.colorSpace = ColorSpace::Jpeg;
>> + format.colorSpace = ColorSpace::Sycc;
>> ret = dev->setFormat(&format);
>> if (ret) {
>> LOG(RPI, Error)
>> diff --git a/src/libcamera/v4l2_device.cpp
>> b/src/libcamera/v4l2_device.cpp
>> index 3fc8438f6579..b22a981ff1a6 100644
>> --- a/src/libcamera/v4l2_device.cpp
>> +++ b/src/libcamera/v4l2_device.cpp
>> @@ -745,8 +745,8 @@ void V4L2Device::eventAvailable()
>> static const std::map<uint32_t, ColorSpace> v4l2ToColorSpace = {
>> { V4L2_COLORSPACE_RAW, ColorSpace::Raw },
>> - { V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg },
>> { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb },
>> + { V4L2_COLORSPACE_JPEG, ColorSpace::Sycc },
>> { V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m },
>> { V4L2_COLORSPACE_REC709, ColorSpace::Rec709 },
>> { V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 },
>> @@ -771,8 +771,8 @@ static const std::map<uint32_t,
>> ColorSpace::Range> v4l2ToRange = {
>> static const std::vector<std::pair<ColorSpace, v4l2_colorspace>>
>> colorSpaceToV4l2 = {
>> { ColorSpace::Raw, V4L2_COLORSPACE_RAW },
>> - { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },
>> { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },
>> + { ColorSpace::Sycc, V4L2_COLORSPACE_JPEG },
>> { ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M },
>> { ColorSpace::Rec709, V4L2_COLORSPACE_REC709 },
>> { ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 },
>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
>> index 505cc3dc1a0d..2a58e34a7cdb 100644
>> --- a/src/py/libcamera/py_main.cpp
>> +++ b/src/py/libcamera/py_main.cpp
>> @@ -602,8 +602,8 @@ PYBIND11_MODULE(_libcamera, m)
>> .def_readwrite("ycbcrEncoding", &ColorSpace::ycbcrEncoding)
>> .def_readwrite("range", &ColorSpace::range)
>> .def_static("Raw", []() { return ColorSpace::Raw; })
>> - .def_static("Jpeg", []() { return ColorSpace::Jpeg; })
>> .def_static("Srgb", []() { return ColorSpace::Srgb; })
>> + .def_static("Sycc", []() { return ColorSpace::Sycc; })
>> .def_static("Smpte170m", []() { return
>> ColorSpace::Smpte170m; })
>> .def_static("Rec709", []() { return ColorSpace::Rec709; })
>> .def_static("Rec2020", []() { return ColorSpace::Rec2020; });
>>
More information about the libcamera-devel
mailing list