[libcamera-devel] [PATCH v2 3/9] android: camera_device: Support multiple stream configurations
Niklas Söderlund
niklas.soderlund at ragnatech.se
Fri Jul 3 12:14:12 CEST 2020
Hi Kieran,
On 2020-07-03 10:30:17 +0100, Kieran Bingham wrote:
> Hi Niklas,
>
> On 03/07/2020 00:22, Niklas Söderlund wrote:
> > Hi Kieran,
> >
> > Thanks for your work.
> >
> > On 2020-07-02 22:36:48 +0100, Kieran Bingham wrote:
> >> Create an initial Camera Configuration using an empty role set, and
> >> populate the StreamConfigurations manually from each of the streams
> >> given by the Android camera3_stream_configuration_t stream_list.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >> src/android/camera_device.cpp | 51 +++++++++++++++++------------------
> >> 1 file changed, 25 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >> index b9031ff0c2a4..03dcdd520794 100644
> >> --- a/src/android/camera_device.cpp
> >> +++ b/src/android/camera_device.cpp
> >> @@ -942,6 +942,16 @@ PixelFormat CameraDevice::toPixelFormat(int format)
> >> */
> >> int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >> {
> >> + /*
> >> + * Generate an empty configuration, and construct a StreamConfiguration
> >> + * for each camera3_stream to add to it.
> >> + */
> >> + config_ = camera_->generateConfiguration();
> >> + if (!config_) {
> >> + LOG(HAL, Error) << "Failed to generate camera configuration";
> >> + return -EINVAL;
> >> + }
> >> +
> >> for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> >> camera3_stream_t *stream = stream_list->streams[i];
> >>
> >> @@ -953,35 +963,18 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >> << ", height: " << stream->height
> >> << ", format: " << utils::hex(stream->format)
> >> << " (" << format.toString() << ")";
> >> - }
> >>
> >> - /* Only one stream is supported. */
> >> - if (stream_list->num_streams != 1) {
> >> - LOG(HAL, Error) << "Only one stream supported";
> >> - return -EINVAL;
> >> - }
> >> - camera3_stream_t *camera3Stream = stream_list->streams[0];
> >> + if (!format.isValid())
> >> + return -EINVAL;
> >>
> >> - /* Translate Android format code to libcamera pixel format. */
> >> - PixelFormat format = toPixelFormat(camera3Stream->format);
> >> - if (!format.isValid())
> >> - return -EINVAL;
> >> + StreamConfiguration streamConfiguration;
> >>
> >> - /*
> >> - * Hardcode viewfinder role, replacing the generated configuration
> >> - * parameters with the ones requested by the Android framework.
> >> - */
> >> - StreamRoles roles = { StreamRole::Viewfinder };
> >> - config_ = camera_->generateConfiguration(roles);
> >> - if (!config_ || config_->empty()) {
> >> - LOG(HAL, Error) << "Failed to generate camera configuration";
> >> - return -EINVAL;
> >> - }
> >> + streamConfiguration.size.width = stream->width;
> >> + streamConfiguration.size.height = stream->height;
> >> + streamConfiguration.pixelFormat = format;
> >
> > This is not related to this patch but blindly translating a HAL format
> > using a lookup table to a libcamera format will not work for RAW. I'm
> > just curious if it is sufficient for JPEG?
> >
>
> No - it is not, but that's not tackled by this patch.
>
> Lets consider this patch as supporting multiple streams of types that
> are already supported ;-)
>
> I would expect this patch to now support multiple YUV streams.
>
>
> >>
> >> - StreamConfiguration *streamConfiguration = &config_->at(0);
> >> - streamConfiguration->size.width = camera3Stream->width;
> >> - streamConfiguration->size.height = camera3Stream->height;
> >> - streamConfiguration->pixelFormat = format;
> >> + config_->addConfiguration(streamConfiguration);
> >> + }
> >>
> >> switch (config_->validate()) {
> >> case CameraConfiguration::Valid:
> >> @@ -996,7 +989,13 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >> return -EINVAL;
> >> }
> >>
> >> - camera3Stream->max_buffers = streamConfiguration->bufferCount;
> >> + for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> >
> > Small nit, would it make sens to iterate over config_ instead of
> > stream_list->num_streams? If we have a discrepancy we would end up with
> > less streams then requested but if we use ->at(i) an exception.
>
> No... or at least - not in the /very/ near future.
>
> There will very soon be an explicit discrepency between the number of
> camera3 streams and the number of libcamera streams.
>
> Adding a JPEG stream will not add a new stream, but we must still
> support it.
>
> Later I add a mapping which stores the correct libcamera index to
> reference for this stream (for which multiple camera3 streams might
> reference a single libcamera stream index).
>
> If I iterate over config_ here, I would still need an index to map
> /back/ to the camera3_stream index, so either way an explicit for loop
> seems better here currently.
>
>
>
> >
> > This is a small thing that poped out at me, with or without this
> > changed,
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> Thanks,
>
> I hope the above makes you happier with this path...
It does, thanks for the explanation.
>
>
> >
> >> + camera3_stream_t *stream = stream_list->streams[i];
> >> + StreamConfiguration &streamConfiguration = config_->at(i);
> >> +
> >> + /* Use the bufferCount confirmed by the validation process. */
> >> + stream->max_buffers = streamConfiguration.bufferCount;
> >> + }
> >>
> >> /*
> >> * Once the CameraConfiguration has been adjusted/validated
> >> --
> >> 2.25.1
> >>
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel at lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
> >
>
> --
> Regards
> --
> Kieran
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list