[libcamera-devel] [PATCH] libcamera: Fix CameraSensor::getFormat() search order

[Laurent Pinchart]

Hi Jacopo,

On Tue, Jun 11, 2019 at 02:50:18PM +0200, Jacopo Mondi wrote:
> On Tue, Jun 11, 2019 at 01:37:11PM +0200, Mickael Guene wrote:
> >  According to the documentation, CameraSensor::getFormat() should select Media
> > bus code from mbusCodes parameter list. It should select the first code from the
> 
> s/from mbusCodes parameter list
>  /from mbusCodes parameters in descresing preference order/

This is redundant with the second sentence.

> > list that is supported by the sensor. Current implementation will wrongly select
> 
> s/Current/However, the current/
> 
> > Media bus code from mbusCodes_ order instead.
> >  This patch aims to fix this wrong behavior.
> 
> I would change the last line with
> 
> "Fix this by first iterating on the mbus code list provided as parameter
> instead of iterating on the sensor supported formats".
> 
> If you agree with the change to the commit message, it could be
> updated when applying the patch.

I propose the following text.

"According to the documentation, the CameraSensor::getFormat() method 
should select the first media bus code from the mbusCodes parameter that 
is supported by the sensor. However, the current implementation wrongly
selects the first media bus code from the codes supported by the sensor
that is listed in the mbusCodes parameter. This results in the
preference order specified by the caller being ignored. Fix it."

> > Signed-off-by: Mickael Guene <mickael.guene at st.com>
> 
> Following this morning clarification on irc:
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > ---
> >
> >  src/libcamera/camera_sensor.cpp | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 2b9d8fa..cb6649e 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -191,8 +191,8 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu
> >  {
> >  	V4L2SubdeviceFormat format{};
> >
> > -	for (unsigned int code : mbusCodes_) {
> > -		if (std::any_of(mbusCodes.begin(), mbusCodes.end(),
> > +	for (unsigned int code : mbusCodes) {
> > +		if (std::any_of(mbusCodes_.begin(), mbusCodes_.end(),
> >  				[code](unsigned int c) { return c == code; })) {
> >  			format.mbus_code = code;
> >  			break;

-- 
Regards,

Laurent Pinchart