<div dir="ltr"><div dir="ltr">Hi David,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 25 Oct 2021 at 10:37, David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</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 Naush<br>
<br>
Thanks for the feedback!<br>
<br>
On Mon, 25 Oct 2021 at 08:45, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> Hi David,<br>
><br>
> Thank you for your patch.<br>
><br>
> On Wed, 20 Oct 2021 at 12:08, David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>> wrote:<br>
>><br>
>> This is so that applications can choose appropriate color spaces which<br>
>> will then be passed down to the V4L2 devices.<br>
>><br>
>> There are two fields: the "requestedColorSpace" which is the one an<br>
>> application wants, and the "actualColorSpace" which is what the<br>
>> underlying hardware can deliver, and which is filled in by the<br>
>> validate() method.<br>
>><br>
>> Signed-off-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
>> ---<br>
>>  include/libcamera/stream.h |  4 ++++<br>
>>  src/libcamera/stream.cpp   | 25 +++++++++++++++++++++++++<br>
>>  2 files changed, 29 insertions(+)<br>
>><br>
>> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h<br>
>> index 0c55e716..fe491ff5 100644<br>
>> --- a/include/libcamera/stream.h<br>
>> +++ b/include/libcamera/stream.h<br>
>> @@ -12,6 +12,7 @@<br>
>>  #include <string><br>
>>  #include <vector><br>
>><br>
>> +#include <libcamera/color_space.h><br>
>>  #include <libcamera/framebuffer.h><br>
>>  #include <libcamera/geometry.h><br>
>>  #include <libcamera/pixel_format.h><br>
>> @@ -47,6 +48,9 @@ struct StreamConfiguration {<br>
>><br>
>>         unsigned int bufferCount;<br>
>><br>
>> +       ColorSpace requestedColorSpace;<br>
>> +       ColorSpace actualColorSpace;<br>
>> +<br>
><br>
><br>
> I had a brief look-ahead in this series, but it was not immediately<br>
> obvious to me why we would need requested and actual? Could<br>
> we make do with just one and update the user requested value<br>
> with what the HW will actually give us?<br>
<br>
Yes, this is an important point so we need to understand if this is necessary!<br>
<br>
The thing that was bothering me here was if the application asks for<br>
some colour space but the driver gives us something else that the<br>
ColorSpace class doesn't recognise, so we get "undefined" back, at<br>
least in some of the fields.<br>
<br>
Now, if the application sees that validate() returns "adjusted", but<br>
thinks "whatever, I'll just go with it", then having "undefined" in<br>
the colour space that we now request is actually an error. (Or at<br>
least, I've defined it to be such.)<br>
<br>
Alternatively, if you look at the returned colour space, see that it's<br>
"fully defined", you can now set "requested = actual" and try<br>
validate() again, and it won't get "adjusted" (on account of the<br>
colour space, anyway).<br>
<br>
Does this make any sense, or am I being way too devious?<br></blockquote><div><br></div><div>Yes, that reasoning makes sense.  I suppose we can achieve the same result with</div><div>a single member variable, but add a tad more logic to the applications. I have no strong </div><div>inclination either way.</div><div><br></div><div>Naush</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>
Thanks!<br>
<br>
Davis<br>
<br>
><br>
>><br>
>>         Stream *stream() const { return stream_; }<br>
>>         void setStream(Stream *stream) { stream_ = stream; }<br>
>>         const StreamFormats &formats() const { return formats_; }<br>
>> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp<br>
>> index b421e17e..1ddbbb8c 100644<br>
>> --- a/src/libcamera/stream.cpp<br>
>> +++ b/src/libcamera/stream.cpp<br>
>> @@ -329,6 +329,31 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)<br>
>>   * \brief Requested number of buffers to allocate for the stream<br>
>>   */<br>
>><br>
>> +/**<br>
>> + * \var StreamConfiguration::requestedColorSpace<br>
>> + * \brief The ColorSpace this stream should have<br>
>> + *<br>
>> + * The generateConfiguration method will generate reasonable default<br>
>> + * values (ColorSpace::Jpeg for stills, ColorSpace::Rec709 for video and<br>
>> + * ColorSpace::Raw for raw streams) but applications are free to overwrite<br>
>> + * this value.<br>
>> + */<br>
>> +<br>
>> +/**<br>
>> + * \var StreamConfiguration::actualColorSpace<br>
>> + * \brief The ColorSpace the will be used for this stream<br>
>> + *<br>
>> + * This field is filled in by CameraConfiguration::validate().<br>
>> + * Normally this should match the requestedColorSpace, but it may differ<br>
>> + * if the hardware does not support it.<br>
>> + *<br>
>> + * In general cameras may have different constraints here, for example,<br>
>> + * they may require all output streams to share the same color space.<br>
>> + * Sometimes the fields within this color space may report "Undefined"<br>
>> + * values if the hardware drivers are going to use a color space that<br>
>> + * is not recognised by the ColorSpace class.<br>
>> + */<br>
>> +<br>
>>  /**<br>
>>   * \fn StreamConfiguration::stream()<br>
>>   * \brief Retrieve the stream associated with the configuration<br>
>> --<br>
>> 2.20.1<br>
>><br>
</blockquote></div></div>