| Message ID | 20220817211824.20980-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@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. > * > @@ -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; }); >
On 8/17/22 23:18, 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@ideasonboard.com> > --- > > 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 ? We have that colorspace: V4L2_COLORSPACE_470_SYSTEM_BG, however modern SDTV transmissions use V4L2_COLORSPACE_SMPTE170M these days. The differences between the two are next to impossible to see. However, I think you misinterpreted the JFIF standard: it does not specify that the BT 601 chromaticities/transfer function should be used, it only uses the YCbCr-RGB conversion that BT.601 describes. Note 3 in section 7 of JFIF is relevant here: "NOTE 3 − As this Recommendation | International Standard is based on the prior informally-circulated JFIF version 1.02 specification that was produced in 1992, which referenced Rec. ITU-R BT.601 (formerly CCIR 601), it references that specification for definition of the E' Y, E' , and E' signals that correspond to the YCBCR values specified herein. However, since the development of the prior JFIF version 1.02 specification, additional industry specifications have been developed, Rec. ITU-R BT.601 has been updated, and common industry practice has emerged which often follows the sYCC specification in IEC 61966-2-1/Amd.1. The difference between the use of the colour interpretation specification in this Recommendation | International Standard and that of the sYCC specification may be considered negligible in practice. Moreover, as previously noted, the colour space specification herein can provide only a basic level of colour fidelity. The use of supplemental metadata such as an ICC profile (e.g., as specified in ISO 15076-1) may be necessary to provide a more accurate colour characterization." In other words, it recommends that sYCC is followed as the default. Note that I believe that the V4L2_COLORSPACE_JPEG define predates the sYCC standard, so there was a good reason why SYCC wasn't used. :-) I wonder if it would make sense to make a V4L2_COLORSPACE_SYCC as well. Probably more effort than it is worth, though. Regards, Hans > > --- > 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. > * > @@ -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; }); >
Hi Hans, On Fri, Aug 19, 2022 at 11:12:56AM +0200, Hans Verkuil wrote: > On 8/17/22 23:18, 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@ideasonboard.com> > > --- > > > > 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 ? > > We have that colorspace: V4L2_COLORSPACE_470_SYSTEM_BG, however modern Indeed, my bad. > SDTV transmissions use V4L2_COLORSPACE_SMPTE170M these days. > > The differences between the two are next to impossible to see. > > However, I think you misinterpreted the JFIF standard: it does not specify > that the BT 601 chromaticities/transfer function should be used, it only > uses the YCbCr-RGB conversion that BT.601 describes. > > Note 3 in section 7 of JFIF is relevant here: > > "NOTE 3 − As this Recommendation | International Standard is based on the prior informally-circulated JFIF version 1.02 > specification that was produced in 1992, which referenced Rec. ITU-R BT.601 (formerly CCIR 601), it references that > specification for definition of the E' Y, E' , and E' signals that correspond to the YCBCR values specified herein. However, > since the development of the prior JFIF version 1.02 specification, additional industry specifications have been developed, Rec. > ITU-R BT.601 has been updated, and common industry practice has emerged which often follows the sYCC specification in IEC > 61966-2-1/Amd.1. The difference between the use of the colour interpretation specification in this Recommendation | > International Standard and that of the sYCC specification may be considered negligible in practice. Moreover, as previously > noted, the colour space specification herein can provide only a basic level of colour fidelity. The use of supplemental metadata > such as an ICC profile (e.g., as specified in ISO 15076-1) may be necessary to provide a more accurate colour characterization." > > In other words, it recommends that sYCC is followed as the default. > > Note that I believe that the V4L2_COLORSPACE_JPEG define predates the sYCC standard, > so there was a good reason why SYCC wasn't used. :-) That's possible, I'll stop short of researching this :-) > I wonder if it would make sense to make a V4L2_COLORSPACE_SYCC as well. Probably > more effort than it is worth, though. For V4L2 I think it's likely more effort than it's worth, yes. For libcamera we'll use sYCC. > > --- > > 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. > > * > > @@ -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; });
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@ideasonboard.com> > > > Reviewed-by: Umang Jain <umang.jain@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; }); >>
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@ideasonboard.com> > > > > > > Reviewed-by: Umang Jain <umang.jain@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; }); > >>
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. * @@ -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; });
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@ideasonboard.com> --- 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(-)