| Message ID | 1560253031-98823-1-git-send-email-mickael.guene@st.com |
|---|---|
| State | Accepted |
| Commit | 846d4c7d3ee7ca059074b2c2d3aeaa941c3f0870 |
| Headers | show |
| Series |
|
| Related | show |
Hi Mickael, thanks for the patch 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/ > 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. > > Signed-off-by: Mickael Guene <mickael.guene@st.com> Following this morning clarification on irc: Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > > 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; > -- > 2.7.4 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@st.com> > > Following this morning clarification on irc: > Reviewed-by: Jacopo Mondi <jacopo@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;
Hi Jacopo, It's ok for me if you update with commit message changed. regards Mickael On 6/11/19 14:50, Jacopo Mondi wrote: > Hi Mickael, > thanks for the patch > > 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/ >> 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.
Hi Laurent, On Tue, Jun 11, 2019 at 04:25:52PM +0300, Laurent Pinchart wrote: > 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." > Agreed. Would you like to give your tag or should I apply the patch right away? Thanks j > > > Signed-off-by: Mickael Guene <mickael.guene@st.com> > > > > Following this morning clarification on irc: > > Reviewed-by: Jacopo Mondi <jacopo@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
Hi Jacopo, On Wed, Jun 12, 2019 at 10:00:46AM +0200, Jacopo Mondi wrote: > On Tue, Jun 11, 2019 at 04:25:52PM +0300, Laurent Pinchart wrote: > > 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." > > Agreed. Would you like to give your tag or should I apply the patch > right away? I'm pushed it already :-) > >>> Signed-off-by: Mickael Guene <mickael.guene@st.com> > >> > >> Following this morning clarification on irc: > >> Reviewed-by: Jacopo Mondi <jacopo@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;
Hi Laurent On Wed, Jun 12, 2019 at 11:12:00AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Wed, Jun 12, 2019 at 10:00:46AM +0200, Jacopo Mondi wrote: > > On Tue, Jun 11, 2019 at 04:25:52PM +0300, Laurent Pinchart wrote: > > > 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." > > > > Agreed. Would you like to give your tag or should I apply the patch > > right away? > > I'm pushed it already :-) > Hope you have not been pushed too hard :p Thanks, I didn't notice it was on master already. Thanks j > > >>> Signed-off-by: Mickael Guene <mickael.guene@st.com> > > >> > > >> Following this morning clarification on irc: > > >> Reviewed-by: Jacopo Mondi <jacopo@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
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;
According to the documentation, CameraSensor::getFormat() should select Media bus code from mbusCodes parameter list. It should select the first code from the list that is supported by the sensor. Current implementation will wrongly select Media bus code from mbusCodes_ order instead. This patch aims to fix this wrong behavior. Signed-off-by: Mickael Guene <mickael.guene@st.com> --- src/libcamera/camera_sensor.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)