libcamera: stream: Add color space to configuration string representation
diff mbox series

Message ID 20250424144713.14279-1-laurent.pinchart@ideasonboard.com
State Accepted
Commit 9b50d3c23dea1bc2882cd3e6566a3d4cb9f7296f
Headers show
Series
  • libcamera: stream: Add color space to configuration string representation
Related show

Commit Message

Laurent Pinchart April 24, 2025, 2:47 p.m. UTC 
Extend the string representation of StreamConfiguration, as returned by
the toString() and operator<<() functions, with color space information.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/stream.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


base-commit: 3569fed7afc16fe31dfbccbfbaeb72a741bc8973
--
Regards,

Laurent Pinchart

Comments

Barnabás Pőcze April 29, 2025, 4:49 p.m. UTC | #1 
Hi

2025. 04. 29. 16:42 keltezéssel, Laurent Pinchart írta:
> On Fri, Apr 25, 2025 at 08:23:06AM +0100, Kieran Bingham wrote:
>> Quoting Laurent Pinchart (2025-04-24 15:47:13)
>>> Extend the string representation of StreamConfiguration, as returned by
>>> the toString() and operator<<() functions, with color space information.
>>
>> This looks reasonable and helpful.
>>
>> It would be nice to see here (or after the ---) what the change
>> looks like in the logs to make it easy to comprehend the update.
> 
> Here an example.
> 
> Before:
> 
> [0:03:53.962430875] [222]  INFO Camera camera.cpp:1205 configuring streams: (0) 1920x1080-YUYV
> 
> After:
> 
> [0:03:53.962430875] [222]  INFO Camera camera.cpp:1205 configuring streams: (0) 1920x1080-YUYV/sYCC

I am wondering if maybe a different separator wouldn't have been better
as such outputs are possible now:

   INFO Camera camera.cpp:1258 configuring streams: (0) 1280x720-MJPEG/Rec709/Rec709/Rec601/Limited

I think the above is not immediately clear.


Regards,
Barnabás Pőcze

> 
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> ---
>>>   src/libcamera/stream.cpp | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
>>> index 978d72752b00..f091487c9353 100644
>>> --- a/src/libcamera/stream.cpp
>>> +++ b/src/libcamera/stream.cpp
>>> @@ -407,7 +407,8 @@ std::string StreamConfiguration::toString() const
>>>    */
>>>   std::ostream &operator<<(std::ostream &out, const StreamConfiguration &cfg)
>>>   {
>>> -       out << cfg.size << "-" << cfg.pixelFormat;
>>> +       out << cfg.size << "-" << cfg.pixelFormat << "/"
>>> +           << ColorSpace::toString(cfg.colorSpace);
>>>          return out;
>>>   }
>>>
>>>
>>> base-commit: 3569fed7afc16fe31dfbccbfbaeb72a741bc8973
>
Laurent Pinchart April 29, 2025, 5:17 p.m. UTC | #2 
On Tue, Apr 29, 2025 at 06:49:24PM +0200, Barnabás Pőcze wrote:
> 2025. 04. 29. 16:42 keltezéssel, Laurent Pinchart írta:
> > On Fri, Apr 25, 2025 at 08:23:06AM +0100, Kieran Bingham wrote:
> >> Quoting Laurent Pinchart (2025-04-24 15:47:13)
> >>> Extend the string representation of StreamConfiguration, as returned by
> >>> the toString() and operator<<() functions, with color space information.
> >>
> >> This looks reasonable and helpful.
> >>
> >> It would be nice to see here (or after the ---) what the change
> >> looks like in the logs to make it easy to comprehend the update.
> > 
> > Here an example.
> > 
> > Before:
> > 
> > [0:03:53.962430875] [222]  INFO Camera camera.cpp:1205 configuring streams: (0) 1920x1080-YUYV
> > 
> > After:
> > 
> > [0:03:53.962430875] [222]  INFO Camera camera.cpp:1205 configuring streams: (0) 1920x1080-YUYV/sYCC
> 
> I am wondering if maybe a different separator wouldn't have been better
> as such outputs are possible now:
> 
>    INFO Camera camera.cpp:1258 configuring streams: (0) 1280x720-MJPEG/Rec709/Rec709/Rec601/Limited
> 
> I think the above is not immediately clear.

What separator would you recommend ?

> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> ---
> >>>   src/libcamera/stream.cpp | 3 ++-
> >>>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> >>> index 978d72752b00..f091487c9353 100644
> >>> --- a/src/libcamera/stream.cpp
> >>> +++ b/src/libcamera/stream.cpp
> >>> @@ -407,7 +407,8 @@ std::string StreamConfiguration::toString() const
> >>>    */
> >>>   std::ostream &operator<<(std::ostream &out, const StreamConfiguration &cfg)
> >>>   {
> >>> -       out << cfg.size << "-" << cfg.pixelFormat;
> >>> +       out << cfg.size << "-" << cfg.pixelFormat << "/"
> >>> +           << ColorSpace::toString(cfg.colorSpace);
> >>>          return out;
> >>>   }
> >>>
> >>>
> >>> base-commit: 3569fed7afc16fe31dfbccbfbaeb72a741bc8973
Barnabás Pőcze April 29, 2025, 5:48 p.m. UTC | #3 
2025. 04. 29. 19:17 keltezéssel, Laurent Pinchart írta:
> On Tue, Apr 29, 2025 at 06:49:24PM +0200, Barnabás Pőcze wrote:
>> 2025. 04. 29. 16:42 keltezéssel, Laurent Pinchart írta:
>>> On Fri, Apr 25, 2025 at 08:23:06AM +0100, Kieran Bingham wrote:
>>>> Quoting Laurent Pinchart (2025-04-24 15:47:13)
>>>>> Extend the string representation of StreamConfiguration, as returned by
>>>>> the toString() and operator<<() functions, with color space information.
>>>>
>>>> This looks reasonable and helpful.
>>>>
>>>> It would be nice to see here (or after the ---) what the change
>>>> looks like in the logs to make it easy to comprehend the update.
>>>
>>> Here an example.
>>>
>>> Before:
>>>
>>> [0:03:53.962430875] [222]  INFO Camera camera.cpp:1205 configuring streams: (0) 1920x1080-YUYV
>>>
>>> After:
>>>
>>> [0:03:53.962430875] [222]  INFO Camera camera.cpp:1205 configuring streams: (0) 1920x1080-YUYV/sYCC
>>
>> I am wondering if maybe a different separator wouldn't have been better
>> as such outputs are possible now:
>>
>>     INFO Camera camera.cpp:1258 configuring streams: (0) 1280x720-MJPEG/Rec709/Rec709/Rec601/Limited
>>
>> I think the above is not immediately clear.
> 
> What separator would you recommend ?

I don't know. I imagine many options could work, e.g. just `-`,
which is already used to separate the size and the pixel format.
But I see this patch has already been merged; and it is probably
not a significant issue.


Regards,
Barnabás Pőcze

> 
>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>>
>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>> ---
>>>>>    src/libcamera/stream.cpp | 3 ++-
>>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
>>>>> index 978d72752b00..f091487c9353 100644
>>>>> --- a/src/libcamera/stream.cpp
>>>>> +++ b/src/libcamera/stream.cpp
>>>>> @@ -407,7 +407,8 @@ std::string StreamConfiguration::toString() const
>>>>>     */
>>>>>    std::ostream &operator<<(std::ostream &out, const StreamConfiguration &cfg)
>>>>>    {
>>>>> -       out << cfg.size << "-" << cfg.pixelFormat;
>>>>> +       out << cfg.size << "-" << cfg.pixelFormat << "/"
>>>>> +           << ColorSpace::toString(cfg.colorSpace);
>>>>>           return out;
>>>>>    }
>>>>>
>>>>>
>>>>> base-commit: 3569fed7afc16fe31dfbccbfbaeb72a741bc8973
>

Patch
diff mbox series 

diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
index 978d72752b00..f091487c9353 100644
--- a/src/libcamera/stream.cpp
+++ b/src/libcamera/stream.cpp
@@ -407,7 +407,8 @@  std::string StreamConfiguration::toString() const
  */
 std::ostream &operator<<(std::ostream &out, const StreamConfiguration &cfg)
 {
-	out << cfg.size << "-" << cfg.pixelFormat;
+	out << cfg.size << "-" << cfg.pixelFormat << "/"
+	    << ColorSpace::toString(cfg.colorSpace);
 	return out;
 }