| Message ID | 20241017124613.3853273-1-mike.rudenko@gmail.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
The chart from camshark (see attachment). -- Best regards, Mikhail Rudenko
Hi Mikhail, Quoting Mikhail Rudenko (2024-10-17 13:47:04) > The chart from camshark (see attachment). > That looks like a good result indeed! I'll test out the patches. Thanks! -- Kieran > -- > Best regards, > Mikhail Rudenko >
Hi Mikhail, Quoting Kieran Bingham (2024-10-17 14:35:40) > Hi Mikhail, > > Quoting Mikhail Rudenko (2024-10-17 13:47:04) > > The chart from camshark (see attachment). > > > > That looks like a good result indeed! > > I'll test out the patches. I've just tried this out - and it seems to work well on the IMX283 I have - but caused quite noticible AnalogueGain oscillations on the IMX335 ... I would 'bet' that's not the fault of this series - but the fact that we don't have per-sensor delayed-controls set up correctly which I already know about - but I haven't seen that get fixed up yet. I think your series probably highlights the fault that already exists... -- Kieran > > Thanks! > -- > Kieran > > > > -- > > Best regards, > > Mikhail Rudenko > >
Hi Kieran, On 2024-10-18 at 23:26 +01, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > Hi Mikhail, > > Quoting Kieran Bingham (2024-10-17 14:35:40) >> Hi Mikhail, >> >> Quoting Mikhail Rudenko (2024-10-17 13:47:04) >> > The chart from camshark (see attachment). >> > >> >> That looks like a good result indeed! >> >> I'll test out the patches. > > I've just tried this out - and it seems to work well on the IMX283 I > have - but caused quite noticible AnalogueGain oscillations on the > IMX335 ... I would 'bet' that's not the fault of this series - but the > fact that we don't have per-sensor delayed-controls set up correctly > which I already know about - but I haven't seen that get fixed up yet. I did a few more tests myself (I use OV4689 btw), and also observed analogue gain oscillations on darker scenes when AGC tries to steer the gain above 1.0. It looks like the hardcoded gain delay value of 1 is wrong at least for my sensor. When I increased it to 2, gain oscillations were gone. I have also tweaked a few things here and there and will send v2 soon. > I think your series probably highlights the fault that already exists... I feel like we have reached the moment when we should switch from the delays hardcoded in the pipeline handler to sensor specific values like rpi does. I could try doing that, but in a separate series. What do you think? -- Best regards, Mikhail
Quoting Mikhail Rudenko (2024-10-20 14:39:45) > Hi Kieran, > > On 2024-10-18 at 23:26 +01, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > > Hi Mikhail, > > > > Quoting Kieran Bingham (2024-10-17 14:35:40) > >> Hi Mikhail, > >> > >> Quoting Mikhail Rudenko (2024-10-17 13:47:04) > >> > The chart from camshark (see attachment). > >> > > >> > >> That looks like a good result indeed! > >> > >> I'll test out the patches. > > > > I've just tried this out - and it seems to work well on the IMX283 I > > have - but caused quite noticible AnalogueGain oscillations on the > > IMX335 ... I would 'bet' that's not the fault of this series - but the > > fact that we don't have per-sensor delayed-controls set up correctly > > which I already know about - but I haven't seen that get fixed up yet. > > I did a few more tests myself (I use OV4689 btw), and also observed > analogue gain oscillations on darker scenes when AGC tries to steer the > gain above 1.0. It looks like the hardcoded gain delay value of 1 is > wrong at least for my sensor. When I increased it to 2, gain > oscillations were gone. I have also tweaked a few things here and there > and will send v2 soon. > > > I think your series probably highlights the fault that already exists... > > I feel like we have reached the moment when we should switch from the > delays hardcoded in the pipeline handler to sensor specific values like > rpi does. I could try doing that, but in a separate series. What do you > think? Oh - absolutely. If it's something you're willing to work on - I think that would be a massive help. Indeed, I think that's a separate series. -- Kieran > > -- > Best regards, > Mikhail
On 2024-10-20 at 18:33 +01, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > Quoting Mikhail Rudenko (2024-10-20 14:39:45) >> Hi Kieran, >> >> On 2024-10-18 at 23:26 +01, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: >> >> > Hi Mikhail, >> > >> > Quoting Kieran Bingham (2024-10-17 14:35:40) >> >> Hi Mikhail, >> >> >> >> Quoting Mikhail Rudenko (2024-10-17 13:47:04) >> >> > The chart from camshark (see attachment). >> >> > >> >> >> >> That looks like a good result indeed! >> >> >> >> I'll test out the patches. >> > >> > I've just tried this out - and it seems to work well on the IMX283 I >> > have - but caused quite noticible AnalogueGain oscillations on the >> > IMX335 ... I would 'bet' that's not the fault of this series - but the >> > fact that we don't have per-sensor delayed-controls set up correctly >> > which I already know about - but I haven't seen that get fixed up yet. >> >> I did a few more tests myself (I use OV4689 btw), and also observed >> analogue gain oscillations on darker scenes when AGC tries to steer the >> gain above 1.0. It looks like the hardcoded gain delay value of 1 is >> wrong at least for my sensor. When I increased it to 2, gain >> oscillations were gone. I have also tweaked a few things here and there >> and will send v2 soon. >> >> > I think your series probably highlights the fault that already exists... >> >> I feel like we have reached the moment when we should switch from the >> delays hardcoded in the pipeline handler to sensor specific values like >> rpi does. I could try doing that, but in a separate series. What do you >> think? > > Oh - absolutely. If it's something you're willing to work on - I think > that would be a massive help. Indeed, I think that's a separate series. I'd like to give it a try. Am I right that adding virtual void getDelays(int &exposureDelay, int &gainDelay, int &vblankDelay, int &hblankDelay) to CameraSensorHelper returning generic delays, then overriding it for sensors where we know the exact delays is a valid approach? Also, why do we have both CamHelper and CameraSensorHelper, with former being significantly more featureful, but all the IPAs except rpi using the latter? -- Best regards, Mikhail
Quoting Mikhail Rudenko (2024-10-21 16:08:56) > > On 2024-10-20 at 18:33 +01, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > > Quoting Mikhail Rudenko (2024-10-20 14:39:45) > >> Hi Kieran, > >> > >> On 2024-10-18 at 23:26 +01, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > >> > >> > Hi Mikhail, > >> > > >> > Quoting Kieran Bingham (2024-10-17 14:35:40) > >> >> Hi Mikhail, > >> >> > >> >> Quoting Mikhail Rudenko (2024-10-17 13:47:04) > >> >> > The chart from camshark (see attachment). > >> >> > > >> >> > >> >> That looks like a good result indeed! > >> >> > >> >> I'll test out the patches. > >> > > >> > I've just tried this out - and it seems to work well on the IMX283 I > >> > have - but caused quite noticible AnalogueGain oscillations on the > >> > IMX335 ... I would 'bet' that's not the fault of this series - but the > >> > fact that we don't have per-sensor delayed-controls set up correctly > >> > which I already know about - but I haven't seen that get fixed up yet. > >> > >> I did a few more tests myself (I use OV4689 btw), and also observed > >> analogue gain oscillations on darker scenes when AGC tries to steer the > >> gain above 1.0. It looks like the hardcoded gain delay value of 1 is > >> wrong at least for my sensor. When I increased it to 2, gain > >> oscillations were gone. I have also tweaked a few things here and there > >> and will send v2 soon. > >> > >> > I think your series probably highlights the fault that already exists... > >> > >> I feel like we have reached the moment when we should switch from the > >> delays hardcoded in the pipeline handler to sensor specific values like > >> rpi does. I could try doing that, but in a separate series. What do you > >> think? > > > > Oh - absolutely. If it's something you're willing to work on - I think > > that would be a massive help. Indeed, I think that's a separate series. > > I'd like to give it a try. Am I right that adding > > virtual void getDelays(int &exposureDelay, int &gainDelay, int &vblankDelay, int &hblankDelay) Yes, quite likely. I don't know what the plumbing will look like yet, and maybe it would help to use something more structured than a series of int references, but that's what's needed somewhere... > to CameraSensorHelper returning generic delays, then overriding it for > sensors where we know the exact delays is a valid approach? Also, why do > we have both CamHelper and CameraSensorHelper, with former being > significantly more featureful, but all the IPAs except rpi using the > latter? Because Raspberry Pi create their own IPA - which doesn't use the CameraSensorHelpers - because they are not as featureful ... and all the other IPAs can not use the Raspberry Pi helpers - because they are in the Raspberry Pi IPA instead of common code ... and no one has yet tried to align the libipa code until now. Ultimately - Making sure that the helpers in libipa can be grown in a way that Raspberry Pi can also use them will then mean that we only need to have *one* location for all the common camera sensor specific helpers. -- Kieran > > -- > Best regards, > Mikhail
On Mon, Oct 21, 2024 at 04:40:30PM +0100, Kieran Bingham wrote: > Quoting Mikhail Rudenko (2024-10-21 16:08:56) > > On 2024-10-20 at 18:33 +01, Kieran Bingham wrote: > > > Quoting Mikhail Rudenko (2024-10-20 14:39:45) > > >> On 2024-10-18 at 23:26 +01, Kieran Bingham wrote: > > >> > Quoting Kieran Bingham (2024-10-17 14:35:40) > > >> >> Quoting Mikhail Rudenko (2024-10-17 13:47:04) > > >> >> > The chart from camshark (see attachment). > > >> >> > > > >> >> > > >> >> That looks like a good result indeed! > > >> >> > > >> >> I'll test out the patches. > > >> > > > >> > I've just tried this out - and it seems to work well on the IMX283 I > > >> > have - but caused quite noticible AnalogueGain oscillations on the > > >> > IMX335 ... I would 'bet' that's not the fault of this series - but the > > >> > fact that we don't have per-sensor delayed-controls set up correctly > > >> > which I already know about - but I haven't seen that get fixed up yet. > > >> > > >> I did a few more tests myself (I use OV4689 btw), and also observed > > >> analogue gain oscillations on darker scenes when AGC tries to steer the > > >> gain above 1.0. It looks like the hardcoded gain delay value of 1 is > > >> wrong at least for my sensor. When I increased it to 2, gain > > >> oscillations were gone. I have also tweaked a few things here and there > > >> and will send v2 soon. > > >> > > >> > I think your series probably highlights the fault that already exists... > > >> > > >> I feel like we have reached the moment when we should switch from the > > >> delays hardcoded in the pipeline handler to sensor specific values like > > >> rpi does. I could try doing that, but in a separate series. What do you > > >> think? > > > > > > Oh - absolutely. If it's something you're willing to work on - I think > > > that would be a massive help. Indeed, I think that's a separate series. > > > > I'd like to give it a try. Am I right that adding > > > > virtual void getDelays(int &exposureDelay, int &gainDelay, int &vblankDelay, int &hblankDelay) > > Yes, quite likely. I don't know what the plumbing will look like yet, > and maybe it would help to use something more structured than a series > of int references, but that's what's needed somewhere... A map of controls to delay values maybe. > > to CameraSensorHelper returning generic delays, then overriding it for > > sensors where we know the exact delays is a valid approach? Also, why do > > we have both CamHelper and CameraSensorHelper, with former being > > significantly more featureful, but all the IPAs except rpi using the > > latter? > > Because Raspberry Pi create their own IPA - which doesn't use the > CameraSensorHelpers - because they are not as featureful ... and all the > other IPAs can not use the Raspberry Pi helpers - because they are in > the Raspberry Pi IPA instead of common code ... and no one has yet tried > to align the libipa code until now. It's also because the RPi IPA predates CameraSensorHelper if I recall correctly. > Ultimately - Making sure that the helpers in libipa can be grown in a > way that Raspberry Pi can also use them will then mean that we only need > to have *one* location for all the common camera sensor specific > helpers.
On 2024-10-21 at 18:44 +03, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Mon, Oct 21, 2024 at 04:40:30PM +0100, Kieran Bingham wrote: >> Quoting Mikhail Rudenko (2024-10-21 16:08:56) >> > On 2024-10-20 at 18:33 +01, Kieran Bingham wrote: >> > > Quoting Mikhail Rudenko (2024-10-20 14:39:45) >> > >> On 2024-10-18 at 23:26 +01, Kieran Bingham wrote: >> > >> > Quoting Kieran Bingham (2024-10-17 14:35:40) >> > >> >> Quoting Mikhail Rudenko (2024-10-17 13:47:04) >> > >> >> > The chart from camshark (see attachment). >> > >> >> > >> > >> >> >> > >> >> That looks like a good result indeed! >> > >> >> >> > >> >> I'll test out the patches. >> > >> > >> > >> > I've just tried this out - and it seems to work well on the IMX283 I >> > >> > have - but caused quite noticible AnalogueGain oscillations on the >> > >> > IMX335 ... I would 'bet' that's not the fault of this series - but the >> > >> > fact that we don't have per-sensor delayed-controls set up correctly >> > >> > which I already know about - but I haven't seen that get fixed up yet. >> > >> >> > >> I did a few more tests myself (I use OV4689 btw), and also observed >> > >> analogue gain oscillations on darker scenes when AGC tries to steer the >> > >> gain above 1.0. It looks like the hardcoded gain delay value of 1 is >> > >> wrong at least for my sensor. When I increased it to 2, gain >> > >> oscillations were gone. I have also tweaked a few things here and there >> > >> and will send v2 soon. >> > >> >> > >> > I think your series probably highlights the fault that already exists... >> > >> >> > >> I feel like we have reached the moment when we should switch from the >> > >> delays hardcoded in the pipeline handler to sensor specific values like >> > >> rpi does. I could try doing that, but in a separate series. What do you >> > >> think? >> > > >> > > Oh - absolutely. If it's something you're willing to work on - I think >> > > that would be a massive help. Indeed, I think that's a separate series. >> > >> > I'd like to give it a try. Am I right that adding >> > >> > virtual void getDelays(int &exposureDelay, int &gainDelay, int &vblankDelay, int &hblankDelay) >> >> Yes, quite likely. I don't know what the plumbing will look like yet, >> and maybe it would help to use something more structured than a series >> of int references, but that's what's needed somewhere... > > A map of controls to delay values maybe. The "series of int references" is what CamHelper currently does, so I thought it could be a starting point. A map sounds more reasonable, yes. I'll try to get an RFC ready this week. >> > to CameraSensorHelper returning generic delays, then overriding it for >> > sensors where we know the exact delays is a valid approach? Also, why do >> > we have both CamHelper and CameraSensorHelper, with former being >> > significantly more featureful, but all the IPAs except rpi using the >> > latter? >> >> Because Raspberry Pi create their own IPA - which doesn't use the >> CameraSensorHelpers - because they are not as featureful ... and all the >> other IPAs can not use the Raspberry Pi helpers - because they are in >> the Raspberry Pi IPA instead of common code ... and no one has yet tried >> to align the libipa code until now. > > It's also because the RPi IPA predates CameraSensorHelper if I recall > correctly. > >> Ultimately - Making sure that the helpers in libipa can be grown in a >> way that Raspberry Pi can also use them will then mean that we only need >> to have *one* location for all the common camera sensor specific >> helpers. I see, thanks. I'll stick to growing CameraSensorHelper as needed for now. -- Best regards, Mikhail