[libcamera-devel] reporting of gain and analogue gain

[Laurent Pinchart]

Hi David,

On Mon, Oct 12, 2020 at 12:30:16PM +0100, David Plowman wrote:
> On Mon, 12 Oct 2020 at 11:01, Kieran Bingham wrote:
> > On 12/10/2020 08:03, David Plowman wrote:
> > > On Mon, 12 Oct 2020 at 00:44, Laurent Pinchart wrote:
> > >> On Sat, Oct 10, 2020 at 09:57:16AM +0100, David Plowman wrote:
> > >>> On Sat, 10 Oct 2020 at 02:43, Laurent Pinchart wrote:
> > >>>> On Wed, Oct 07, 2020 at 05:15:15PM +0100, David Plowman wrote:
> > >>>>> On Wed, 7 Oct 2020 at 15:58, Kieran Bingham wrote:
> > >>>>>> On 07/10/2020 15:41, David Plowman wrote:
> > >>>>>>> Hi everyone
> > >>>>>>>
> > >>>>>>> Whilst doing some maintenance on the Raspberry Pi AGC I noticed a bit
> > >>>>>>> of an oversight - we have no way to report the total gain being
> > >>>>>>> applied to an image. We have controls to report:
> > >>>>>>>
> > >>>>>>> * the exposure time of the sensor
> > >>>>>>> * the analogue gain applied in the sensor
> > >>>>>>> * red and blue colour gains applied by the AWB
> > >>>>>>>
> > >>>>>>> but nothing for any (global) digital gain demanded by the AGC. The
> > >>>>>>> consequence is that when I capture a jpeg I always report an ISO value
> > >>>>>>> based purely on the analogue gain which may be incorrect (when a
> > >>>>>>> higher gain is required than the sensor allows then we make up the
> > >>>>>>> difference with digital gain).
> > >>>>
> > >>>> Just to make sure there's no misunderstanding, that's the digital gain
> > >>>> applied in the sensor, right ?
> > >>>
> > >>> Sorry, I wasn't clear about that. No, I was referring to the gain
> > >>> applied by the ISP.
> > >>
> > >> I'm glad I asked a stupid question then :-)
> > >>
> > >> Should we consider the sensor digital gain too in this discussion, even
> > >> if not used for Raspberry Pi ?
> > >
> > > Yes, we probably should, though we'd certainly like it to be easy to
> > > continue to ignore it! I guess we could say something like:
> > >
> > > * A pipeline handler may, or may not, expose a "sensor digital gain" control.
> > > * A pipeline handler may, or may not, use the sensor's digital gain,
> > > if it has one.
> > > * If a pipeline handler uses the sensor's digital gain, it may, or may
> > > not, report the "sensor digital gain" explicitly.
> > > * If a pipeline handler uses, but does not report, the sensor's
> > > digital gain then it "should" fold the value into the reported
> > > analogue gain.
> > >
> > > That's all a bit vague, though!
> > >
> > >>>>>>> Any thoughts on this? Obviously there could be a digital gain control...
> > >>>>>>
> > >>>>>> I assume the IPA would be then responsible for setting this? Is it
> > >>>>>> read-only?
> > >>>>>
> > >>>>> Certainly in the Raspberry Pi implementation it would be a read-only
> > >>>>> control which the IPA sets for you. You could imagine some
> > >>>>> implementations would allow you to set it (the old proprietary stack
> > >>>>> of ours does, which I think is a source of confusion) but we're trying
> > >>>>> to keep the behaviour simple.
> > >>>>>
> > >>>>>> Would we expect an application to be able to control both the analogue
> > >>>>>> gain and the digital gain manually? (When AGC is disabled I guess?)
> > >>>>>>
> > >>>>>> Or would they only see an overall 'gain' control?
> > >>>>>
> > >>>>> The AGC algorithm we've implemented actually treats "gain" as a single
> > >>>>> thing. If the analogue gain goes high enough that's what you'll get.
> > >>>>> If you want to go higher, the analogue gain will max out and the
> > >>>>> remainder will be digital gain.
> > >>>>>
> > >>>>>> I'm suspecting that indeed, it would be exposed as a separate control,
> > >>>>>> because it will expose a feature that the ISP is capable of delivering.
> > >>>>>
> > >>>>> Yes, probably. I think maybe I feel another patch set coming on.
> > >>>>> Perhaps I'll get the other AGC ones out of the way first while we wait
> > >>>>> to see if anyone else has any opinions...
> > >>>>
> > >>>> I can see two options, either reporting the analog and digital gains as
> > >>>> separate controls, or as a combined gain control. The latter would allow
> > >>>> abstracting different sensor architectures more easily, at the expense
> > >>>> of not exposing the analog/digital split.
> > >>>>
> > >>>> Are there use cases on the application side that would benefit from
> > >>>> knowing the analog/digital gain split ? And are there use cases for
> > >>>> applying digital gain without maxing the analog gain out first ?
> > >>>
> > >>> The discussion probably changes a bit since I was referring to the
> > >>> digital gain in the ISP. Dealing with that one first, I think you
> > >>> could expect that an application would want to distinguish between the
> > >>> two. For example, when recording the "ISO" for a raw file you'd use
> > >>> the analogue gain, but when recording it for a jpeg you'd have to take
> > >>> both into account.
> > >>
> > >> Agreed. And if we take sensor digital gain into account too, that would
> > >> be recorded in the ISO value for both DNG and JPEG.
> >
> > So it sounds like we also need these to be stream specific metadata
> > entries (a bit like the discussion on pipeline depth will also need to
> > be stream specific ...)
> 
> That's an interesting thought. To be honest, I had imagined
> applications simply being able to find all the different gains so that
> they can figure it out for themselves, but I guess that's another way
> to look at it. I guess the two aren't mutually exclusive - report per
> stream gains by all means, but supplying metadata with all the
> different numbers recorded explicitly is desirable anyway.
> 
> > >>> I can imagine that some AGC implementations would let you set the
> > >>> digital gain in the ISP explicitly (indeed the existing Broadcom
> > >>> camera stack does!) but I think that's often been a source of
> > >>> confusion for us and our users and so, as part of my obsessive mission
> > >>> to make the IPAs simpler, I've combined the two together.
> > >>
> > >> Another stupid question, when you say you've combined the two together,
> > >> that's the ISP digital gain and ?
> > >
> > > The Raspberry PI behaviour is currently that you can ask for what is
> > > effectively a total gain. If the requested gain can be achieved purely
> > > with analogue gain, that's what you get. You only get digital gain
> > > when the analogue gain has maxed out. (Some of our users have actually
> > > expressed that this is the behaviour they expect.)
> >
> > This sounds reasonable to me at first hand, I can't imagine an
> > application 'preferring' digital gain over analogue gain... *1
> >
> > *1: Unless maybe an art installation wants to produce artificially noisy
> > images ;-) ? - There's always a use for an unwanted feature ... hehe
> 
> Well, I guess there's always one person somewhere who wants "feature
> X", however surprising it may seem. Overall I'm inclined to keep
> things simple for the vast majority of users, having been burnt by
> over-complexity with all this stuff in the past. If any such request
> becomes even slightly more mainstream then I'd happily revisit it, and
> I'd also hope that enough knowledge develops in the community (with
> our support) that folks can add such things for themselves.
> 
> > >>> (There'd be nothing to stop us limiting the analogue gain if we wanted
> > >>> to or - here's an entertaining idea - splitting the two gains up in
> > >>> the exposure profile so you could ramp them up separately, but
> > >>> overall, I'm disinclined to add this unless there's a requirement.)
> > >>>
> > >>> As you say, digital gain in the sensor is a thing too, though I can't
> > >>> say that we've ever used it much (maybe ever?) ourselves. It's
> > >>> certainly more reasonable to sweep that into the analogue gain value,
> > >>> I'd have thought, as it would apply to raw files too. Exposing that to
> > >>> application control is perfectly reasonable, I guess, though I think
> > >>> in Pi-world we would want to "opt out" of extra complexity like that!
> > >>
> > >> Do we also need to take the ColourGains property into account ? What are
> > >> all the gain stages you have in your hardware pipeline ?
> > >
> > > So the pipeline in the Pi only has one stage where all these gains are
> > > applied. There is a single gain value for each of red, green and blue.
> > > The interface to these three numbers is split in two so that we can
> > > define the colour gains (red and blue, which map onto
> > > V4L2_CID_RED/BLUE_BALANCE) and a "global" gain separately.
> > >
> > > The three gains applied by the hardware block are then exactly:
> > > red_gain*global_gain, global_gain (for green), and
> > > blue_gain*global_gain. These numbers are always chosen so that the
> > > smallest actual gain ever applied is 1 (though they will all get
> > > larger as more digital gain is demanded).
> > >
> > > Personally I think the handling of the colour gains seems mostly OK to
> > > me, though I would quite like V4L2_CID_GREEN_BALANCE if it were
> > > possible, as this would simplify the AGC somewhat (I could spend less
> > > time worrying about what happens when either the red or blue colour
> > > gains go less than one).
> >
> > I don't think I understand /why/ there isn't a V4L2_CID_GREEN_BALANCE
> > (though the drivers/media/usb/gspca/m5602 driver defines a custom one)
> >
> > A patch was submitted in 2009 [0], but seems Laurent asked for
> > clarification:
> >
> > [0] https://patchwork.kernel.org/patch/5101/
> >
> > > "What's a "green balance" exactly ? The "red balance" and "blue
> > > balance" controls make up the two components of the white balance
> > > control (which can also be expressed in color temperature units). How
> > > does the "green balance" relate to those ?"
> > Which seemed to kill the patch,
> >
> > Then in fact - it looks like nothing happened for some time until
> > another [1] attempt to get green balance adjustment in for gspca..
> >
> > [1] https://lore.kernel.org/linux-media/505ADD14.7070208@redhat.com/
> >
> > But that resulted in the following statement:
> >
> > > Many webcams have RGB gains, but we don't have standard CID-s for these,
> > > instead we've Blue and Red Balance. This has grown historically because of
> > > the bttv cards which actually have Blue and Red balance controls in hardware,
> > > rather then the usual RGB gain triplet. Various gspca drivers cope with this
> > > in different ways.
> > >
> > > If you look at the pac7302 driver before your latest 4 patches it has
> > > a Red and Blue balance control controlling the Red and Blue gain, and a
> > > Whitebalance control, which is not White balance at all, but simply
> > > controls the green gain...
> > >
> > > And as said other drivers have similar (albeit usually different) hacks.
> > >
> > > At a minimum I would like you to rework your patches to:
> > > 1) Not add the new Green balance, and instead modify the existing whitebalance
> > > to control the new green gain you've found. Keeping things as broken as
> > > they are, but not worse; and
> > > 2) Try to use both the page 0 reg 0x01 - 0x03 and page 0 reg 0xc5 - 0xc7
> > > at the same time to get a wider ranged control. Maybe 0xc5 - 0xc7 are
> > > simply the most significant bits of a wider ranged gain ?
> > > Note that if you cannot control them both from a single control in such
> > > a way that you get a smooth control range, then lets just fix
> > > 0xc5 - 0xc7 at a value of 2 for all 3 and be done with it, but at least
> > > we should try :)
> > >
> > > As said the above is the minimum, what I would really like is a discussion
> > > on linux-media about adding proper RGB gain controls for all the cameras
> > > which have these.
> >
> > And perhaps that discussion for proper RGB gain controls has never
> > happened? (Has it, and I can't find it?)
> >
> > Perhaps worse - not having it - is leading to complete mis-use [2] of
> > other controls in out-of-tree drivers (yadda yadda, we don't care about
> > out of tree yadda yadda - but if something leads people to do this -
> > it's bad!)
> >
> > [2] https://xterra2.avnet.com/xilinx/vitis_embedded_platform_source/zcu104/zcu104_mc4/-/blob/98a9b8f3cc816e6dc1abca1890bf9c2903db6ff4/petalinux/project-spec/meta-user/recipes-kernel/linux/linux-xlnx/0031-add-avt-multi-sensor-fmc-driver-2018-3.patch#L345
> >
> > If devices can distinctly expose each of an R, G, B gain - do we need
> > controls to expose those directly? or is handling this through the R/B
> > balance sufficient?
> >
> > What happens if you want to increase the G gain without increasing the
> > R/B gains? I guess that then implies in essence the only calculation is
> > to reduce the other gains...
> >
> > Or rather - perhaps because the controls are a 'colour balance' they are
> > relative (balanced) against Green, and therefore must always be set in
> > that way.
> 
> Wow. I can see there's a big can of worms there. From my point of
> view, the current arrangement is fine until one of the red or blue
> colour gains goes below one. The (red, green, blue) gains might for
> example be (0.8, 1.0, 1.6). Now that's bad because you will
> de-saturate the red channel and all your white highlights will go
> cyan.
> 
> So what the Pi AGC does is to drive the global gain
> (V4L2_CID_DIGITAL_GAIN) to 1/0.8 = 1.25 in this case (reducing the
> exposure and analogue gain to achieve that), so you'll actually end up
> with gains of 1.25*(0.8, 1.0, 1.6) = (1.0, 1.25, 2.0). If we could set
> "green balance" explicitly to 1.25 then we could avoid ever setting
> red/blue balances less than one, and the AGC simply wouldn't have to
> worry about it. There'd be less code, and a weird corner case would
> vanish. (You could argue that "balance" is the wrong term; we're
> really setting the three colour gains directly.)
> 
> But anyway, I don't want to get too worked up about this, I would say
> it classifies only as an "irritation". We can always add a custom
> control for "green balance" later... :-)

This doesn't invalidate any of the points above, but I'd like to point
out that the V4L2 API can always be extended. We shouldn't work around
API limitations, but make the API support our use cases. As has been
well explained above, the red and blue balance controls were ad-hoc
controls to expose the features of bttv hardware, and have then been
used to control colour gains in unrelated use cases instead of creating
new, more appropriate controls.

This isn't something that needs to be addressed as a prerequisite, but
if it would improve the AWB algorithm to create new colour gains (and in
that case I'd go for four gains, not three, or even a compound controls
for a gain array whose components could be device-specific), then I'd be
happy to submit a patch for the V4L2 API.

-- 
Regards,

Laurent Pinchart