| Message ID | 20201202115253.14705-2-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for your patch. On Wed, 2 Dec 2020 at 11:53, David Plowman <david.plowman@raspberrypi.com> wrote: > This makes it possible to tell whether we're starting the sensor for > the first time, or whether it's happening because of a mode switch or > because the camera has been paused and re-started. Depending on this, > some sensors may require us to drop different numbers of frames. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/raspberrypi/raspberrypi.cpp | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > index 69be5e4e..b8298768 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -67,7 +67,7 @@ public: > IPARPi() > : lastMode_({}), controller_(), controllerInit_(false), > frameCount_(0), checkCount_(0), mistrustCount_(0), > - lsTable_(nullptr) > + lsTable_(nullptr), firstStart_(true) > { > } > > @@ -145,6 +145,9 @@ private: > /* LS table allocation passed in from the pipeline handler. */ > FileDescriptor lsTableHandle_; > void *lsTable_; > + > + /* Distinguish the first camera start from others. */ > + bool firstStart_; > }; > > int IPARPi::init(const IPASettings &settings) > @@ -179,6 +182,8 @@ int IPARPi::start(const IPAOperationData &ipaConfig, > IPAOperationData *result) > result->operation |= RPi::IPA_CONFIG_SENSOR; > } > > + firstStart_ = false; > + > return 0; > } > > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi David, Thank you for the patch. On Wed, Dec 02, 2020 at 11:52:49AM +0000, David Plowman wrote: > This makes it possible to tell whether we're starting the sensor for > the first time, or whether it's happening because of a mode switch or > because the camera has been paused and re-started. Depending on this, > some sensors may require us to drop different numbers of frames. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> This looks good to me, but I'd squash it with patch 2/5 as the chance is very small and it's easier to review it when also seeing how the new variable is used. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> On a side note, how does mode change and initial start differ ? Don't we stop the sensor when reconfiguring the camera ? > --- > src/ipa/raspberrypi/raspberrypi.cpp | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 69be5e4e..b8298768 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -67,7 +67,7 @@ public: > IPARPi() > : lastMode_({}), controller_(), controllerInit_(false), > frameCount_(0), checkCount_(0), mistrustCount_(0), > - lsTable_(nullptr) > + lsTable_(nullptr), firstStart_(true) > { > } > > @@ -145,6 +145,9 @@ private: > /* LS table allocation passed in from the pipeline handler. */ > FileDescriptor lsTableHandle_; > void *lsTable_; > + > + /* Distinguish the first camera start from others. */ > + bool firstStart_; > }; > > int IPARPi::init(const IPASettings &settings) > @@ -179,6 +182,8 @@ int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result) > result->operation |= RPi::IPA_CONFIG_SENSOR; > } > > + firstStart_ = false; > + > return 0; > } >
Hi Laurent Thanks for the review. On Sat, 5 Dec 2020 at 19:57, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi David, > > Thank you for the patch. > > On Wed, Dec 02, 2020 at 11:52:49AM +0000, David Plowman wrote: > > This makes it possible to tell whether we're starting the sensor for > > the first time, or whether it's happening because of a mode switch or > > because the camera has been paused and re-started. Depending on this, > > some sensors may require us to drop different numbers of frames. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > This looks good to me, but I'd squash it with patch 2/5 as the chance is > very small and it's easier to review it when also seeing how the new > variable is used. Will do! > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > On a side note, how does mode change and initial start differ ? Don't we > stop the sensor when reconfiguring the camera ? The difference is that on first camera start, the algorithms are starting "from scratch", with no idea what initial values to use. On subsequent mode switches, the algorithms are known to be in a sensible place and we just leave them be (we don't reset them, though the change of camera mode can some some other effects). Best regards David > > > --- > > src/ipa/raspberrypi/raspberrypi.cpp | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > index 69be5e4e..b8298768 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -67,7 +67,7 @@ public: > > IPARPi() > > : lastMode_({}), controller_(), controllerInit_(false), > > frameCount_(0), checkCount_(0), mistrustCount_(0), > > - lsTable_(nullptr) > > + lsTable_(nullptr), firstStart_(true) > > { > > } > > > > @@ -145,6 +145,9 @@ private: > > /* LS table allocation passed in from the pipeline handler. */ > > FileDescriptor lsTableHandle_; > > void *lsTable_; > > + > > + /* Distinguish the first camera start from others. */ > > + bool firstStart_; > > }; > > > > int IPARPi::init(const IPASettings &settings) > > @@ -179,6 +182,8 @@ int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result) > > result->operation |= RPi::IPA_CONFIG_SENSOR; > > } > > > > + firstStart_ = false; > > + > > return 0; > > } > > > > -- > Regards, > > Laurent Pinchart
Hi David, On Sat, Dec 05, 2020 at 10:39:55PM +0000, David Plowman wrote: > On Sat, 5 Dec 2020 at 19:57, Laurent Pinchart wrote: > > On Wed, Dec 02, 2020 at 11:52:49AM +0000, David Plowman wrote: > > > This makes it possible to tell whether we're starting the sensor for > > > the first time, or whether it's happening because of a mode switch or > > > because the camera has been paused and re-started. Depending on this, > > > some sensors may require us to drop different numbers of frames. > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > This looks good to me, but I'd squash it with patch 2/5 as the chance is > > very small and it's easier to review it when also seeing how the new > > variable is used. > > Will do! > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > On a side note, how does mode change and initial start differ ? Don't we > > stop the sensor when reconfiguring the camera ? > > The difference is that on first camera start, the algorithms are > starting "from scratch", with no idea what initial values to use. On > subsequent mode switches, the algorithms are known to be in a sensible > place and we just leave them be (we don't reset them, though the > change of camera mode can some some other effects). Ah yes that makes sense. What if the application stops the camera and restarts it "a long time" later ? Would we need some kind of timeout after which algorithms should drop their state (or at least consider the state doesn't shorten the convergence time anymore) ? > > > --- > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > index 69be5e4e..b8298768 100644 > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > @@ -67,7 +67,7 @@ public: > > > IPARPi() > > > : lastMode_({}), controller_(), controllerInit_(false), > > > frameCount_(0), checkCount_(0), mistrustCount_(0), > > > - lsTable_(nullptr) > > > + lsTable_(nullptr), firstStart_(true) > > > { > > > } > > > > > > @@ -145,6 +145,9 @@ private: > > > /* LS table allocation passed in from the pipeline handler. */ > > > FileDescriptor lsTableHandle_; > > > void *lsTable_; > > > + > > > + /* Distinguish the first camera start from others. */ > > > + bool firstStart_; > > > }; > > > > > > int IPARPi::init(const IPASettings &settings) > > > @@ -179,6 +182,8 @@ int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result) > > > result->operation |= RPi::IPA_CONFIG_SENSOR; > > > } > > > > > > + firstStart_ = false; > > > + > > > return 0; > > > } > > >
Hi again! On Sat, 5 Dec 2020 at 23:03, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi David, > > On Sat, Dec 05, 2020 at 10:39:55PM +0000, David Plowman wrote: > > On Sat, 5 Dec 2020 at 19:57, Laurent Pinchart wrote: > > > On Wed, Dec 02, 2020 at 11:52:49AM +0000, David Plowman wrote: > > > > This makes it possible to tell whether we're starting the sensor for > > > > the first time, or whether it's happening because of a mode switch or > > > > because the camera has been paused and re-started. Depending on this, > > > > some sensors may require us to drop different numbers of frames. > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > > > This looks good to me, but I'd squash it with patch 2/5 as the chance is > > > very small and it's easier to review it when also seeing how the new > > > variable is used. > > > > Will do! > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > On a side note, how does mode change and initial start differ ? Don't we > > > stop the sensor when reconfiguring the camera ? > > > > The difference is that on first camera start, the algorithms are > > starting "from scratch", with no idea what initial values to use. On > > subsequent mode switches, the algorithms are known to be in a sensible > > place and we just leave them be (we don't reset them, though the > > change of camera mode can some some other effects). > > Ah yes that makes sense. What if the application stops the camera and > restarts it "a long time" later ? Would we need some kind of timeout > after which algorithms should drop their state (or at least consider the > state doesn't shorten the convergence time anymore) ? Yes, I guess that's a possibility. Though I think I'd be reluctant to make the algorithms handle that themselves, probably an application should know better whether this might be the situation - at which point there would have to be some way to "reset" the camera system, which would in turn "reset" all the IPAs. So maybe that's one to think about a bit. In the meantime I suppose closing and re-opening the camera system is not a terrible workaround... Thanks! David > > > > > --- > > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 ++++++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > > index 69be5e4e..b8298768 100644 > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > > @@ -67,7 +67,7 @@ public: > > > > IPARPi() > > > > : lastMode_({}), controller_(), controllerInit_(false), > > > > frameCount_(0), checkCount_(0), mistrustCount_(0), > > > > - lsTable_(nullptr) > > > > + lsTable_(nullptr), firstStart_(true) > > > > { > > > > } > > > > > > > > @@ -145,6 +145,9 @@ private: > > > > /* LS table allocation passed in from the pipeline handler. */ > > > > FileDescriptor lsTableHandle_; > > > > void *lsTable_; > > > > + > > > > + /* Distinguish the first camera start from others. */ > > > > + bool firstStart_; > > > > }; > > > > > > > > int IPARPi::init(const IPASettings &settings) > > > > @@ -179,6 +182,8 @@ int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result) > > > > result->operation |= RPi::IPA_CONFIG_SENSOR; > > > > } > > > > > > > > + firstStart_ = false; > > > > + > > > > return 0; > > > > } > > > > > > -- > Regards, > > Laurent Pinchart
Hi David, On Sat, Dec 05, 2020 at 11:10:48PM +0000, David Plowman wrote: > On Sat, 5 Dec 2020 at 23:03, Laurent Pinchart wrote: > > On Sat, Dec 05, 2020 at 10:39:55PM +0000, David Plowman wrote: > > > On Sat, 5 Dec 2020 at 19:57, Laurent Pinchart wrote: > > > > On Wed, Dec 02, 2020 at 11:52:49AM +0000, David Plowman wrote: > > > > > This makes it possible to tell whether we're starting the sensor for > > > > > the first time, or whether it's happening because of a mode switch or > > > > > because the camera has been paused and re-started. Depending on this, > > > > > some sensors may require us to drop different numbers of frames. > > > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > > > > > This looks good to me, but I'd squash it with patch 2/5 as the chance is > > > > very small and it's easier to review it when also seeing how the new > > > > variable is used. > > > > > > Will do! > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > On a side note, how does mode change and initial start differ ? Don't we > > > > stop the sensor when reconfiguring the camera ? > > > > > > The difference is that on first camera start, the algorithms are > > > starting "from scratch", with no idea what initial values to use. On > > > subsequent mode switches, the algorithms are known to be in a sensible > > > place and we just leave them be (we don't reset them, though the > > > change of camera mode can some some other effects). > > > > Ah yes that makes sense. What if the application stops the camera and > > restarts it "a long time" later ? Would we need some kind of timeout > > after which algorithms should drop their state (or at least consider the > > state doesn't shorten the convergence time anymore) ? > > Yes, I guess that's a possibility. Though I think I'd be reluctant to > make the algorithms handle that themselves, probably an application > should know better whether this might be the situation - at which > point there would have to be some way to "reset" the camera system, > which would in turn "reset" all the IPAs. That's a good point. A timeout in the IPA would be fairly arbitrary, especially consider the use case of stop motion capture in a controlled light environment (although in that case one would probably want to set exposure and gains explicitly). > So maybe that's one to think about a bit. In the meantime I suppose > closing and re-opening the camera system is not a terrible > workaround... Having to stop and restart the camera manager is a bit harsh as it will affect all cameras. A different API is likely needed. Let's just keep it in mind. > > > > > --- > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 ++++++- > > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > > > index 69be5e4e..b8298768 100644 > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > > > @@ -67,7 +67,7 @@ public: > > > > > IPARPi() > > > > > : lastMode_({}), controller_(), controllerInit_(false), > > > > > frameCount_(0), checkCount_(0), mistrustCount_(0), > > > > > - lsTable_(nullptr) > > > > > + lsTable_(nullptr), firstStart_(true) > > > > > { > > > > > } > > > > > > > > > > @@ -145,6 +145,9 @@ private: > > > > > /* LS table allocation passed in from the pipeline handler. */ > > > > > FileDescriptor lsTableHandle_; > > > > > void *lsTable_; > > > > > + > > > > > + /* Distinguish the first camera start from others. */ > > > > > + bool firstStart_; > > > > > }; > > > > > > > > > > int IPARPi::init(const IPASettings &settings) > > > > > @@ -179,6 +182,8 @@ int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result) > > > > > result->operation |= RPi::IPA_CONFIG_SENSOR; > > > > > } > > > > > > > > > > + firstStart_ = false; > > > > > + > > > > > return 0; > > > > > } > > > > >
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 69be5e4e..b8298768 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -67,7 +67,7 @@ public: IPARPi() : lastMode_({}), controller_(), controllerInit_(false), frameCount_(0), checkCount_(0), mistrustCount_(0), - lsTable_(nullptr) + lsTable_(nullptr), firstStart_(true) { } @@ -145,6 +145,9 @@ private: /* LS table allocation passed in from the pipeline handler. */ FileDescriptor lsTableHandle_; void *lsTable_; + + /* Distinguish the first camera start from others. */ + bool firstStart_; }; int IPARPi::init(const IPASettings &settings) @@ -179,6 +182,8 @@ int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result) result->operation |= RPi::IPA_CONFIG_SENSOR; } + firstStart_ = false; + return 0; }
This makes it possible to tell whether we're starting the sensor for the first time, or whether it's happening because of a mode switch or because the camera has been paused and re-started. Depending on this, some sensors may require us to drop different numbers of frames. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/ipa/raspberrypi/raspberrypi.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)