[libcamera-devel] [PATCH 5/5] android: camera_device: List RAW resolutions

[Jacopo Mondi]

Hi Kieran,

On Wed, Sep 02, 2020 at 02:15:44PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 02/09/2020 11:47, Jacopo Mondi wrote:
> > The resolutions supported for the RAW formats cannot be tested from
> > a list of known sizes like the processed ones. This is mainly due to the
> > fact RAW streams are produced by capturing frames at the CSI-2 receiver
> > output and their size corresponds to the sensor's native sizes.
> >
> > In order to obtain the RAW frame size generate a temporary
> > CameraConfiguration for the Role::StillCaptureRAW role and inspect the
> > map of StreamFormats returned by the pipeline handler.
>
> Presumably, it will also be heavily dependant upon the actual chosen
> streams from the other requested streams ... and only a subset might
> really be available when a full configuration is used?

Not sure I got you here :/
>
>
>
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 21 +++++++++++++++++----
> >  src/android/camera_device.h   |  2 ++
> >  2 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 765c3292e4f3..181fca83988d 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -314,6 +314,17 @@ std::vector<Size> CameraDevice::listProcessedResolutions(CameraConfiguration *ca
> >  	return supportedResolutions;
> >  }
> >
> > +std::vector<Size> CameraDevice::listRawResolutions(const libcamera::PixelFormat &pixelFormat)
> > +{
>
> same comment on the function name, that we're filtering (rather than
> 'listing' valid resolutions. Though maybe this one is subtly
> different... but I'd keep both namings consistent either way.

Well, we're actually listing them here :D
And anyway, there's not much filtering going on here, maybe a bit more
in the corresponding non-RAW implementation as we actually test a list
of sizes and report only the supported ones.

In any case, I would keep the naming of the two functions consistent
whatever we chose.

>
> > +	std::unique_ptr<CameraConfiguration> cameraConfig =
> > +		camera_->generateConfiguration({ StillCaptureRaw });
> > +	StreamConfiguration &cfg = cameraConfig->at(0);
> > +	const StreamFormats &formats = cfg.formats();
> > +	std::vector<Size> supportedResolutions = formats.sizes(pixelFormat);
>
> I'm a bit weary of this.
>
> If you apply this function to a UVC camera for example, it would simply
> return a list of sizes which represent the available frame sizes...
>

Pardon my ignorance, does uvc support RAW ?

> Scratch that, I see it's filtered by pixelFormat ... ok - so I guess
> that works.
>
>
> I wonder if this couldn't also be handled by the other filter fucntion
> but I assume not easily, as you've done this ;-)

Well, they do two different things. I actually considered doing what
I'm doing here (building on top of StreamFormats) for non-RAW stream,
but it turned out to be a whack-a-mole game of finding the right Role
to request to have the sizes for the format we are interested in. And
that gets pretty pipeline-implementation-dependent as there's no
strict rule on what formats corresponds to a role. So I might be
looking for supported NV12 sizes and my best bet is to ask for
Viewfinder and see what StreamFormats are returned, but it's a bet and
some pipeline handler might only support RGB for viewfinder and NV12
for StillCapture (it's an example, not sure it makes any sense) so I
could end up testing all roles, filtering double results, re-ordering
etc... Seems like a no-go to me.

The other way around is possible as well: list RAW sizes by using the
same method as it's used for non-RAW ones, with the difference that we
accept adjusted sizes, as the pipeline handler will adjust to the
closest available sensor frame size for each tested image resolution.
Again, it seemd sub-optimal and requires filtering doubles and testing
an un-necessary number of times, so I created two separate functions
in the end.

Be aware that in the back of my mind there's also a listJPEGSizes()
that queries the encoder for the reasons explained in my other reply,
if that makes any sense :)

Thanks
  j


>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
> > +
> > +	return supportedResolutions;
> > +}
> > +
> >  /*
> >   * Initialize the format conversion map to translate from Android format
> >   * identifier to libcamera pixel formats and fill in the list of supported
> > @@ -466,13 +477,15 @@ int CameraDevice::initializeStreamConfigurations()
> >  				<< camera3Format.name << " to: "
> >  				<< mappedFormat.toString();
> >
> > +		std::vector<Size> resolutions;
> >  		const PixelFormatInfo &info = PixelFormatInfo::info(mappedFormat);
> >  		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> > -			continue;
> > +			resolutions = listRawResolutions(mappedFormat);
> > +		else
> > +			resolutions = listProcessedResolutions(cameraConfig.get(),
> > +							       mappedFormat,
> > +							       cameraResolutions);
> >
> > -		std::vector<Size> resolutions = listProcessedResolutions(cameraConfig.get(),
> > -									 mappedFormat,
> > -									 cameraResolutions);
> >  		for (const Size &res : resolutions) {
> >  			streamConfigurations_.push_back({ res, androidFormat });
> >
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 18fd5ff03cde..230e89b011e6 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -97,6 +97,8 @@ private:
> >  	listProcessedResolutions(libcamera::CameraConfiguration *cameraConfig,
> >  				 const libcamera::PixelFormat &pixelFormat,
> >  				 const std::vector<libcamera::Size> &resolutions);
> > +	std::vector<libcamera::Size>
> > +	listRawResolutions(const libcamera::PixelFormat &pixelFormat);
> >
> >  	std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
> >  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
> >
>
> --
> Regards
> --
> Kieran