Message ID | 20220802185719.380855-2-umang.jain@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Umang, Thank you for the patch. On Wed, Aug 03, 2022 at 12:27:16AM +0530, Umang Jain via libcamera-devel wrote: > Rectify the ColorSpace::Srgb to denote that it does not use > any Y'Cbcr encoding and uses full range. > > The kernel on the other hand, recommends to use Rec601 as the encoding > for V4L2_COLORSPACE_SRGB. It is not very explicit but it can be > inferred that the kernel assumes V4L2_COLORSPACE_SRGB is a YUV-encoded > one. However, when the data is in RGB, no encoding is required (and > this is denoted by YcbcrEncoding::None in libcamera). > > Hence, to be clear on the libcamera colorspace API, rectify the > ColorSpace::Srgb preset to use YcbcrEncoding::None. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/color_space.cpp | 9 +++------ > src/libcamera/v4l2_device.cpp | 2 -- > 2 files changed, 3 insertions(+), 8 deletions(-) > > diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp > index caf39760..73148228 100644 > --- a/src/libcamera/color_space.cpp > +++ b/src/libcamera/color_space.cpp > @@ -254,16 +254,13 @@ const ColorSpace ColorSpace::Jpeg = { > }; > > /** > - * \brief A constant representing the sRGB color space > - * > - * This is identical to the JPEG color space except that the Y'CbCr > - * range is limited rather than full. > + * \brief A constant representing the sRGB color space (non-YUV format) I would write "(RGB formats only)" to be even clearer. > */ > const ColorSpace ColorSpace::Srgb = { > Primaries::Rec709, > TransferFunction::Srgb, > - YcbcrEncoding::Rec601, > - Range::Limited > + YcbcrEncoding::None, > + Range::Full > }; > > /** > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 3fc8438f..a4446fbf 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -746,7 +746,6 @@ 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 }, This will cause an issue, as V4L2Device::toColorSpace() will return nullopt if the kernel reports V4L2_COLORSPACE_SRGB. You should instead use the kernel definition of the SRGB color space here: { V4L2_COLORSPACE_SRGB, { ColorSpace::Primaries::Rec709, ColorSpace::TransferFunction::Srgb, ColorSpace::YcbcrEncoding::Rec601, ColorSpace::Range::Limited } }, However, that won't be enough. V4L2Device::toColorSpace() also needs to set colorSpace.ycbcrEncoding to YcbcrEncoding::None and colorSpace.range to Range::Full if the format is an RGB format. I think it would be best to do so in a patch before this one. > { V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m }, > { V4L2_COLORSPACE_REC709, ColorSpace::Rec709 }, > { V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 }, > @@ -772,7 +771,6 @@ 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 }, This, on the other hand, should be fine. > { ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, > { ColorSpace::Rec709, V4L2_COLORSPACE_REC709 }, > { ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 },
diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp index caf39760..73148228 100644 --- a/src/libcamera/color_space.cpp +++ b/src/libcamera/color_space.cpp @@ -254,16 +254,13 @@ const ColorSpace ColorSpace::Jpeg = { }; /** - * \brief A constant representing the sRGB color space - * - * This is identical to the JPEG color space except that the Y'CbCr - * range is limited rather than full. + * \brief A constant representing the sRGB color space (non-YUV format) */ const ColorSpace ColorSpace::Srgb = { Primaries::Rec709, TransferFunction::Srgb, - YcbcrEncoding::Rec601, - Range::Limited + YcbcrEncoding::None, + Range::Full }; /** diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 3fc8438f..a4446fbf 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -746,7 +746,6 @@ 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_SMPTE170M, ColorSpace::Smpte170m }, { V4L2_COLORSPACE_REC709, ColorSpace::Rec709 }, { V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 }, @@ -772,7 +771,6 @@ 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::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, { ColorSpace::Rec709, V4L2_COLORSPACE_REC709 }, { ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 },
Rectify the ColorSpace::Srgb to denote that it does not use any Y'Cbcr encoding and uses full range. The kernel on the other hand, recommends to use Rec601 as the encoding for V4L2_COLORSPACE_SRGB. It is not very explicit but it can be inferred that the kernel assumes V4L2_COLORSPACE_SRGB is a YUV-encoded one. However, when the data is in RGB, no encoding is required (and this is denoted by YcbcrEncoding::None in libcamera). Hence, to be clear on the libcamera colorspace API, rectify the ColorSpace::Srgb preset to use YcbcrEncoding::None. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/libcamera/color_space.cpp | 9 +++------ src/libcamera/v4l2_device.cpp | 2 -- 2 files changed, 3 insertions(+), 8 deletions(-)