| Message ID | 20210928153634.5864-1-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | a733e0647a2c3cbbacac9110b01afa1e2a2d68d7 |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for the patch. On Tue, Sep 28, 2021 at 04:36:34PM +0100, David Plowman wrote: > This fixes bug https://bugs.libcamera.org/show_bug.cgi?id=74. This should be moved before the Signed-off-by line, with Bug: https://bugs.libcamera.org/show_bug.cgi?id=74. > The cause is that we read out delayed values using a frame's sequence > number (DelayedControls::get). But we fill the values up > (DelayedControls::applyControls) incrementing writeCount by only one > even if the sequence number has jumped by several since last > time. This is exactly what happens when frames are being dropped. > > So the fix is to increment writeCount by "as much as the sequence > number has jumped since last time", which means that we just follow > the sequence number directly. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/libcamera/delayed_controls.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp > index 90ce7e0b..9667187e 100644 > --- a/src/libcamera/delayed_controls.cpp > +++ b/src/libcamera/delayed_controls.cpp > @@ -279,7 +279,7 @@ void DelayedControls::applyControls(uint32_t sequence) > } > } > > - writeCount_++; > + writeCount_ = sequence - firstSequence_ + 1; I'm always confused when I read this file, I think one day I'll dive deep and do something about it :-) Until then, this looks good to me. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> A second pair of eyes would be nice (and testing on IPU3 too). > > while (writeCount_ > queueCount_) { > LOG(DelayedControls, Debug)
Hi David, Thank you for your patch. On Tue, 28 Sept 2021 at 16:36, David Plowman <david.plowman@raspberrypi.com> wrote: > This fixes bug https://bugs.libcamera.org/show_bug.cgi?id=74. > > The cause is that we read out delayed values using a frame's sequence > number (DelayedControls::get). But we fill the values up > (DelayedControls::applyControls) incrementing writeCount by only one > even if the sequence number has jumped by several since last > time. This is exactly what happens when frames are being dropped. > > So the fix is to increment writeCount by "as much as the sequence > number has jumped since last time", which means that we just follow > the sequence number directly. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> Tested-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/delayed_controls.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/delayed_controls.cpp > b/src/libcamera/delayed_controls.cpp > index 90ce7e0b..9667187e 100644 > --- a/src/libcamera/delayed_controls.cpp > +++ b/src/libcamera/delayed_controls.cpp > @@ -279,7 +279,7 @@ void DelayedControls::applyControls(uint32_t sequence) > } > } > > - writeCount_++; > + writeCount_ = sequence - firstSequence_ + 1; > > while (writeCount_ > queueCount_) { > LOG(DelayedControls, Debug) > -- > 2.20.1 > >
Hello On Wed, Sep 29, 2021 at 12:58:27AM +0300, Laurent Pinchart wrote: > Hi David, > > Thank you for the patch. > > On Tue, Sep 28, 2021 at 04:36:34PM +0100, David Plowman wrote: > > This fixes bug https://bugs.libcamera.org/show_bug.cgi?id=74. > > This should be moved before the Signed-off-by line, with > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=74. > > > The cause is that we read out delayed values using a frame's sequence > > number (DelayedControls::get). But we fill the values up > > (DelayedControls::applyControls) incrementing writeCount by only one > > even if the sequence number has jumped by several since last > > time. This is exactly what happens when frames are being dropped. > > > > So the fix is to increment writeCount by "as much as the sequence > > number has jumped since last time", which means that we just follow > > the sequence number directly. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/libcamera/delayed_controls.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp > > index 90ce7e0b..9667187e 100644 > > --- a/src/libcamera/delayed_controls.cpp > > +++ b/src/libcamera/delayed_controls.cpp > > @@ -279,7 +279,7 @@ void DelayedControls::applyControls(uint32_t sequence) > > } > > } > > > > - writeCount_++; > > + writeCount_ = sequence - firstSequence_ + 1; > > I'm always confused when I read this file, I think one day I'll dive > deep and do something about it :-) Until then, this looks good to me. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > A second pair of eyes would be nice (and testing on IPU3 too). I've run a few CTS runs and it doesn't seem to introduce regressions. However I'm wondering how this chaneg plays with queueCount > > > > > while (writeCount_ > queueCount_) { ^ this What I wonder about is: if a frame is dropped, should we advance the count and 'burn' the controls that were meant for it ? Because in this case it seems to me that if the number of dropped frames exceeds the number of controls available in the queue, we end up pushing {} but we advance queueCount by one only. If it's intended to burn all the controls meant for dropped frames we should probably increment queueCount by the same amount writeCount_ has been incremented with. > > LOG(DelayedControls, Debug) > > -- > Regards, > > Laurent Pinchart
Hi Jacopo Thanks for the question! On Wed, 29 Sept 2021 at 10:51, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hello > > On Wed, Sep 29, 2021 at 12:58:27AM +0300, Laurent Pinchart wrote: > > Hi David, > > > > Thank you for the patch. > > > > On Tue, Sep 28, 2021 at 04:36:34PM +0100, David Plowman wrote: > > > This fixes bug https://bugs.libcamera.org/show_bug.cgi?id=74. > > > > This should be moved before the Signed-off-by line, with > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=74. > > > > > The cause is that we read out delayed values using a frame's sequence > > > number (DelayedControls::get). But we fill the values up > > > (DelayedControls::applyControls) incrementing writeCount by only one > > > even if the sequence number has jumped by several since last > > > time. This is exactly what happens when frames are being dropped. > > > > > > So the fix is to increment writeCount by "as much as the sequence > > > number has jumped since last time", which means that we just follow > > > the sequence number directly. > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > src/libcamera/delayed_controls.cpp | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp > > > index 90ce7e0b..9667187e 100644 > > > --- a/src/libcamera/delayed_controls.cpp > > > +++ b/src/libcamera/delayed_controls.cpp > > > @@ -279,7 +279,7 @@ void DelayedControls::applyControls(uint32_t sequence) > > > } > > > } > > > > > > - writeCount_++; > > > + writeCount_ = sequence - firstSequence_ + 1; > > > > I'm always confused when I read this file, I think one day I'll dive > > deep and do something about it :-) Until then, this looks good to me. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > A second pair of eyes would be nice (and testing on IPU3 too). > > I've run a few CTS runs and it doesn't seem to introduce regressions. > > However I'm wondering how this chaneg plays with queueCount > > > > > > > > > while (writeCount_ > queueCount_) { > ^ > this > > What I wonder about is: if a frame is dropped, should we advance the > count and 'burn' the controls that were meant for it ? Because in this > case it seems to me that if the number of dropped frames exceeds the > number of controls available in the queue, we end up pushing {} but we > advance queueCount by one only. That loop will run, incrementing queueCount (in "push") by 1 each time, until it has caught up with writeCount - which I believe to be the correct behaviour. Each call will copy the previous control entries to the next one, so anyone who asks for values for a frame we dropped will simply get the most recent values that we did see. Even if the number of dropped frames exceeds the length of the queue the behaviour seems fine - the entire queue will fill up with the last seen values. > > If it's intended to burn all the controls meant for dropped frames we > should probably increment queueCount by the same amount writeCount_ > has been incremented with. Not sure what you mean by "burn" the controls? The client code can ask for values from sequence numbers that were never actually seen (if it wants), and will get the last known values back. Does that make sense? Best regards David > > > > > LOG(DelayedControls, Debug) > > > > > -- > > Regards, > > > > Laurent Pinchart
Hi David On Wed, Sep 29, 2021 at 11:17:23AM +0100, David Plowman wrote: > Hi Jacopo > > Thanks for the question! > > On Wed, 29 Sept 2021 at 10:51, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hello > > > > On Wed, Sep 29, 2021 at 12:58:27AM +0300, Laurent Pinchart wrote: > > > Hi David, > > > > > > Thank you for the patch. > > > > > > On Tue, Sep 28, 2021 at 04:36:34PM +0100, David Plowman wrote: > > > > This fixes bug https://bugs.libcamera.org/show_bug.cgi?id=74. > > > > > > This should be moved before the Signed-off-by line, with > > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=74. > > > > > > > The cause is that we read out delayed values using a frame's sequence > > > > number (DelayedControls::get). But we fill the values up > > > > (DelayedControls::applyControls) incrementing writeCount by only one > > > > even if the sequence number has jumped by several since last > > > > time. This is exactly what happens when frames are being dropped. > > > > > > > > So the fix is to increment writeCount by "as much as the sequence > > > > number has jumped since last time", which means that we just follow > > > > the sequence number directly. > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > src/libcamera/delayed_controls.cpp | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp > > > > index 90ce7e0b..9667187e 100644 > > > > --- a/src/libcamera/delayed_controls.cpp > > > > +++ b/src/libcamera/delayed_controls.cpp > > > > @@ -279,7 +279,7 @@ void DelayedControls::applyControls(uint32_t sequence) > > > > } > > > > } > > > > > > > > - writeCount_++; > > > > + writeCount_ = sequence - firstSequence_ + 1; > > > > > > I'm always confused when I read this file, I think one day I'll dive > > > deep and do something about it :-) Until then, this looks good to me. > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > A second pair of eyes would be nice (and testing on IPU3 too). > > > > I've run a few CTS runs and it doesn't seem to introduce regressions. > > > > However I'm wondering how this chaneg plays with queueCount > > > > > > > > > > > > > while (writeCount_ > queueCount_) { > > ^ > > this > > > > What I wonder about is: if a frame is dropped, should we advance the > > count and 'burn' the controls that were meant for it ? Because in this > > case it seems to me that if the number of dropped frames exceeds the > > number of controls available in the queue, we end up pushing {} but we > > advance queueCount by one only. > > That loop will run, incrementing queueCount (in "push") by 1 each > time, until it has caught up with writeCount - which I believe to be > the correct behaviour. Each call will copy the previous control > entries to the next one, so anyone who asks for values for a frame we > dropped will simply get the most recent values that we did see. > > Even if the number of dropped frames exceeds the length of the queue > the behaviour seems fine - the entire queue will fill up with the last > seen values. > > > > > If it's intended to burn all the controls meant for dropped frames we > > should probably increment queueCount by the same amount writeCount_ > > has been incremented with. > > Not sure what you mean by "burn" the controls? The client code can ask > for values from sequence numbers that were never actually seen (if it > wants), and will get the last known values back. Does that make sense? Let's assume that we have 3 controls in the values_ queue waiting to be applied to the 'next' frames writeCount = x queueCount = x + 3 values_ = [ C(x+1), C(x+2), C(x+3) ] We drop 4 frames hence writeCount = x + 4 and we have to advance queueCount by pushing {} and copying the last values values_ = [ C(x+1), C(x+2), C(x+3), C(x+3) ] What I meant by burnt is that if I'm not mistaken C(x+1) and C(x+2) are lost and I wonder if we shouldn't instead values_ = [ C(x+1), C(x+2), C(x+3), C(x+1), C(x+2), C(x+3) ] when frames are dropped. Or have I got it all wrong ? > > Best regards > David > > > > > > > > > LOG(DelayedControls, Debug) > > > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart
Hi Jacopo Thanks for clarifying! On Wed, 29 Sept 2021 at 11:30, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi David > > On Wed, Sep 29, 2021 at 11:17:23AM +0100, David Plowman wrote: > > Hi Jacopo > > > > Thanks for the question! > > > > On Wed, 29 Sept 2021 at 10:51, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > Hello > > > > > > On Wed, Sep 29, 2021 at 12:58:27AM +0300, Laurent Pinchart wrote: > > > > Hi David, > > > > > > > > Thank you for the patch. > > > > > > > > On Tue, Sep 28, 2021 at 04:36:34PM +0100, David Plowman wrote: > > > > > This fixes bug https://bugs.libcamera.org/show_bug.cgi?id=74. > > > > > > > > This should be moved before the Signed-off-by line, with > > > > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=74. > > > > > > > > > The cause is that we read out delayed values using a frame's sequence > > > > > number (DelayedControls::get). But we fill the values up > > > > > (DelayedControls::applyControls) incrementing writeCount by only one > > > > > even if the sequence number has jumped by several since last > > > > > time. This is exactly what happens when frames are being dropped. > > > > > > > > > > So the fix is to increment writeCount by "as much as the sequence > > > > > number has jumped since last time", which means that we just follow > > > > > the sequence number directly. > > > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > > --- > > > > > src/libcamera/delayed_controls.cpp | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp > > > > > index 90ce7e0b..9667187e 100644 > > > > > --- a/src/libcamera/delayed_controls.cpp > > > > > +++ b/src/libcamera/delayed_controls.cpp > > > > > @@ -279,7 +279,7 @@ void DelayedControls::applyControls(uint32_t sequence) > > > > > } > > > > > } > > > > > > > > > > - writeCount_++; > > > > > + writeCount_ = sequence - firstSequence_ + 1; > > > > > > > > I'm always confused when I read this file, I think one day I'll dive > > > > deep and do something about it :-) Until then, this looks good to me. > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > A second pair of eyes would be nice (and testing on IPU3 too). > > > > > > I've run a few CTS runs and it doesn't seem to introduce regressions. > > > > > > However I'm wondering how this chaneg plays with queueCount > > > > > > > > > > > > > > > > > while (writeCount_ > queueCount_) { > > > ^ > > > this > > > > > > What I wonder about is: if a frame is dropped, should we advance the > > > count and 'burn' the controls that were meant for it ? Because in this > > > case it seems to me that if the number of dropped frames exceeds the > > > number of controls available in the queue, we end up pushing {} but we > > > advance queueCount by one only. > > > > That loop will run, incrementing queueCount (in "push") by 1 each > > time, until it has caught up with writeCount - which I believe to be > > the correct behaviour. Each call will copy the previous control > > entries to the next one, so anyone who asks for values for a frame we > > dropped will simply get the most recent values that we did see. > > > > Even if the number of dropped frames exceeds the length of the queue > > the behaviour seems fine - the entire queue will fill up with the last > > seen values. > > > > > > > > If it's intended to burn all the controls meant for dropped frames we > > > should probably increment queueCount by the same amount writeCount_ > > > has been incremented with. > > > > Not sure what you mean by "burn" the controls? The client code can ask > > for values from sequence numbers that were never actually seen (if it > > wants), and will get the last known values back. Does that make sense? > > Let's assume that we have 3 controls in the values_ queue waiting to > be applied to the 'next' frames > > writeCount = x > queueCount = x + 3 > values_ = [ C(x+1), C(x+2), C(x+3) ] > > We drop 4 frames hence > > writeCount = x + 4 > > and we have to advance queueCount by pushing {} and copying the last > values > > values_ = [ C(x+1), C(x+2), C(x+3), C(x+3) ] Yes, this is what we get. > > What I meant by burnt is that if I'm not mistaken C(x+1) and C(x+2) > are lost and I wonder if we shouldn't instead I wouldn't describe them as "lost". The client code can still access them with get(x+1), get(x+3) etc. > > values_ = [ C(x+1), C(x+2), C(x+3), C(x+1), C(x+2), C(x+3) ] I think the trouble with this is that DelayedControls are more of an array than a queue. Don't think of it in terms of accessing "the most recent values" or "the values from n frames ago". We access them with an array index, which happens to be the actual frame sequence number (more or less). (The fact that only the 16 most recent items are stored is kind of an "implementation detail".) So with those values, the code would, for frame x+4, call get(x+4) and come back with C(x+1) - not the right values. > > when frames are dropped. > > Or have I got it all wrong ? Have I explained it any better? :) David > > > > > > Best regards > > David > > > > > > > > > > > > > LOG(DelayedControls, Debug) > > > > > > > > > > > -- > > > > Regards, > > > > > > > > Laurent Pinchart
Hi David, On Wed, Sep 29, 2021 at 11:51:24AM +0100, David Plowman wrote: > Hi Jacopo > > Thanks for clarifying! > > On Wed, 29 Sept 2021 at 11:30, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi David > > > > On Wed, Sep 29, 2021 at 11:17:23AM +0100, David Plowman wrote: > > > Hi Jacopo > > > > > > Thanks for the question! > > > > > > On Wed, 29 Sept 2021 at 10:51, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > Hello > > > > > > > > On Wed, Sep 29, 2021 at 12:58:27AM +0300, Laurent Pinchart wrote: > > > > > Hi David, > > > > > > > > > > Thank you for the patch. > > > > > > > > > > On Tue, Sep 28, 2021 at 04:36:34PM +0100, David Plowman wrote: > > > > > > This fixes bug https://bugs.libcamera.org/show_bug.cgi?id=74. > > > > > > > > > > This should be moved before the Signed-off-by line, with > > > > > > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=74. > > > > > > > > > > > The cause is that we read out delayed values using a frame's sequence > > > > > > number (DelayedControls::get). But we fill the values up > > > > > > (DelayedControls::applyControls) incrementing writeCount by only one > > > > > > even if the sequence number has jumped by several since last > > > > > > time. This is exactly what happens when frames are being dropped. > > > > > > > > > > > > So the fix is to increment writeCount by "as much as the sequence > > > > > > number has jumped since last time", which means that we just follow > > > > > > the sequence number directly. > > > > > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > > > --- > > > > > > src/libcamera/delayed_controls.cpp | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp > > > > > > index 90ce7e0b..9667187e 100644 > > > > > > --- a/src/libcamera/delayed_controls.cpp > > > > > > +++ b/src/libcamera/delayed_controls.cpp > > > > > > @@ -279,7 +279,7 @@ void DelayedControls::applyControls(uint32_t sequence) > > > > > > } > > > > > > } > > > > > > > > > > > > - writeCount_++; > > > > > > + writeCount_ = sequence - firstSequence_ + 1; > > > > > > > > > > I'm always confused when I read this file, I think one day I'll dive > > > > > deep and do something about it :-) Until then, this looks good to me. > > > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > > A second pair of eyes would be nice (and testing on IPU3 too). > > > > > > > > I've run a few CTS runs and it doesn't seem to introduce regressions. > > > > > > > > However I'm wondering how this chaneg plays with queueCount > > > > > > > > > > > > > > > > > > > > > while (writeCount_ > queueCount_) { > > > > ^ > > > > this > > > > > > > > What I wonder about is: if a frame is dropped, should we advance the > > > > count and 'burn' the controls that were meant for it ? Because in this > > > > case it seems to me that if the number of dropped frames exceeds the > > > > number of controls available in the queue, we end up pushing {} but we > > > > advance queueCount by one only. > > > > > > That loop will run, incrementing queueCount (in "push") by 1 each > > > time, until it has caught up with writeCount - which I believe to be > > > the correct behaviour. Each call will copy the previous control > > > entries to the next one, so anyone who asks for values for a frame we > > > dropped will simply get the most recent values that we did see. > > > > > > Even if the number of dropped frames exceeds the length of the queue > > > the behaviour seems fine - the entire queue will fill up with the last > > > seen values. > > > > > > > > > > > If it's intended to burn all the controls meant for dropped frames we > > > > should probably increment queueCount by the same amount writeCount_ > > > > has been incremented with. > > > > > > Not sure what you mean by "burn" the controls? The client code can ask > > > for values from sequence numbers that were never actually seen (if it > > > wants), and will get the last known values back. Does that make sense? > > > > Let's assume that we have 3 controls in the values_ queue waiting to > > be applied to the 'next' frames > > > > writeCount = x > > queueCount = x + 3 > > values_ = [ C(x+1), C(x+2), C(x+3) ] > > > > We drop 4 frames hence > > > > writeCount = x + 4 > > > > and we have to advance queueCount by pushing {} and copying the last > > values > > > > values_ = [ C(x+1), C(x+2), C(x+3), C(x+3) ] > > Yes, this is what we get. > > > > > What I meant by burnt is that if I'm not mistaken C(x+1) and C(x+2) > > are lost and I wonder if we shouldn't instead > > I wouldn't describe them as "lost". The client code can still access > them with get(x+1), get(x+3) etc. > > > > > values_ = [ C(x+1), C(x+2), C(x+3), C(x+1), C(x+2), C(x+3) ] > > I think the trouble with this is that DelayedControls are more of an > array than a queue. Don't think of it in terms of accessing "the most > recent values" or "the values from n frames ago". We access them with > an array index, which happens to be the actual frame sequence number > (more or less). (The fact that only the 16 most recent items are > stored is kind of an "implementation detail".) > > So with those values, the code would, for frame x+4, call get(x+4) and > come back with C(x+1) - not the right values. > My understanding was that ideally if we drop a frame, the setting that were applied to it should be re-applied to the next one, and the whole 'queue' is shifted by one place. That's not the case as I understand it. Thanks for the explanation then! Tested-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > > > when frames are dropped. > > > > Or have I got it all wrong ? > > Have I explained it any better? :) > > David > > > > > > > > > > > Best regards > > > David > > > > > > > > > > > > > > > > > LOG(DelayedControls, Debug) > > > > > > > > > > > > > > -- > > > > > Regards, > > > > > > > > > > Laurent Pinchart
diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp index 90ce7e0b..9667187e 100644 --- a/src/libcamera/delayed_controls.cpp +++ b/src/libcamera/delayed_controls.cpp @@ -279,7 +279,7 @@ void DelayedControls::applyControls(uint32_t sequence) } } - writeCount_++; + writeCount_ = sequence - firstSequence_ + 1; while (writeCount_ > queueCount_) { LOG(DelayedControls, Debug)
This fixes bug https://bugs.libcamera.org/show_bug.cgi?id=74. The cause is that we read out delayed values using a frame's sequence number (DelayedControls::get). But we fill the values up (DelayedControls::applyControls) incrementing writeCount by only one even if the sequence number has jumped by several since last time. This is exactly what happens when frames are being dropped. So the fix is to increment writeCount by "as much as the sequence number has jumped since last time", which means that we just follow the sequence number directly. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/libcamera/delayed_controls.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)