[libcamera-devel] Questions about tonemapping
David Plowman
david.plowman at raspberrypi.com
Wed Jun 16 14:21:53 CEST 2021
Hi Paul
Thanks for sending this round!
On Wed, 16 Jun 2021 at 10:46, <paul.elder at ideasonboard.com> wrote:
>
> Hi Naush,
>
> On Wed, Jun 16, 2021 at 10:11:57AM +0100, Naushir Patuck wrote:
> > Hi Paul,
> >
> > Here are some initial thoughts from me. I'm sure David will have some views on
> > this as well.
>
> Thank you for the feedback.
>
> >
> > On Wed, 16 Jun 2021 at 09:47, <paul.elder at ideasonboard.com> wrote:
> >
> > Hello Naush and David,
> >
> > We are planning to add controls for tonemapping [1], and we were
> > thinking to use the same controls that the android HAL uses.
> >
> > Something along the lines of:
> >
> > - TonemapMode
> > type: int32_t
> > draft: true
> > description: |
> > High-level global contrast/gamma/tonemapping control. Currently
> > identical to ANDROID_TONEMAP_MODE.
> > enum:
> > - name: TonemapModeContrastCurve
> > value: 0
> > description: |
> > Use the tonemapping curve specified in TonemapCurve* entries.
> > All color enhancement and tonemapping must be disabled, except
> > for applying the tonemapping curve specified by TonemapCurve*.
> > - name: TonemapModeFast
> > value: 1
> > description: |
> > Advanced gamma mapping and color enhancement may be applied,
> > without reducing frame rate compared to raw sensor output.
> > - name: TonemapModeHighQuality
> > value: 2
> > description: |
> > High-quality gamma mapping and color enhancement will be
> > applied, at the cost of possibly reduced frame rate compared to
> > raw sensor output.
> >
> >
> > I always wondered why Android had a Fast and HQ mode for tone mapping -
> > assuming this
> > is talking about global contrast enhancement, and not local which the
> > description implies.
> > All it is doing is an in -> out mapping, so I don't get why we need different
> > quality settings.
>
> Maybe, for example, the fast one has fewer points in the mapping while
> the hq one has more, and the number of points affects the processing
> speed...?
If you're doing it in h/w it's hard to imagine that the number of
points, or the precision, would affect throughput. However, I guess
you could have a h/w implementation that has poor precision and so you
might under some circumstances prefer to do it in software. This is of
course likely to be much slower (especially if you subsequently have
to do YUV conversions in software as well).
>
> If you don't think that we need to separate them we could unify them
> as letting the IPA decide on the curve, and in the HAL just map them
> both to fast and hq.
I don't mind. We'd treat "fast" and "hq" as being the same.
>
> > This may be related, but colour enhancement seems a bit vague, do you have any
> > more
> > details on that?
>
> tbh I don't know about that; I just copy and pasted the description from
> the android HAL documentation... :S
I agree that it would be good to understand what this is about. What's
colour enhancement got to do with gamma curves?
>
> >
> >
> > - name: TonemapModeGammaValue
> > value: 3
> > description: |
> > Use the gamma value specified in TonemapGamma to perform
> > tonemapping. All color enhancement and tonemapping must be
> > disabled, except for applying the tonemapping curve specified
> > by TonemapGamma.
> > - name: TonemapModePresetCurve
> > value: 4
> > description: |
> > Use the present tonemapping curve specified in
> > TonemapPresetCurve to perform tonemapping. All color
> > enhancement and tonemapping must be disabled, except for
> > applying the tonemapping curve specified by TonemapPresetCurve.
> >
> > - TonemapGamma
> > type: float
> > draft: true
> > description: |
> > Tonemapping curve to use when TonemapMode is TonemapModeGammaValue.
> > The tonemap curve will be defined by the following formula: * OUT =
> > pow(IN, 1.0 / gamma) where IN and OUT is the input pixel value
> > scaled to range [0.0, 1.0], pow is the power function and gamma is
> > the gamma value specified by this key. The same curve will be
> > applied to all color channels. The camera device may clip the input
> > gamma value to its supported range. The actual applied value will
> > be returned in result metadata. The valid range of gamma value
> > varies on different devices, but values within [1.0, 5.0] are
> > guaranteed not to be clipped. Currently identical to
> > ANDROID_TONEMAP_GAMMA.
> >
> > - TonemapPresetCurve
> > type: int32_t
> > draft: true
> > description: |
> > Tonemapping curve to use when TonemapMode is
> > TonemapModePresetCurve.
> > enum:
> > - name: TonemapPresetCurveSRGB
> > value: 0
> > description: [2]
> > - name: TonemapPresetCurveREC709
> > value: 1
> > description: [3]
I might have added an "identity" option in case anyone wants linear
output. In fact, I did! https://patchwork.libcamera.org/patch/12519/
:-)
> >
> >
> > This bit is very related to the ColourSpace work that David has submitted for
> > review. My feeling is that these presets do not make much sense in isolation
> > without
> > specifying things like colour primaries and ranges.
>
> Maybe we could use this as an int alone and the IPA could have its own
> preset curves instead?
I think it's OK to refer to "SRGB" or "REC709" transfer functions. The
understanding - at least on the Pi platform - is that the camera
tuning and the underlying system will interpret these directions
"appropriately".
>
> Although then that won't map well to the HAL. Maybe that's okay, or we
> could just map to sRGB anyway?
>
> Fast/hq also use preset curves though, so we could ditch this control
> entirely?
My overall feeling here is that whilst a tonemap control is fine, we
do slightly have the tail wagging the dog. I can't see why an
application would ever set a specific tonemap curve, except perhaps in
some very niche circumstances. But I can see an application requesting
a specific colour space. In fact, every application should do so every
time it runs.
So I'd be nervous about implementing this control until we have
figured out how it interacts with colour spaces. Realistically, I
think that means adding colour space support first, possibly along the
lines discussed here:
https://patchwork.libcamera.org/project/libcamera/list/?series=2111
Having a control for the tonemap then seems reasonable. Certainly a
fast/hq option makes sense (like we have fast/hq options for denoise),
whilst the options that set specific transfer curves seem more
dubious. But perhaps platforms can choose to implement those or not?
>
> >
> >
> >
> > - TonemapCurve{Red,Green,Blue}
> > type: float
> > draft: true
> > description: |
> > An array of [ [in, out], [in, out], ... ] format to describe a
> > tonemapping curve to be applied when TonemapMode is set to
> > TonemapModeContrastCurve.
> > size: [64] (we need some way to specify the size)
Specifying SRGB or REC709 outside of the choice of colour space seems
a bit weird to me, but supplying exact curves or gamma values seems
considerably stranger again. How would an application know what to ask
for? Why would it override the tuning that's been done carefully for
this sensor? Putting that aside, if we want to allow users to ask for
stupid gamma curves - I suppose we can allow it. Though this might be
one of those things the Pi would rather not implement...
> >
> >
> > Generally speaking, do you expect different curves for R/G/B? There may be
>
> android's TonemapCurve control describes the tonemap curve with a 2xMx3
> array. (we don't necessarily have to do the same thing, that's just the
> starting point of the brainstorm)
I've never really understood why anyone would ever want different
curves for R/G/B. It would turn things that your AWB has carefully
turned to grey, non-grey again. That seems a bit weird, but I have no
particular objection for platforms that can support that.
>
> > a hardware limitation that only allows a single curve, so perhaps the
> > description
> > could be updated to reflect this?
>
> The description reflects this imo, just that the control name doesn't.
>
> Maybe we could have a separate control that would use a single curve for
> all of R/G/B? Then I'm wondering how to handle the case if an android
> application requests different curves; should we just average or take
> only one curve? Or not allow the application to set curves at all if the
> hardware doesn't support multicolor curves?
>
> >
> >
> > What feedback do you have on these controls? How might you implement
> > these? Is it something that could be done? Do you forsee any issues?
> >
> >
> > Overall, I do find it quite strange that a user can take over the gamma/ce
> > control
> > from the AGC. I think the biggest challenge is how does this control play well
> > with
> > AGC? Perhaps there needs to be a Auto tone-mapping mode that allows the AGC
> > to do what it needs to do?
>
> fast, hq, and preset curve modes are auto, so that should be fine.
On the Pi platform, we've actually taken control of the gamma curve
away from the AGC and put it into a separate control algorithm, which
we call the "contrast" algorithm. We specify a custom curve in our
tuning file which then gets adjusted over on the GPU according to the
selected colour space.
One feature we have is "adaptive contrast enhancement". So when an
image histogram suggests that the global contrast is a bit low then we
boost the contrast in the gamma curves. We might consider providing
the ability to enable or disable this feature. (See
https://git.linuxtv.org/libcamera.git/tree/src/ipa/raspberrypi/controller/rpi/contrast.cpp)
(Note how we also apply user-supplied global brightness/contrast
adjustments in our gamma curves too.)
That's probably enough for now! To summarise:
* It seems mostly ok to me.
* Some of it seems rather Android specific and I'm not clear how
useful it is for other clients.
* But it doesn't bother me that much, particularly if a platform can
opt out of those bits!
* However: have a plan for colour spaces at the same time! (It's a
shame Android doesn't appear to have that much to say about them.)
Best regards
David
>
> > Otherwise, this seems to be an easy way to mess
> > up the
> > quality of the AGC output, even though the intention may be the opposite :-)
>
> Aha, yeah :)
>
>
> Thanks,
>
> Paul
>
> >
> > Another one is how this plays with Colourspace setup, where the tone mapping is
> > just one
> > part of the equation.
> >
> > [1] https://bugs.libcamera.org/show_bug.cgi?id=56
> > [2] https://developer.android.com/reference/images/camera2/metadata/
> > android.tonemap.curveRed/srgb_tonemap.png
> > [3] https://developer.android.com/reference/images/camera2/metadata/
> > android.tonemap.curveRed/rec709_tonemap.png
> >
More information about the libcamera-devel
mailing list