[libcamera-devel] [PATCH] libcamera: color_space: Rename Jpeg to Sycc
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Aug 22 17:33:49 CEST 2022
Hi Umang,
On Mon, Aug 22, 2022 at 08:52:42PM +0530, Umang Jain wrote:
> 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
I assume you mean above.
> * - <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)
Good points. There are also two other occurrence of "JPEG". I'll apply
the following changes for v2.
diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp
index 6ace40dcb0fb..877c03848db0 100644
--- a/src/libcamera/color_space.cpp
+++ b/src/libcamera/color_space.cpp
@@ -29,21 +29,28 @@ namespace libcamera {
* (sometimes also referred to as the quantisation) of the color space.
*
* Certain combinations of these fields form well-known standard color
- * spaces such as "JPEG" or "REC709".
+ * spaces such as "sRGB" or "Rec709".
*
* In the strictest sense a "color space" formally only refers to the
* color primaries and white point. Here, however, the ColorSpace class
* adopts the common broader usage that includes the transfer function,
* Y'CbCr encoding method and quantisation.
*
- * For more information on the specific color spaces described here, please
- * see:
+ * More information on color spaces is available in the V4L2 documentation, see
+ * in particular
*
* - <a href="https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-details.html#col-srgb">sRGB</a>
* - <a href="https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-details.html#col-jpeg">JPEG</a>
* - <a href="https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-details.html#col-smpte-170m">SMPTE 170M</a>
* - <a href="https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-details.html#col-rec709">Rec.709</a>
* - <a href="https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-details.html#col-bt2020">Rec.2020</a>
+ *
+ * Note that there is no guarantee of a 1:1 mapping between color space names
+ * and definitions in libcamera and V4L2. A notable difference is that the sYCC
+ * libcamera color space is called JPEG in V4L2 due to historical reasons.
+ *
+ * \todo Define the color space fully in the libcamera API to avoid referencing
+ * V4L2
*/
/**
@@ -245,7 +252,7 @@ const ColorSpace ColorSpace::Raw = {
/**
* \brief A constant representing the sRGB color space
*
- * This is identical to the JPEG color space except that the Y'CbCr
+ * This is identical to the sYCC color space except that the Y'CbCr
* range is limited rather than full.
*/
const ColorSpace ColorSpace::Srgb = {
The "Note that ..." part should be expanded in the patch that redefines
sRGB to list that difference as well.
> >> *
> >> @@ -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; });
> >>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list