<div dir="ltr"><div dir="ltr">Hi all,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 25 Jul 2022 at 13:22, Nicolas Dufresne via libcamera-devel <<a href="mailto:libcamera-devel@lists.libcamera.org">libcamera-devel@lists.libcamera.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi David,<br>
<br>
Le lundi 25 juillet 2022 à 11:46 +0100, David Plowman a écrit :<br>
> Hi Umang<br>
> <br>
> Thanks for this patch. I was wondering if I could ask for a little<br>
> more information on how it would be used.<br>
> <br>
> In the original libcamera ColorSpace implementation I was very keen to<br>
> avoid "unknown" or "default" values inside ColorSpaces. I was taking<br>
> the view that applications really should say what ColorSpace they<br>
> want, but at the same time I knew there will always be (bad)<br>
> applications that don't think about colour spaces and so I let them<br>
> use std::optional to allow the ColorSpace to be completely<br>
> unspecified.<br>
<br>
"wrong" application is a bit of an over statement. Even for cameras, the<br>
colorspace is not always something that can be freely picked by the application.<br>
The V4L2 driver, or the HW may have some limitations. A simple example is UVC,<br>
you pretty much get what you get and you can't influence it. For this reason, a<br>
proper colorspace API needs to work both ways. Which basically imply, an<br>
application should be able to not care about selecting / influencing the<br>
colorspace and just read whatever the driver/HW produces.<br></blockquote><div><br></div><div>Wouldn't the pipeline_handler::validate() take care of this? It ought to know the</div><div>sensor output colourspace, and return that regardless of what the application</div><div>may have requested. Apologies if I got this wrong.</div><div><br></div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
regards,<br>
Nicolas<br>
<br>
> <br>
> In V4L2, my understanding is that "_DEFAULT" normally means that you<br>
> infer the value from the overall V4L2_COLORSPACE (someone please<br>
> correct me if that's wrong!). In libcamera we don't have an "overall<br>
> colour space" in this way, so what would it then mean? For example, is<br>
> "ColorSpace::Range::Default" ever the same as<br>
> "ColorSpace::Range::Full", or is it always different? How do we (can<br>
> we even) document what it means?<br>
> <br>
> I'd be interested to hear what other folks think. Possibly I'm being a<br>
> bit paranoid, but then I have been bitten by "wrong colour space"<br>
> problems often enough in the past (usually just before a product<br>
> ships!).<br>
> <br>
> Thanks!<br>
> David<br>
> <br>
> On Sun, 24 Jul 2022 at 15:44, Umang Jain via libcamera-devel<br>
> <<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a>> wrote:<br>
> > <br>
> > Add a Colorspace::Default which corresponds to default V4L2<br>
> > colorspace identifiers.<br>
> > <br>
> > \todo Add defaults to python colorspace bindings<br>
> > <br>
> > Signed-off-by: Umang Jain <<a href="mailto:umang.jain@ideasonboard.com" target="_blank">umang.jain@ideasonboard.com</a>><br>
> > ---<br>
> > include/libcamera/color_space.h | 5 +++++<br>
> > src/libcamera/color_space.cpp | 29 ++++++++++++++++++++++++++++-<br>
> > src/libcamera/v4l2_device.cpp | 5 +++++<br>
> > 3 files changed, 38 insertions(+), 1 deletion(-)<br>
> > <br>
> > diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h<br>
> > index 086c56c1..0ad8da17 100644<br>
> > --- a/include/libcamera/color_space.h<br>
> > +++ b/include/libcamera/color_space.h<br>
> > @@ -16,6 +16,7 @@ class ColorSpace<br>
> > {<br>
> > public:<br>
> > enum class Primaries {<br>
> > + Default,<br>
> > Raw,<br>
> > Smpte170m,<br>
> > Rec709,<br>
> > @@ -23,12 +24,14 @@ public:<br>
> > };<br>
> > <br>
> > enum class TransferFunction {<br>
> > + Default,<br>
> > Linear,<br>
> > Srgb,<br>
> > Rec709,<br>
> > };<br>
> > <br>
> > enum class YcbcrEncoding {<br>
> > + Default,<br>
> > None,<br>
> > Rec601,<br>
> > Rec709,<br>
> > @@ -36,6 +39,7 @@ public:<br>
> > };<br>
> > <br>
> > enum class Range {<br>
> > + Default,<br>
> > Full,<br>
> > Limited,<br>
> > };<br>
> > @@ -45,6 +49,7 @@ public:<br>
> > {<br>
> > }<br>
> > <br>
> > + static const ColorSpace Default;<br>
> > static const ColorSpace Raw;<br>
> > static const ColorSpace Jpeg;<br>
> > static const ColorSpace Srgb;<br>
> > diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp<br>
> > index 895e5c8e..d52f58cf 100644<br>
> > --- a/src/libcamera/color_space.cpp<br>
> > +++ b/src/libcamera/color_space.cpp<br>
> > @@ -50,6 +50,9 @@ namespace libcamera {<br>
> > * \enum ColorSpace::Primaries<br>
> > * \brief The color primaries for this color space<br>
> > *<br>
> > + * \var ColorSpace::Primaries::Default<br>
> > + * \brief Use the default primaries as defined by the driver<br>
> > + *<br>
> > * \var ColorSpace::Primaries::Raw<br>
> > * \brief These are raw colors directly from a sensor, the primaries are<br>
> > * unspecified<br>
> > @@ -68,6 +71,9 @@ namespace libcamera {<br>
> > * \enum ColorSpace::TransferFunction<br>
> > * \brief The transfer function used for this color space<br>
> > *<br>
> > + * \var ColorSpace::TransferFunction::Default<br>
> > + * \brief Use the default transfer function as defined by the driver<br>
> > + *<br>
> > * \var ColorSpace::TransferFunction::Linear<br>
> > * \brief This color space uses a linear (identity) transfer function<br>
> > *<br>
> > @@ -82,6 +88,9 @@ namespace libcamera {<br>
> > * \enum ColorSpace::YcbcrEncoding<br>
> > * \brief The Y'CbCr encoding<br>
> > *<br>
> > + * \var ColorSpace::YcbcrEncoding::Default<br>
> > + * \brief Use the default Y'CbCr encoding as defined by the driver<br>
> > + *<br>
> > * \var ColorSpace::YcbcrEncoding::None<br>
> > * \brief There is no defined Y'CbCr encoding (used for non-YUV formats)<br>
> > *<br>
> > @@ -99,6 +108,9 @@ namespace libcamera {<br>
> > * \enum ColorSpace::Range<br>
> > * \brief The range (sometimes "quantisation") for this color space<br>
> > *<br>
> > + * \var ColorSpace::Range::Default<br>
> > + * Use the default range as defined by the driver<br>
> > + *<br>
> > * \var ColorSpace::Range::Full<br>
> > * \brief This color space uses full range pixel values<br>
> > *<br>
> > @@ -132,7 +144,8 @@ std::string ColorSpace::toString() const<br>
> > {<br>
> > /* Print out a brief name only for standard color spaces. */<br>
> > <br>
> > - static const std::array<std::pair<ColorSpace, const char *>, 6> colorSpaceNames = { {<br>
> > + static const std::array<std::pair<ColorSpace, const char *>, 7> colorSpaceNames = { {<br>
> > + { ColorSpace::Default, "Default" },<br>
> > { ColorSpace::Raw, "RAW" },<br>
> > { ColorSpace::Jpeg, "JPEG" },<br>
> > { ColorSpace::Srgb, "sRGB" },<br>
> > @@ -150,23 +163,27 @@ std::string ColorSpace::toString() const<br>
> > /* Assemble a name made of the constituent fields. */<br>
> > <br>
> > static const std::map<Primaries, std::string> primariesNames = {<br>
> > + { Primaries::Default, "Default" },<br>
> > { Primaries::Raw, "RAW" },<br>
> > { Primaries::Smpte170m, "SMPTE170M" },<br>
> > { Primaries::Rec709, "Rec709" },<br>
> > { Primaries::Rec2020, "Rec2020" },<br>
> > };<br>
> > static const std::map<TransferFunction, std::string> transferNames = {<br>
> > + { TransferFunction::Default, "Default" },<br>
> > { TransferFunction::Linear, "Linear" },<br>
> > { TransferFunction::Srgb, "sRGB" },<br>
> > { TransferFunction::Rec709, "Rec709" },<br>
> > };<br>
> > static const std::map<YcbcrEncoding, std::string> encodingNames = {<br>
> > + { YcbcrEncoding::Default, "Default" },<br>
> > { YcbcrEncoding::None, "None" },<br>
> > { YcbcrEncoding::Rec601, "Rec601" },<br>
> > { YcbcrEncoding::Rec709, "Rec709" },<br>
> > { YcbcrEncoding::Rec2020, "Rec2020" },<br>
> > };<br>
> > static const std::map<Range, std::string> rangeNames = {<br>
> > + { Range::Default, "Default" },<br>
> > { Range::Full, "Full" },<br>
> > { Range::Limited, "Limited" },<br>
> > };<br>
> > @@ -232,6 +249,16 @@ std::string ColorSpace::toString(const std::optional<ColorSpace> &colorSpace)<br>
> > * \brief The pixel range used with by color space<br>
> > */<br>
> > <br>
> > +/**<br>
> > + * \brief A constant representing a default color space<br>
> > + */<br>
> > +const ColorSpace ColorSpace::Default = {<br>
> > + Primaries::Default,<br>
> > + TransferFunction::Default,<br>
> > + YcbcrEncoding::Default,<br>
> > + Range::Default<br>
> > +};<br>
> > +<br>
> > /**<br>
> > * \brief A constant representing a raw color space (from a sensor)<br>
> > */<br>
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp<br>
> > index 3fc8438f..ecfcf337 100644<br>
> > --- a/src/libcamera/v4l2_device.cpp<br>
> > +++ b/src/libcamera/v4l2_device.cpp<br>
> > @@ -770,6 +770,7 @@ static const std::map<uint32_t, ColorSpace::Range> v4l2ToRange = {<br>
> > };<br>
> > <br>
> > static const std::vector<std::pair<ColorSpace, v4l2_colorspace>> colorSpaceToV4l2 = {<br>
> > + { ColorSpace::Default, V4L2_COLORSPACE_DEFAULT },<br>
> > { ColorSpace::Raw, V4L2_COLORSPACE_RAW },<br>
> > { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },<br>
> > { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },<br>
> > @@ -779,6 +780,7 @@ static const std::vector<std::pair<ColorSpace, v4l2_colorspace>> colorSpaceToV4l<br>
> > };<br>
> > <br>
> > static const std::map<ColorSpace::Primaries, v4l2_colorspace> primariesToV4l2 = {<br>
> > + { ColorSpace::Primaries::Default, V4L2_COLORSPACE_DEFAULT },<br>
> > { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },<br>
> > { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },<br>
> > { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },<br>
> > @@ -786,18 +788,21 @@ static const std::map<ColorSpace::Primaries, v4l2_colorspace> primariesToV4l2 =<br>
> > };<br>
> > <br>
> > static const std::map<ColorSpace::TransferFunction, v4l2_xfer_func> transferFunctionToV4l2 = {<br>
> > + { ColorSpace::TransferFunction::Default, V4L2_XFER_FUNC_DEFAULT },<br>
> > { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },<br>
> > { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },<br>
> > { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },<br>
> > };<br>
> > <br>
> > static const std::map<ColorSpace::YcbcrEncoding, v4l2_ycbcr_encoding> ycbcrEncodingToV4l2 = {<br>
> > + { ColorSpace::YcbcrEncoding::Default, V4L2_YCBCR_ENC_DEFAULT },<br>
> > { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },<br>
> > { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },<br>
> > { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },<br>
> > };<br>
> > <br>
> > static const std::map<ColorSpace::Range, v4l2_quantization> rangeToV4l2 = {<br>
> > + { ColorSpace::Range::Default, V4L2_QUANTIZATION_DEFAULT },<br>
> > { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },<br>
> > { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },<br>
> > };<br>
> > --<br>
> > 2.31.1<br>
> > <br>
<br>
</blockquote></div></div>