[libcamera-devel,v2,06/12] libcamera: camera_configuration: Return config index

9467 diff mbox series

When adding a new StreamConfiguration to a CameraConfiguration instance
it might be useful to know how what is the index of the newly added item
to be able to retrieve it later.

Return the index of the newly inserted item, which corresponds to the
0-indexed number of items in the CameraConfiguration::config_ vector,
from CameraConfiguration::addConfiguration().

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/camera.h | 2 +-
 src/libcamera/camera.cpp   | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

Hi Jacopo,

Thank you for the patch.

On Wed, Sep 02, 2020 at 05:22:30PM +0200, Jacopo Mondi wrote:
> When adding a new StreamConfiguration to a CameraConfiguration instance
> it might be useful to know how what is the index of the newly added item
> to be able to retrieve it later.
> 
> Return the index of the newly inserted item, which corresponds to the
> 0-indexed number of items in the CameraConfiguration::config_ vector,
> from CameraConfiguration::addConfiguration().

Looking at the rest of the series, this is only used in 07/12 to replace
a local counter. I'm not sure if it will also be useful for other users
of the API, so I have no strong opinion. Please however note that you
could already use CameraConfiguration::size() to replace the counter,
without this patch. I'll trust you to decide which course of action is
best (and the camera configuration API will be reworked, so this may not
matter that much).

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/camera.h | 2 +-
>  src/libcamera/camera.cpp   | 5 ++++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 272c12c3c473..53c32afa23a5 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -39,7 +39,7 @@ public:
>  
>  	virtual ~CameraConfiguration();
>  
> -	void addConfiguration(const StreamConfiguration &cfg);
> +	int addConfiguration(const StreamConfiguration &cfg);
>  	virtual Status validate() = 0;
>  
>  	StreamConfiguration &at(unsigned int index);
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 4a9c19c33cfb..9a8923620e24 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -104,10 +104,13 @@ CameraConfiguration::~CameraConfiguration()
>  /**
>   * \brief Add a stream configuration to the camera configuration
>   * \param[in] cfg The stream configuration
> + * \return The index of the newly added stream configuration
>   */
> -void CameraConfiguration::addConfiguration(const StreamConfiguration &cfg)
> +int CameraConfiguration::addConfiguration(const StreamConfiguration &cfg)
>  {
>  	config_.push_back(cfg);
> +
> +	return config_.size() - 1;
>  }
>  
>  /**

The return value is useful if the index of added point is not always
the deterministic place, like std::map.
However, like std::vector, the adding point is always the same, end of
vector, per the implementation.
If it is guaranteed at the interface level, I think the return value
is unnecessary.

On Sun, Sep 6, 2020 at 6:30 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Sep 02, 2020 at 05:22:30PM +0200, Jacopo Mondi wrote:
> > When adding a new StreamConfiguration to a CameraConfiguration instance
> > it might be useful to know how what is the index of the newly added item
> > to be able to retrieve it later.
> >
> > Return the index of the newly inserted item, which corresponds to the
> > 0-indexed number of items in the CameraConfiguration::config_ vector,
> > from CameraConfiguration::addConfiguration().
>
> Looking at the rest of the series, this is only used in 07/12 to replace
> a local counter. I'm not sure if it will also be useful for other users
> of the API, so I have no strong opinion. Please however note that you
> could already use CameraConfiguration::size() to replace the counter,
> without this patch. I'll trust you to decide which course of action is
> best (and the camera configuration API will be reworked, so this may not
> matter that much).
>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  include/libcamera/camera.h | 2 +-
> >  src/libcamera/camera.cpp   | 5 ++++-
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 272c12c3c473..53c32afa23a5 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -39,7 +39,7 @@ public:
> >
> >       virtual ~CameraConfiguration();
> >
> > -     void addConfiguration(const StreamConfiguration &cfg);
> > +     int addConfiguration(const StreamConfiguration &cfg);
> >       virtual Status validate() = 0;
> >
> >       StreamConfiguration &at(unsigned int index);
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 4a9c19c33cfb..9a8923620e24 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -104,10 +104,13 @@ CameraConfiguration::~CameraConfiguration()
> >  /**
> >   * \brief Add a stream configuration to the camera configuration
> >   * \param[in] cfg The stream configuration
> > + * \return The index of the newly added stream configuration
> >   */
> > -void CameraConfiguration::addConfiguration(const StreamConfiguration &cfg)
> > +int CameraConfiguration::addConfiguration(const StreamConfiguration &cfg)
> >  {
> >       config_.push_back(cfg);
> > +
> > +     return config_.size() - 1;
> >  }
> >
> >  /**
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel