<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thank you for the review!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 23 Nov 2022 at 13:52, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush<br>
<br>
Thanks for the patch.<br>
<br>
On Tue, 22 Nov 2022 at 11:22, Naushir Patuck via libcamera-devel<br>
<<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a>> wrote:<br>
><br>
> Release the statistics buffer after running the through the AWB calculations.<br>
> Only the "counted" statistics are copied out to a local structure, so keeping<br>
> the statistics buffer allows the algorithm to see the "uncounted" statistics as<br>
> well.<br>
<br>
I guess I still feel a bit weird about this. The code used to copy out<br>
everything it needed so that it could release the buffer as soon as<br>
possible. Maybe that's not a meaningful concern any more...?<br></blockquote><div><br></div><div>In the original code, we used to make a copy of the stats dmabuf, and this</div><div>copy (through a shared_ptr) was what the controller algorithms would get.</div><div>We now copy values out of the stats dmabuf into the new stats structure,</div><div>and pass this in as a shared_ptr.</div><div><br></div><div>As such, moving the reset() to release the buffer to later does not have any</div><div>impact on recycling the stats dmabuf back to the ISP.  The alternative to this</div><div>change would be to cache the stats grid sizes (that's what we really need)</div><div>instead of keeping the shared_ptr referenced for a bit longer?</div><div><br></div><div>Naush</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
> This is currently handled by hard-coding the total number of statistics regions<br>
> regions based on the structure definition in the bcm2835_isp_stats structure.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
<br>
In spite of my probably unnecessary weird feelings, it's probably of<br>
no consequence, so:<br>
<br>
Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
<br>
Thanks!<br>
David<br>
<br>
> ---<br>
>  src/ipa/raspberrypi/controller/rpi/awb.cpp | 10 +++++-----<br>
>  1 file changed, 5 insertions(+), 5 deletions(-)<br>
><br>
> diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp<br>
> index 8d8ddf0913f7..861022014896 100644<br>
> --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp<br>
> +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp<br>
> @@ -432,11 +432,6 @@ void Awb::prepareStats()<br>
>          */<br>
>         generateStats(zones_, statistics_->awb_stats, config_.minPixels,<br>
>                       config_.minG);<br>
> -       /*<br>
> -        * we're done with these; we may as well relinquish our hold on the<br>
> -        * pointer.<br>
> -        */<br>
> -       statistics_.reset();<br>
>         /*<br>
>          * apply sensitivities, so values appear to come from our "canonical"<br>
>          * sensor.<br>
> @@ -733,6 +728,11 @@ void Awb::doAwb()<br>
>                         << " with gains r " << asyncResults_.gainR<br>
>                         << " and b " << asyncResults_.gainB;<br>
>         }<br>
> +       /*<br>
> +        * we're done with these; we may as well relinquish our hold on the<br>
> +        * pointer.<br>
> +        */<br>
> +       statistics_.reset();<br>
>  }<br>
><br>
>  /* Register algorithm with the system. */<br>
> --<br>
> 2.25.1<br>
><br>
</blockquote></div></div>