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

[Hirokazu Honda]

Hi Laurent,

On Wed, Sep 22, 2021 at 9:54 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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

s/practive/practice/.

>                  * 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.
>

The comment desciption looks good. Thanks.

-Hiro
> > > > +              */
> > > > +#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