[libcamera-devel] [PATCH v5 1/3] android: camera_device: Introduce Camera3StreamConfig

[Laurent Pinchart]

Hi Hiro and Kieran,

On Wed, Dec 09, 2020 at 10:31:09AM +0000, Kieran Bingham wrote:
> On 09/12/2020 05:54, Hirokazu Honda wrote:
> > Camera3StreamConfig is a new class to store camera3_stream and
> > types with associated StreamConfiguration.
> > 
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> Perhaps this could be squashed with 2/3 which uses it - but w're at v5
> here, so I don't think it's worth it now.
> 
> It looks like this series is likely ready for merging, so no need to
> repost just to squash.
> 
> > ---
> >  src/android/camera_device.cpp | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 675af570..09269d95 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -128,6 +128,18 @@ const std::map<int, const Camera3Format> camera3FormatsMap = {
> >  	},
> >  };
> > 
> > +/*
> > + * \struct Camera3StreamConfig
> > + * \brief Data to store StreamConfiguration associated with camera3_stream(s).
> > + * \var streams List of streams requested by Android HAL client.
> > + * \var types List of CameraStream::Type associated with streams.
> > + * \var config StreamConfiguration for streams.
> > + */
> > +struct Camera3StreamConfig {
> > +	std::vector<camera3_stream_t*> streams;
> > +	std::vector<CameraStream::Type> types;
> 
> As far as i can tell, in 2/3 both streams and types are always used at
> the same index?
> 
> If that's the case, I would have preferred to see them grouped directly
> so that it is enforced.
> 
> That could either be by putting a std::pair in the vector (though I'm
> not sure if that gets a bit ugly) or grouping them together in a struct
> ... but then we have yet another type...
> 
> So - I'm not going to say that's needed right now - but I'm just
> cautious of adding lots of multiple data types that require the same
> indexing, rather than putting the grouped data together, and stored
> inside a single data structure...
> 
> In this case, I don't think it matters as it's just semantics for
> accessing the data.

Certainly not a blocker, so,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

I would however also prefer grouping the two in the same vector.
Something like the following could do.

struct Camera3StreamConfig {
	struct Camera3Stream {
		camera3_stream_t *stream;
		CameraStream::Type type;
	};

	StreamConfiguration config;
	std::vector<Camera3Stream> streams;
};

At some point I think we need to go through the HAL implementation to
clean up the naming scheme, it's not very consistent. A common prefix
for data types relating to the HAL, as well as a naming scheme for
variables, would be nice.

> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > +	StreamConfiguration config;
> > +};
> >  } /* namespace */
> > 
> >  LOG_DECLARE_CATEGORY(HAL)

-- 
Regards,

Laurent Pinchart