<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 28 Apr 2021 at 13:16, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.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 David,<br>
<br>
On Tue, Apr 27, 2021 at 09:22:00AM +0100, David Plowman wrote:<br>
> On Tue, 27 Apr 2021 at 07:20, Laurent Pinchart wrote:<br>
> > On Sat, Apr 17, 2021 at 09:19:51AM +0100, David Plowman wrote:<br>
> > > On Fri, 16 Apr 2021 at 22:56, Laurent Pinchart wrote:<br>
> > > > On Wed, Apr 14, 2021 at 11:29:53AM +0100, David Plowman wrote:<br>
> > > > > This allows derived classes to override them if they have any special<br>
> > > > > behaviours to implement. For instance if a particular camera mode<br>
> > > > > produces a different signal level to other modes, you might choose to<br>
> > > > > address that in the gain or exposure methods.<br>
> > > ><br>
> > > > For my information, would you be able to give an example of how this<br>
> > > > would be used ? What kind of particular camera mode would produce a<br>
> > > > different signal level, is this related to binning, or something else ?<br>
> > ><br>
> > > Yes, so the sensor I'm looking at produces twice the signal level in<br>
> > > the binning modes that it does in the full resolution mode. I think<br>
> > > this difference would be awkward for applications which means we have<br>
> > > to hide it somewhere.<br>
> > ><br>
> > > One of my earlier emails talked a little bit about hiding this in the<br>
> > > AGC. You'd have to know a "base gain" for every mode (so 1 for full<br>
> > > res, 2 for binned), and then have some process where we have public<br>
> > > exposure/gain numbers (which are the same for all modes) and there's a<br>
> > > process of conversion into and out of the AGC and IPA which deal with<br>
> > > real exposure/gain values. But I'm really not sure I want to go there,<br>
> > > at least right now!<br>
> > ><br>
> > > I think this is a pragmatic alternative. You can already hide the<br>
> > > difference by changing the CamHelper's Gain and GainCode functions, to<br>
> > > apply the magic extra factor of 2 when necessary. This change here<br>
> > > makes it possible to apply that factor in the exposure instead - you<br>
> > > could imagine someone wanting the lowest noise possible and accepting<br>
> > > that the real exposure will be double (I wouldn't want to double<br>
> > > exposures without people explicitly making that choice). And finally,<br>
> > > this change doesn't make it any more difficult to do the compensation<br>
> > > in the AGC at a later date, should we wish.<br>
> > ><br>
> > > In the CamHelper for my new sensor, I even make it easy to mix the<br>
> > > factor of 2 across both gain and exposure, so you could, for example,<br>
> > > have sqrt(2) in each.<br>
> > ><br>
> > > That got rather long... I hope it was understandable!<br>
> ><br>
> > Thanks for the information. There may be something that I don't get<br>
> > though, as I don't see why the exposure needs to be affected. The<br>
> > exposure time has a big influence on motion blur, so it seems to me that<br>
> > we shouldn't cheat here. The gain seems to be a better place to hide<br>
> > this sensor feature. I wonder if we should consider moving from gain to<br>
> > sensor sensitivity.<br>
> <br>
> You're right, exposure doesn't *need* to be affected, and indeed my<br>
> default position is to handle the difference using gain instead. The<br>
> intention is that people are *able* to keep the gain to a minimum if<br>
> they want to, at the cost of changing the exposure - but this has to<br>
> be a deliberate choice.<br>
><br>
> I agree the "sensitivity" is a good idea, and indeed a much better<br>
> term than I used above, which was "base gain".<br>
> <br>
> Note that sensitivity is a per-mode thing here. I think it's a good<br>
> solution but has some consequences:<br>
> <br>
> - If an application wants to change camera mode and set the exposure<br>
> they're going to have to deal with per-mode sensitivities and adapt<br>
> their numbers accordingly. Do we like this? I'm always very nervous of<br>
> complicating an application's life!<br>
><br>
> - You could try and handle per-mode sensitivities either outside the<br>
> AGC (effectively that's what this allows) or you could handle it<br>
> within the AGC. Whenever the camera mode changes you'd have to adjust<br>
> the state variables according to the ratio of old_sensitivity /<br>
> new_sensitivity. You'd probably have to use a different exposure<br>
> profile, possibly it needs to be set by the application - or could we<br>
> do something more automatic? The implementation would also depend on<br>
> whether we want applications to have to pay attention to the "per-mode<br>
> sensitivities" or not.<br>
> <br>
> So overall, I'm in several minds about going this "per-mode<br>
> sensitivity" route and the work it requires on the AGC, as well as the<br>
> implications for applications. In the meantime, making the exposure<br>
> methods virtual allows these sensors to work and doesn't make a later<br>
> step towards per-mode sensitivities any more difficult. You would<br>
> simply do that work and then delete your virtual exposure methods.<br>
<br>
I'm sorry, but I still don't see why it requires making the below<br>
functions virtual. They convert between exposure expressed in lines and<br>
exposure expressed as a time, why would that need to be sensor-specific<br>
? I guess I need to see an actual use case first.<br></blockquote><div><br></div><div>This is entirely tangential to David's usage, but I also need this method to</div><div>be virtual so I can override this for imx477 ultra long exposure modes. In</div><div>this mode, we have some additional scaling to apply to the standard equation.</div><div><br></div><div>Regards,</div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> Possibly I've repeated myself rather a lot there - but maybe I've<br>
> explained it better this time? :)<br>
> <br>
> > > > > Signed-off-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > > > > Reviewed-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > ><br>
> > > > Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
> > > ><br>
> > > > > ---<br>
> > > > > src/ipa/raspberrypi/cam_helper.hpp | 4 ++--<br>
> > > > > 1 file changed, 2 insertions(+), 2 deletions(-)<br>
> > > > ><br>
> > > > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp<br>
> > > > > index 1b2d6eec..4053a870 100644<br>
> > > > > --- a/src/ipa/raspberrypi/cam_helper.hpp<br>
> > > > > +++ b/src/ipa/raspberrypi/cam_helper.hpp<br>
> > > > > @@ -66,8 +66,8 @@ public:<br>
> > > > > virtual ~CamHelper();<br>
> > > > > void SetCameraMode(const CameraMode &mode);<br>
> > > > > MdParser &Parser() const { return *parser_; }<br>
> > > > > - uint32_t ExposureLines(double exposure_us) const;<br>
> > > > > - double Exposure(uint32_t exposure_lines) const; // in us<br>
> > > > > + virtual uint32_t ExposureLines(double exposure_us) const;<br>
> > > > > + virtual double Exposure(uint32_t exposure_lines) const; // in us<br>
> > > > > virtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,<br>
> > > > > double maxFrameDuration) const;<br>
> > > > > virtual uint32_t GainCode(double gain) const = 0;<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
_______________________________________________<br>
libcamera-devel mailing list<br>
<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
<a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>