[libcamera-devel] [PATCH v2 2/2] android: Log stream types using PixelFormat::toString()

[Laurent Pinchart]

On Thu, Nov 25, 2021 at 09:53:24PM +0000, Kieran Bingham wrote:
> Quoting Umang Jain (2021-11-25 14:25:01)
> > On 11/25/21 7:46 PM, Kieran Bingham wrote:
> > > Quoting Laurent Pinchart (2021-11-25 13:49:48)
> > >> On Thu, Nov 25, 2021 at 01:28:11PM +0000, Kieran Bingham wrote:
> > >>> Quoting Umang Jain (2021-11-25 13:12:24)
> > >>>> Log stream types using PixelFormat::toString() instead of hex values,
> > >>>> since that is more human readable.
> > >>>>
> > >>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > >>>> ---
> > >>>>   src/android/camera_capabilities.cpp | 5 +++--
> > >>>>   src/android/camera_device.cpp       | 2 +-
> > >>>>   2 files changed, 4 insertions(+), 3 deletions(-)
> > >>>>
> > >>>> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > >>>> index f357902e..71f302c4 100644
> > >>>> --- a/src/android/camera_capabilities.cpp
> > >>>> +++ b/src/android/camera_capabilities.cpp
> > >>>> @@ -713,7 +713,8 @@ int CameraCapabilities::initializeStreamConfigurations()
> > >>>>          LOG(HAL, Debug) << "Collected stream configuration map: ";
> > >>>>          for (const auto &entry : streamConfigurations_)
> > >>>>                  LOG(HAL, Debug) << "{ " << entry.resolution.toString() << " - "
> > >>>> -                               << utils::hex(entry.androidFormat) << " }";
> > >>>> +                               << toPixelFormat(entry.androidFormat).toString()
> > >>>
> > >>> Do all androidFormats get correctly converted to libcamera formats
> > >>> currently?
> > >>>
> > >>> This might still need to print the Android format, or have an android
> > >>> specific string function... (Or print both the android hex string, and
> > >>> our representation of it?)
> > >>
> > >> That's also my concern. If we receive from the camera service a format
> > >> we don't support, printing an "unknown" or similar name hinders
> > >> debugging. I'd rather keep the android format in hex in the locations
> > >> before we validate it.
> > >
> > > I think we should have both. Or only print the hex if it's unknown ...
> > 
> > In that case, the hex is printed if the format is unknown as part of 
> > Error logging. It is done through CameraCapabilites::toPixelFormat()
> 
> In which case, I'd be content with this.
> 
> Laurent?

I'm sorry, but it still bothers me a bit :-S The mapping between Android
formats and libcamera formats isn't 1:1, we map multiple Android formats
to NV21 for instance. With this patch we lose the ability to tell the
different Android formats apart.

The Camera3Format structure contains a name member. If we want to print
a name instead of the hex value, we should add a function that will use
the camera3FormatsMap to lookup the name and return something like
"Unknown (0x....)" when the format isn't known.

> > >>>> +                               << " }";
> > >>>>   
> > >>>>          return 0;
> > >>>>   }
> > >>>> @@ -1303,7 +1304,7 @@ int CameraCapabilities::initializeStaticMetadata()
> > >>>>                  minFrameDurations.push_back(entry.minFrameDurationNsec);
> > >>>>   
> > >>>>                  LOG(HAL, Debug)
> > >>>> -                       << "Output Stream: " << utils::hex(entry.androidFormat)
> > >>>> +                       << "Output Stream: " << toPixelFormat(entry.androidFormat).toString()
> > >>>>                          << " (" << entry.resolution.toString() << ")["
> > >>>>                          << entry.minFrameDurationNsec << "]"
> > >>>>                          << "@" << fps;
> > >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > >>>> index ad4bf07c..cc6dccc1 100644
> > >>>> --- a/src/android/camera_device.cpp
> > >>>> +++ b/src/android/camera_device.cpp
> > >>>> @@ -566,7 +566,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > >>>>                                 << ", direction: " << directionToString(stream->stream_type)
> > >>>>                                 << ", width: " << stream->width
> > >>>>                                 << ", height: " << stream->height
> > >>>> -                              << ", format: " << utils::hex(stream->format)
> > >>>> +                              << ", format: " << format.toString()
> > 
> > Also, the 'format' here comes from
> > 
> >          PixelFormat format = capabilities_.toPixelFormat(stream->format);
> > 
> > a few lines above.
> > 
> > >>>>                                 << ", rotation: " << rotationToString(stream->rotation)
> > >>>>   #if defined(OS_CHROMEOS)
> > >>>>                                 << ", crop_rotate_scale_degrees: "

-- 
Regards,

Laurent Pinchart