[libcamera-devel] [PATCH 1/1] libcamera: controls: Add controls for HDR

[Jacopo Mondi]

Hi David

On Mon, Aug 21, 2023 at 10:06:46AM +0100, David Plowman via libcamera-devel wrote:
> Hi Naush
>
> Thanks for the comments.
>
> On Mon, 21 Aug 2023 at 09:41, Naushir Patuck <naush at raspberrypi.com> wrote:
> >
> > Hi David,
> >
> > Thank you for your patch.
> >
> > On Fri, 28 Jul 2023 at 13:28, David Plowman via libcamera-devel
> > <libcamera-devel at lists.libcamera.org> wrote:
> > >
> > > We add an HdrMode control (to enable and disable HDR processing)
> > > and an HdrChannel, which indicates what kind of HDR frame (short, long
> > > medium) has just arrived.
> > >
> > > Currently the HdrMode supports three values:
> > >
> > > * Off - no HDR processing at all.
> > > * MultiExposure - frames at multiple different exposures are combined
> > >   to create HDR images.
> > > * SingleExposure - multiple frames all at the same exposure are
> > >   combined to create HDR images.
> > >
> > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > >  src/libcamera/control_ids.yaml | 47 ++++++++++++++++++++++++++++++++++
> > >  1 file changed, 47 insertions(+)
> > >
> > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > index 056886e6..34df7adb 100644
> > > --- a/src/libcamera/control_ids.yaml
> > > +++ b/src/libcamera/control_ids.yaml
> > > @@ -701,6 +701,53 @@ controls:
> > >              Continuous AF is paused. No further state changes or lens movements
> > >              will occur until the AfPauseResume control is sent.
> > >
> > > +  - HdrMode:
> > > +      type: int32_t
> > > +      description: |
> > > +        Control to set the mode to be used for High Dynamic Range (HDR)
> > > +        imaging.
> > > +
> > > +      enum:
> > > +        - name: HdrModeOff
> > > +          value: 0
> > > +          description: |
> > > +            HDR is not enabled.
> > > +        - name: HdrModeMultiExposure
> > > +          value: 1
> > > +          description: |
> > > +            Multiple exposures will be used to create HDR images.
> > > +        - name: HdrModeSingleExposure
> > > +          value: 2
> > > +          description: |
> > > +            Multiple frames all at a single exposure will be used to create HDR
> > > +            images.
> > > +
> > > +  - HdrChannel:
> > > +      type: int32_t
> > > +      description: |
> > > +        This value is reported back to the application so that it can discover
> > > +        whether this capture corresponds to the short or long exposure image (or
> > > +        any other image used by the HDR procedure).
> >
> > Only a minor comment about this control.  Instead of an enum, should we use an
> > explicit integer to determine channel number?  Then we are not limited to only
> > short/medium/long channels as below?
>
> This slightly gets us into the portable-applications vs.
> platform-specific-capabilities debate...
>
> I'd quite like to have "AgcChannel" metadata that tells you exactly
> this - namely which AGC channel this frame is from. But other
> platforms might have no such concept even if they have some kind of
> HDR support.
>
> So I'm slightly inclined to have a more generic notion of long/short
> frame for HDR, whilst leaving the door open to adding "AgcChannel"
> when it's needed.

Related to the discussion on "[PATCH v2 2/2] ipa: rpi: vc4: data:
Update tuning files for HDR ", the association between a channel index
(defined by their declaration order) and the menomic name is done in
the config file, using the "channel_map" property.

To know what "short" corresponds to, an application has to look at the
config file and reverse the association. Is this fine in your opinion ?


>
> But I agree that there's no clear answer here, and other people's
> opinions would be interesting.
>
> Thanks!
> David
>
> >
> > I'm not too bothered either way so,
> >
> > Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> >
> > > +      enum:
> > > +        - name: HdrChannelNone
> > > +          value: 0
> > > +          description: |
> > > +            This image does not correspond to any of the captures used to create
> > > +            an HDR image.
> > > +        - name: HdrChannelShort
> > > +          value: 1
> > > +          description: |
> > > +            This is a short exposure image.
> > > +        - name: HdrChannelMedium
> > > +          value: 2
> > > +          description: |
> > > +            This is a medium exposure image.
> > > +        - name: HdrChannelLong
> > > +          value: 3
> > > +          description: |
> > > +            This is a long exposure image.
> > > +
> > >    # ----------------------------------------------------------------------------
> > >    # Draft controls section
> > >
> > > --
> > > 2.30.2
> > >