[libcamera-devel] [PATCH v2 07/13] android: camera_stream: Fetch format and size from configuration

[Jacopo Mondi]

Hi Kieran

On Tue, Oct 06, 2020 at 08:53:11PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 06/10/2020 15:44, Jacopo Mondi wrote:
> > Fetch the format and size of the libcamera::StreamConfiguration
> > associated with a CameraStream by accessing the configuration by
> > index.
> >
> > This removes the need to store the libcamera stream format and sizes
> > as class members and avoid duplicating information that might get out
> > of sync.
> >
> > It also allows to remove the StreamConfiguration from the constructor
> > parameters list, as it can be identified by its index. While at it,
> > re-order the constructor parameters order.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/android/camera_device.cpp |  8 +++-----
> >  src/android/camera_stream.cpp | 15 +++++----------
> >  src/android/camera_stream.h   | 18 ++++--------------
> >  3 files changed, 12 insertions(+), 29 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 47c423195796..678dca609003 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1206,9 +1206,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  		streamConfiguration.pixelFormat = format;
> >
> >  		config_->addConfiguration(streamConfiguration);
> > -		unsigned int index = config_->size() - 1;
>
> Hrm ... personally, I think specifying that this is an index here
> explicitly helps the readability below.
>
> > -		streams_.emplace_back(this, stream, streamConfiguration,
> > -				      CameraStream::Type::Direct, index);
> > +		streams_.emplace_back(this, CameraStream::Type::Direct,
> > +				      stream, config_->size() - 1);
> >  		stream->priv = static_cast<void *>(&streams_.back());
> >  	}
> >
> > @@ -1262,8 +1261,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  			index = config_->size() - 1;
> >  		}
> >
> > -		StreamConfiguration &cfg = config_->at(index);
> > -		streams_.emplace_back(this, jpegStream, cfg, type, index);
> > +		streams_.emplace_back(this, type, jpegStream, index);
> >  		jpegStream->priv = static_cast<void *>(&streams_.back());
> >  	}
> >
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index 9c7819efb679..3946a2cdf844 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -17,18 +17,13 @@ using namespace libcamera;
> >
> >  LOG_DECLARE_CATEGORY(HAL);
> >
> > -CameraStream::CameraStream(CameraDevice *cameraDevice,
> > -			   camera3_stream_t *camera3Stream,
> > -			   const libcamera::StreamConfiguration &cfg,
> > -			   Type type, unsigned int index)
> > -	: cameraDevice_(cameraDevice), camera3Stream_(camera3Stream),
> > -	  type_(type), index_(index)
> > +CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
> > +			   camera3_stream_t *camera3Stream, unsigned int index)
> > +	: cameraDevice_(cameraDevice), type_(type),
> > +	  camera3Stream_(camera3Stream), index_(index)
> >  {
> >  	config_ = cameraDevice_->cameraConfiguration();
> >
> > -	format_ = cfg.pixelFormat;
> > -	size_ = cfg.size;
> > -
> >  	if (type_ == Type::Internal || type_ == Type::Mapped)
> >  		encoder_ = std::make_unique<EncoderLibJpeg>();
> >  }
> > @@ -63,7 +58,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source,
> >  	exif.setMake("libcamera");
> >  	exif.setModel("cameraModel");
> >  	exif.setOrientation(cameraDevice_->orientation());
> > -	exif.setSize(size_);
> > +	exif.setSize(configuration().size);
> >  	/*
> >  	 * We set the frame's EXIF timestamp as the time of encode.
> >  	 * Since the precision we need for EXIF timestamp is only one
> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > index d8d9d8c4b237..f46cfd605d03 100644
> > --- a/src/android/camera_stream.h
> > +++ b/src/android/camera_stream.h
> > @@ -106,16 +106,11 @@ public:
> >  		Internal,
> >  		Mapped,
> >  	};
> > -	CameraStream(CameraDevice *cameraDevice,
> > -		     camera3_stream_t *androidStream,
> > -		     const libcamera::StreamConfiguration &cfg,
> > -		     Type type, unsigned int index);
> > +	CameraStream(CameraDevice *cameraDevice, Type type,
> > +		     camera3_stream_t *camera3Stream, unsigned int index);
> >
> > -	const camera3_stream_t &camera3Stream() const { return *camera3Stream_; }
> > -	const libcamera::PixelFormat &format() const { return format_; }
> > -	const libcamera::Size &size() const { return size_; }
> >  	Type type() const { return type_; }
> > -
> > +	const camera3_stream_t &camera3Stream() const { return *camera3Stream_; }
> >  	const libcamera::StreamConfiguration &configuration() const;
> >  	libcamera::Stream *stream() const;
> >
> > @@ -126,13 +121,8 @@ public:
> >  private:
> >  	CameraDevice *cameraDevice_;
> >  	libcamera::CameraConfiguration *config_;
> > -	camera3_stream_t *camera3Stream_;
> >  	Type type_;
> > -
> > -	/* Libcamera facing format and sizes. */
> > -	libcamera::PixelFormat format_;
> > -	libcamera::Size size_;
> > -
> > +	camera3_stream_t *camera3Stream_;
>
> a new line here?

I considered that, but I had troubles identifying in which blocks to
place members:

        CameraDevice
        CameraConfiguration
        Type

        camera3

        /*
         * Big comment
         */
         index
         Encoder

Too sparse, why index and encoder in the same block ?

        CameraDevice
        CameraConfiguration
        Type
        camera3

        /*
         * Big comment
         */
         index
         encoder


Why index is different than others?

        CameraDevice
        CameraConfiguration
        Type
        camera3
        /*
         * Big comment
         */
         index
         encoder

This seems simpler.

>
> But nothing blocking here.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Thanks
  j

>
> >  	/*
> >  	 * The index of the libcamera StreamConfiguration as added during
> >  	 * configureStreams(). A single libcamera Stream may be used to deliver
> >
>
> --
> Regards
> --
> Kieran