[libcamera-devel] [PATCH v4 3/3] android: camera_device: Configure one stream for identical stream requests

[Laurent Pinchart]

Hi Hiro,

Sorry for the late reply.

On Fri, Sep 03, 2021 at 06:11:30PM +0900, Hirokazu Honda wrote:
> On Fri, Sep 3, 2021 at 5:36 PM Laurent Pinchart wrote:
> > On Wed, Sep 01, 2021 at 05:03:02PM +0900, Hirokazu Honda wrote:
> > > An Android HAL client may request identical stream requests. It is
> >
> > s/identical stream requests/multiple identical streams/
> >
> > > redundant that a native camera device produces a separate stream for
> > > each of the identical requests.
> > > Configure camera one stream configuration for the identical stream
> > > requests.
> >
> > And here I'd write
> >
> > Configure the camera with a single stream in that case. The other
> > identical HAL streams will be produced by the YUV post-processor.
> >
> > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  src/android/camera_device.cpp | 28 +++++++++++++++++++++++++++-
> > >  1 file changed, 27 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 324b997f..51d5370e 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -618,12 +618,38 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > >                       continue;
> > >               }
> > >
> > > +             /*
> > > +              * Always add GRALLOC_USAGE_HW_CAMERA_WRITE to the usage on
> > > +              * ChromeOS because cros::CameraBufferManager imports the stream
> > > +              * buffer with the usage.
> >
> > I'm still not sure to follow you. What happens if you don't set
> > GRALLOC_USAGE_HW_CAMERA_WRITE ?
> 
> This gmb_bo_import call fails. That is, the buffer cannot be mapped
> and even constructing CameraBuffer fails.
> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc;l=669;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e
> 
> Well, GBM_BO_USE_CAMERA_WRITE may be unnecessary, but I can understand
> they want to add it for safety.

Right. This bothers me a bit, but it's not your fault :-) The gralloc
usage flags are supposed to indicate expected usage of a buffer to allow
selection of a suitable memory allocation strategy compatible with all
devices involved. In practice, it's also used to pick pixel formats, so
we need to specify GRALLOC_USAGE_HW_CAMERA_WRITE for all buffers that
will be filled by the camera HAL, or the wrong format will be selected
(or allocation could fail completely). That's a design deficiency of the
gralloc and camera HAL APIs in my opinion, but not something we can
address.

I expect the same to be true for other Android system, so I think it
would make sense to set GRALLOC_USAGE_HW_CAMERA_WRITE unconditionally.
How about this ?

		/*
		 * While gralloc usage flags are supposed to report usage
		 * patterns to select a suitable buffer allocation strategy, in
		 * practive they're also used to make other decisions, such as
		 * selecting the actual format for the IMPLEMENTATION_DEFINED
		 * HAL pixel format. To avoid issues, we thus have to set the
		 * GRALLOC_USAGE_HW_CAMERA_WRITE flag unconditionally, even for
		 * streams that will be produced in software.
		 */
		stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;

If you're fine with this, I'll push the series with this change.

> > > +              */
> > > +#if defined(OS_CHROMEOS)
> > > +             stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;
> > > +#endif
> > > +
> > > +             /*
> > > +              * If a CameraStream with the same size and format of the
> > > +              * current stream has already been requested, associate the two.
> > > +              */
> > > +             auto iter = std::find_if(
> > > +                     streamConfigs.begin(), streamConfigs.end(),
> > > +                     [size, format](const Camera3StreamConfig &streamConfig) {
> > > +                             return streamConfig.config.size == size &&
> > > +                                    streamConfig.config.pixelFormat == format;
> > > +                     });
> > > +             if (iter != streamConfigs.end()) {
> > > +                     /* Add usage to copy the buffer in streams[0] to stream. */
> > > +                     iter->streams[0].stream->usage |= GRALLOC_USAGE_SW_READ_OFTEN;
> > > +                     stream->usage |= GRALLOC_USAGE_SW_WRITE_OFTEN;
> > > +                     iter->streams.push_back({ stream, CameraStream::Type::Mapped });
> > > +                     continue;
> > > +             }
> > > +
> > >               Camera3StreamConfig streamConfig;
> > >               streamConfig.streams = { { stream, CameraStream::Type::Direct } };
> > >               streamConfig.config.size = size;
> > >               streamConfig.config.pixelFormat = format;
> > >               streamConfigs.push_back(std::move(streamConfig));
> > > -
> >
> > This isn't needed.
> >
> > >               /* This stream will be produced by hardware. */
> > >               stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;
> > >       }

-- 
Regards,

Laurent Pinchart