[libcamera-devel,v3] libcamera: controls: Add controls for HDR

19101 diff mbox series

We add an HdrMode control (to enable and disable HDR processing)
and an HdrChannel, which indicates what kind of HDR frame (short, long
or otherwise) has just arrived.

Currently the HdrMode supports the following values:

* Off - no HDR processing at all.
* MultiExposureUnmerged - frames at multiple different exposures are
  produced, but not merged together. They are returned "as is".
* MultiExposure - frames at multiple different exposures are merged
  to create HDR images.
* SingleExposure - multiple frames all at the same exposure are
  merged to create HDR images.
* Night - multiple frames will be combined to create "night mode"
  images.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/libcamera/control_ids.yaml | 63 ++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

Hi David,

Quoting David Plowman via libcamera-devel (2023-10-05 11:41:33)
> We add an HdrMode control (to enable and disable HDR processing)
> and an HdrChannel, which indicates what kind of HDR frame (short, long
> or otherwise) has just arrived.
> 
> Currently the HdrMode supports the following values:
> 
> * Off - no HDR processing at all.
> * MultiExposureUnmerged - frames at multiple different exposures are
>   produced, but not merged together. They are returned "as is".
> * MultiExposure - frames at multiple different exposures are merged
>   to create HDR images.
> * SingleExposure - multiple frames all at the same exposure are
>   merged to create HDR images.
> * Night - multiple frames will be combined to create "night mode"
>   images.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/libcamera/control_ids.yaml | 63 ++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index f2e542f4..e3699e74 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -774,6 +774,69 @@ 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: HdrModeMultiExposureUnmerged
> +          value: 1
> +          description: |
> +            Multiple exposures will be generated in an alternating fashion.
> +            However, they will not be merged together and will be returned to
> +            the application as they are. The expectation is that, if necessary,
> +            the application can merge them to create HDR images for itself.
> +        - name: HdrModeMultiExposure
> +          value: 2
> +          description: |
> +            Multiple exposures will be generated and merged to create HDR
> +            images.
> +        - name: HdrModeSingleExposure
> +          value: 3
> +          description: |
> +            Multiple frames all at a single exposure will be used to create HDR
> +            images.
> +        - name: HdrModeNight
> +          value: 4
> +          description: |
> +            Multiple frames will be combined to produce "night mode" images.

MdrModeMultiExposureUnmerged has been documented to describe what that
is doing. HdrModeMultiExposure, and HdrModeSingleExposure are more self
explanatory - but HdrModeNight might need more explaining.

Does this just mean sequential images are stacked? Are there different
exposures? Are there any other expectations that a user may need or
assumptions they could get wrong?

Or is it just 'Hey do anything you like to make pictures look better at
night time or in low light conditions'?.



> +
> +  - 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).
> +
> +      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.
> +        - name: HdrChannelNight
> +          value: 4
> +          description: |
> +            This frame has been used to produce a "night mode" image.

What does it mean if a 'single' frame has been used to produce a night
mode image. Does that mean it's blended with previous frames? Or is this
frame only 'one' of a set?

"Frame has been used to produce" makes it sound like the result of using
this frame will be output later and maybe this frame should be discarded
because it's not the "final" night mode image ?



> +
>    # ----------------------------------------------------------------------------
>    # Draft controls section
>  
> -- 
> 2.30.2
>

Hi Kieran

Thanks for the questions!

On Wed, 11 Oct 2023 at 11:20, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Hi David,
>
> Quoting David Plowman via libcamera-devel (2023-10-05 11:41:33)
> > We add an HdrMode control (to enable and disable HDR processing)
> > and an HdrChannel, which indicates what kind of HDR frame (short, long
> > or otherwise) has just arrived.
> >
> > Currently the HdrMode supports the following values:
> >
> > * Off - no HDR processing at all.
> > * MultiExposureUnmerged - frames at multiple different exposures are
> >   produced, but not merged together. They are returned "as is".
> > * MultiExposure - frames at multiple different exposures are merged
> >   to create HDR images.
> > * SingleExposure - multiple frames all at the same exposure are
> >   merged to create HDR images.
> > * Night - multiple frames will be combined to create "night mode"
> >   images.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  src/libcamera/control_ids.yaml | 63 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> >
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index f2e542f4..e3699e74 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -774,6 +774,69 @@ 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: HdrModeMultiExposureUnmerged
> > +          value: 1
> > +          description: |
> > +            Multiple exposures will be generated in an alternating fashion.
> > +            However, they will not be merged together and will be returned to
> > +            the application as they are. The expectation is that, if necessary,
> > +            the application can merge them to create HDR images for itself.
> > +        - name: HdrModeMultiExposure
> > +          value: 2
> > +          description: |
> > +            Multiple exposures will be generated and merged to create HDR
> > +            images.
> > +        - name: HdrModeSingleExposure
> > +          value: 3
> > +          description: |
> > +            Multiple frames all at a single exposure will be used to create HDR
> > +            images.
> > +        - name: HdrModeNight
> > +          value: 4
> > +          description: |
> > +            Multiple frames will be combined to produce "night mode" images.
>
> MdrModeMultiExposureUnmerged has been documented to describe what that
> is doing. HdrModeMultiExposure, and HdrModeSingleExposure are more self
> explanatory - but HdrModeNight might need more explaining.
>
> Does this just mean sequential images are stacked? Are there different
> exposures? Are there any other expectations that a user may need or
> assumptions they could get wrong?
>
> Or is it just 'Hey do anything you like to make pictures look better at
> night time or in low light conditions'?.

It means this last thing. I don't feel I can judge what any other
vendor will do HDR-wise, so these modes are the most generic "hooks" I
can think of that stand a fair chance of vendors at large being able
to hang their HDR implementations off them.

For night mode in particular - it's "do whatever it is that you do".
Though there's an implicit "probably do it with some kind of frame
merging or HDR-like procedure", because I think a number of vendors do
indeed do this. But there's no knowing for sure. "Night modes" might
be completely different things too, like we have this magic HDR mode
in the imx708 sensor which just doesn't fit in here.

>
>
>
> > +
> > +  - 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).
> > +
> > +      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.
> > +        - name: HdrChannelNight
> > +          value: 4
> > +          description: |
> > +            This frame has been used to produce a "night mode" image.
>
> What does it mean if a 'single' frame has been used to produce a night
> mode image. Does that mean it's blended with previous frames? Or is this
> frame only 'one' of a set?

It's hard to say what this really means, as I think different vendors
will do all kinds of different things. Unfortunately I suspect the HDR
or "night mode" is likely to be very vendor specific, and we may find
they work quite differently.

In our case, all it really means is that the exposure for the frame
was calculated by AEC/AGC running with its "night mode" settings. It
will have been combined with previous "night mode" frames (if any).

>
> "Frame has been used to produce" makes it sound like the result of using
> this frame will be output later and maybe this frame should be discarded
> because it's not the "final" night mode image ?

To some extent this is me using vague language because I can't say for
sure what anyone else will do. Actually it does so happen that in our
"night mode" you will need to discard a few frames to give the image
merging time to work (you could take the very first image - but it
will be noisy). But other vendors may not do it like this. Or they
may. But probably not. Who knows!

But I'm definitely open to rewording any of this if we can agree on
the level of vaguess/specificness that's required...

David

>
>
>
> > +
> >    # ----------------------------------------------------------------------------
> >    # Draft controls section
> >
> > --
> > 2.30.2
> >

Hello David,

Chiming in at last, sorry for the delay.

On Wed, Oct 11, 2023 at 11:39:26AM +0100, David Plowman via libcamera-devel wrote:
> On Wed, 11 Oct 2023 at 11:20, Kieran Bingham wrote:
> > Quoting David Plowman via libcamera-devel (2023-10-05 11:41:33)
> > > We add an HdrMode control (to enable and disable HDR processing)
> > > and an HdrChannel, which indicates what kind of HDR frame (short, long
> > > or otherwise) has just arrived.
> > >
> > > Currently the HdrMode supports the following values:
> > >
> > > * Off - no HDR processing at all.
> > > * MultiExposureUnmerged - frames at multiple different exposures are
> > >   produced, but not merged together. They are returned "as is".
> > > * MultiExposure - frames at multiple different exposures are merged
> > >   to create HDR images.
> > > * SingleExposure - multiple frames all at the same exposure are
> > >   merged to create HDR images.
> > > * Night - multiple frames will be combined to create "night mode"
> > >   images.
> > >
> > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > > ---
> > >  src/libcamera/control_ids.yaml | 63 ++++++++++++++++++++++++++++++++++
> > >  1 file changed, 63 insertions(+)
> > >
> > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > index f2e542f4..e3699e74 100644
> > > --- a/src/libcamera/control_ids.yaml
> > > +++ b/src/libcamera/control_ids.yaml
> > > @@ -774,6 +774,69 @@ 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.

Sounds quite non controversial :-) Maybe s/not enabled/disabled/

> > > +        - name: HdrModeMultiExposureUnmerged
> > > +          value: 1
> > > +          description: |
> > > +            Multiple exposures will be generated in an alternating fashion.
> > > +            However, they will not be merged together and will be returned to
> > > +            the application as they are. The expectation is that, if necessary,
> > > +            the application can merge them to create HDR images for itself.
> > > +        - name: HdrModeMultiExposure
> > > +          value: 2
> > > +          description: |
> > > +            Multiple exposures will be generated and merged to create HDR
> > > +            images.
> > > +        - name: HdrModeSingleExposure
> > > +          value: 3
> > > +          description: |
> > > +            Multiple frames all at a single exposure will be used to create HDR
> > > +            images.

If a platform supports both the multi-exposure and single-exposure
modes, how will an application decide which one to use, what are the
pros and cons ?

> > > +        - name: HdrModeNight
> > > +          value: 4
> > > +          description: |
> > > +            Multiple frames will be combined to produce "night mode" images.
> >
> > MdrModeMultiExposureUnmerged has been documented to describe what that
> > is doing. HdrModeMultiExposure, and HdrModeSingleExposure are more self
> > explanatory

It feels a bit like we're mixing different concepts in the same control,
but I'm OK with it for now.

What does bother me though is that there's no definition of HDR
anywhere. I think that needs to be fixed, in order to clearly set the
boundaries of what falls in the category of HDR-related controls, and
what is entirely separate.

> > - but HdrModeNight might need more explaining.
> >
> > Does this just mean sequential images are stacked? Are there different
> > exposures? Are there any other expectations that a user may need or
> > assumptions they could get wrong?
> >
> > Or is it just 'Hey do anything you like to make pictures look better at
> > night time or in low light conditions'?.
> 
> It means this last thing. I don't feel I can judge what any other
> vendor will do HDR-wise, so these modes are the most generic "hooks" I
> can think of that stand a fair chance of vendors at large being able
> to hang their HDR implementations off them.
> 
> For night mode in particular - it's "do whatever it is that you do".
> Though there's an implicit "probably do it with some kind of frame
> merging or HDR-like procedure", because I think a number of vendors do
> indeed do this. But there's no knowing for sure. "Night modes" might
> be completely different things too,

If it's not HDR, then does it really belong to this control ?

> like we have this magic HDR mode
> in the imx708 sensor which just doesn't fit in here.

What is that magic HDR mode ?

> > > +
> > > +  - 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).

You mention short and long here, but there's also a medium value below.
If the implementation uses two exposures only, which ones will it be ?
This should be documented. Also, in the single exposure case, will the
channel be set to HdrChannelNone ? This should also be documented.
Similarly, in the HdrModeMultiExposure mode, what will be reported here,
given that all images will be the result of merging multiple exposures ?

> > > +
> > > +      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.
> > > +        - name: HdrChannelNight
> > > +          value: 4
> > > +          description: |
> > > +            This frame has been used to produce a "night mode" image.

This reminds me of the three-states boolean:
https://thedailywtf.com/articles/What_Is_Truth_0x3f_.

> > What does it mean if a 'single' frame has been used to produce a night
> > mode image. Does that mean it's blended with previous frames? Or is this
> > frame only 'one' of a set?
> 
> It's hard to say what this really means, as I think different vendors
> will do all kinds of different things. Unfortunately I suspect the HDR
> or "night mode" is likely to be very vendor specific, and we may find
> they work quite differently.
> 
> In our case, all it really means is that the exposure for the frame
> was calculated by AEC/AGC running with its "night mode" settings. It
> will have been combined with previous "night mode" frames (if any).

If I understand you correctly, this is single exposure HDR, but with AGC
running in night mode. Do you then need this value ? It seems to me that
you could report HdrChannelNone here, and applications would still know
that night mode is being used from the value of HdrMode.

> > "Frame has been used to produce" makes it sound like the result of using
> > this frame will be output later and maybe this frame should be discarded
> > because it's not the "final" night mode image ?
> 
> To some extent this is me using vague language because I can't say for
> sure what anyone else will do. Actually it does so happen that in our
> "night mode" you will need to discard a few frames to give the image
> merging time to work (you could take the very first image - but it
> will be noisy). But other vendors may not do it like this. Or they
> may. But probably not. Who knows!
> 
> But I'm definitely open to rewording any of this if we can agree on
> the level of vaguess/specificness that's required...

I want controls to be defined in an unambiguous way, for the benefit of
application developers who will then know what they get, and IPA
developers who will know what to implement. There's some leeway in the
sense that implementations can be given some latitude, but that latitude
needs to be documented with clear boundaries.

As guessing what other platforms would do, I would rather be more
precise here and document the use case(s) you want to target. When we
will implement support for other platforms, we'll revisit it.

> > > +
> > >    # ----------------------------------------------------------------------------
> > >    # Draft controls section
> > >

Hi Laurent

Thanks for the comments!

On Thu, 19 Oct 2023 at 00:54, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hello David,
>
> Chiming in at last, sorry for the delay.
>
> On Wed, Oct 11, 2023 at 11:39:26AM +0100, David Plowman via libcamera-devel wrote:
> > On Wed, 11 Oct 2023 at 11:20, Kieran Bingham wrote:
> > > Quoting David Plowman via libcamera-devel (2023-10-05 11:41:33)
> > > > We add an HdrMode control (to enable and disable HDR processing)
> > > > and an HdrChannel, which indicates what kind of HDR frame (short, long
> > > > or otherwise) has just arrived.
> > > >
> > > > Currently the HdrMode supports the following values:
> > > >
> > > > * Off - no HDR processing at all.
> > > > * MultiExposureUnmerged - frames at multiple different exposures are
> > > >   produced, but not merged together. They are returned "as is".
> > > > * MultiExposure - frames at multiple different exposures are merged
> > > >   to create HDR images.
> > > > * SingleExposure - multiple frames all at the same exposure are
> > > >   merged to create HDR images.
> > > > * Night - multiple frames will be combined to create "night mode"
> > > >   images.
> > > >
> > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > > > ---
> > > >  src/libcamera/control_ids.yaml | 63 ++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 63 insertions(+)
> > > >
> > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > > index f2e542f4..e3699e74 100644
> > > > --- a/src/libcamera/control_ids.yaml
> > > > +++ b/src/libcamera/control_ids.yaml
> > > > @@ -774,6 +774,69 @@ 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.
>
> Sounds quite non controversial :-) Maybe s/not enabled/disabled/

Sure, can change that.

>
> > > > +        - name: HdrModeMultiExposureUnmerged
> > > > +          value: 1
> > > > +          description: |
> > > > +            Multiple exposures will be generated in an alternating fashion.
> > > > +            However, they will not be merged together and will be returned to
> > > > +            the application as they are. The expectation is that, if necessary,
> > > > +            the application can merge them to create HDR images for itself.
> > > > +        - name: HdrModeMultiExposure
> > > > +          value: 2
> > > > +          description: |
> > > > +            Multiple exposures will be generated and merged to create HDR
> > > > +            images.
> > > > +        - name: HdrModeSingleExposure
> > > > +          value: 3
> > > > +          description: |
> > > > +            Multiple frames all at a single exposure will be used to create HDR
> > > > +            images.
>
> If a platform supports both the multi-exposure and single-exposure
> modes, how will an application decide which one to use, what are the
> pros and cons ?

I think it's a case of "read the platform documentation". A Pi 5 will
do both, but we very strongly recommend use of single-exposure mode.

>
> > > > +        - name: HdrModeNight
> > > > +          value: 4
> > > > +          description: |
> > > > +            Multiple frames will be combined to produce "night mode" images.
> > >
> > > MdrModeMultiExposureUnmerged has been documented to describe what that
> > > is doing. HdrModeMultiExposure, and HdrModeSingleExposure are more self
> > > explanatory
>
> It feels a bit like we're mixing different concepts in the same control,
> but I'm OK with it for now.
>
> What does bother me though is that there's no definition of HDR
> anywhere. I think that needs to be fixed, in order to clearly set the
> boundaries of what falls in the category of HDR-related controls, and
> what is entirely separate.

I think that's a fair point, though it's quite hard to describe.

For us, it's a process of producing multiple frames at either a single
or multiple different exposures, and then optionally processing them
using methods that may include merging/fusing frames, and various
types of tonemapping.

But basically yeah, it's a real mixed bag of imaging techniques. We
want to bring to bear any functionality or tricks that we have in our
pipeline to accomplish certain outcomes, and we can probably expect
other vendors to take a similar view.

>
> > > - but HdrModeNight might need more explaining.
> > >
> > > Does this just mean sequential images are stacked? Are there different
> > > exposures? Are there any other expectations that a user may need or
> > > assumptions they could get wrong?
> > >
> > > Or is it just 'Hey do anything you like to make pictures look better at
> > > night time or in low light conditions'?.
> >
> > It means this last thing. I don't feel I can judge what any other
> > vendor will do HDR-wise, so these modes are the most generic "hooks" I
> > can think of that stand a fair chance of vendors at large being able
> > to hang their HDR implementations off them.
> >
> > For night mode in particular - it's "do whatever it is that you do".
> > Though there's an implicit "probably do it with some kind of frame
> > merging or HDR-like procedure", because I think a number of vendors do
> > indeed do this. But there's no knowing for sure. "Night modes" might
> > be completely different things too,
>
> If it's not HDR, then does it really belong to this control ?

Our implementation of "night mode" is a form of HDR. But I can't speak
for other vendors. If their "night mode" isn't a form of HDR, then I'd
expect them not to implement this here. Short of tuning the whole "HDR
Mode" thing into a "Special Vendor Imaging Mode", though I'm not sure
if we really want to go there just yet!

Generally I think a big problem in this area is that we have no clue
what any other vendor will want to do, so it's hard to know how
specific or precise we can be. I appreciate this is a general
libcamera problem, and as soon as we get beyond the trivial "set the
exposure to X" it becomes quite hard to know how to deal with any of
it.

>
> > like we have this magic HDR mode
> > in the imx708 sensor which just doesn't fit in here.
>
> What is that magic HDR mode ?

This is the in-sensor HDR mode, i.e. everything happens in the sensor.
Multiple exposures are merged and tonemapped, and turned back into raw
Bayer to be sent to the Pi. This is where we need V4L2 to indicate
special capabilities of its camera modes, but that's a whole other can
of worms.

>
> > > > +
> > > > +  - 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).
>
> You mention short and long here, but there's also a medium value below.
> If the implementation uses two exposures only, which ones will it be ?
> This should be documented. Also, in the single exposure case, will the
> channel be set to HdrChannelNone ? This should also be documented.
> Similarly, in the HdrModeMultiExposure mode, what will be reported here,
> given that all images will be the result of merging multiple exposures ?

I expect other HDR implementations would be quite likely to use a
"medium channel". So I don't mind dropping it... though I think at
some point someone would probably want to add it back. Maybe even us
one day! I can see arguments for "very short" and "very long" too...

In our implementation the HdrChannel is helpful because you can switch
seemlessly between the "off", "single-exposure" and "night" modes. It
obviously takes a few frames for each change to happen, but the
HdrChannel tells you exactly when you get a frame in the mode you've
requested. I would certainly want to preserve this.

Actually we aren't reporting the HdrMode currently, though I think we
clearly should. But knowing the long/short channel is still useful in
the "multi-exposure" mode. In some respects it's just that "this is
what the code does", but It's certainly useful to see the "long"
channel image come through. When you see that, then you know the
merging has happened. Even knowing the HdrMode isn't enough - because
the first frame in the "multi-exposure" mode might not be the result
of merging two frames (it might be made entirely from the short
frame). There is some degree of overlap with the "IQ stability"
control here, so these questions need to be addressed really quite
urgently.

>
> > > > +
> > > > +      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.
> > > > +        - name: HdrChannelNight
> > > > +          value: 4
> > > > +          description: |
> > > > +            This frame has been used to produce a "night mode" image.
>
> This reminds me of the three-states boolean:
> https://thedailywtf.com/articles/What_Is_Truth_0x3f_.
>
> > > What does it mean if a 'single' frame has been used to produce a night
> > > mode image. Does that mean it's blended with previous frames? Or is this
> > > frame only 'one' of a set?
> >
> > It's hard to say what this really means, as I think different vendors
> > will do all kinds of different things. Unfortunately I suspect the HDR
> > or "night mode" is likely to be very vendor specific, and we may find
> > they work quite differently.
> >
> > In our case, all it really means is that the exposure for the frame
> > was calculated by AEC/AGC running with its "night mode" settings. It
> > will have been combined with previous "night mode" frames (if any).
>
> If I understand you correctly, this is single exposure HDR, but with AGC
> running in night mode. Do you then need this value ? It seems to me that
> you could report HdrChannelNone here, and applications would still know
> that night mode is being used from the value of HdrMode.

Yes, I'm not sure. We aren't currently reporting back the value of
HdrMode, though as I've said we need to do that. With that, reporting
the night mode channel as being the "short" channel feels quite
natural to me, because that's what it is!

>
> > > "Frame has been used to produce" makes it sound like the result of using
> > > this frame will be output later and maybe this frame should be discarded
> > > because it's not the "final" night mode image ?
> >
> > To some extent this is me using vague language because I can't say for
> > sure what anyone else will do. Actually it does so happen that in our
> > "night mode" you will need to discard a few frames to give the image
> > merging time to work (you could take the very first image - but it
> > will be noisy). But other vendors may not do it like this. Or they
> > may. But probably not. Who knows!
> >
> > But I'm definitely open to rewording any of this if we can agree on
> > the level of vaguess/specificness that's required...
>
> I want controls to be defined in an unambiguous way, for the benefit of
> application developers who will then know what they get, and IPA
> developers who will know what to implement. There's some leeway in the
> sense that implementations can be given some latitude, but that latitude
> needs to be documented with clear boundaries.
>
> As guessing what other platforms would do, I would rather be more
> precise here and document the use case(s) you want to target. When we
> will implement support for other platforms, we'll revisit it.

In principle I'm happy to document "what the Pi does". Though I worry
that it is unlikely that other platforms would want to do the same
things and use the same mechanisms. But maybe we wait just for them to
catch us up... (I'm guessing this might take a while) ?

Thanks!
David

>
> > > > +
> > > >    # ----------------------------------------------------------------------------
> > > >    # Draft controls section
> > > >
>
> --
> Regards,
>
> Laurent Pinchart

Hello David,

On Thu, Oct 19, 2023 at 09:34:48AM +0100, David Plowman wrote:
> On Thu, 19 Oct 2023 at 00:54, Laurent Pinchart wrote:
> > > > Quoting David Plowman via libcamera-devel (2023-10-05 11:41:33)
> > > > > We add an HdrMode control (to enable and disable HDR processing)
> > > > > and an HdrChannel, which indicates what kind of HDR frame (short, long
> > > > > or otherwise) has just arrived.
> > > > >
> > > > > Currently the HdrMode supports the following values:
> > > > >
> > > > > * Off - no HDR processing at all.
> > > > > * MultiExposureUnmerged - frames at multiple different exposures are
> > > > >   produced, but not merged together. They are returned "as is".
> > > > > * MultiExposure - frames at multiple different exposures are merged
> > > > >   to create HDR images.
> > > > > * SingleExposure - multiple frames all at the same exposure are
> > > > >   merged to create HDR images.
> > > > > * Night - multiple frames will be combined to create "night mode"
> > > > >   images.
> > > > >
> > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > > > > ---
> > > > >  src/libcamera/control_ids.yaml | 63 ++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 63 insertions(+)
> > > > >
> > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > > > index f2e542f4..e3699e74 100644
> > > > > --- a/src/libcamera/control_ids.yaml
> > > > > +++ b/src/libcamera/control_ids.yaml
> > > > > @@ -774,6 +774,69 @@ 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.
> >
> > Sounds quite non controversial :-) Maybe s/not enabled/disabled/
> 
> Sure, can change that.
> 
> > > > > +        - name: HdrModeMultiExposureUnmerged
> > > > > +          value: 1
> > > > > +          description: |
> > > > > +            Multiple exposures will be generated in an alternating fashion.
> > > > > +            However, they will not be merged together and will be returned to
> > > > > +            the application as they are. The expectation is that, if necessary,
> > > > > +            the application can merge them to create HDR images for itself.
> > > > > +        - name: HdrModeMultiExposure
> > > > > +          value: 2
> > > > > +          description: |
> > > > > +            Multiple exposures will be generated and merged to create HDR
> > > > > +            images.
> > > > > +        - name: HdrModeSingleExposure
> > > > > +          value: 3
> > > > > +          description: |
> > > > > +            Multiple frames all at a single exposure will be used to create HDR
> > > > > +            images.
> >
> > If a platform supports both the multi-exposure and single-exposure
> > modes, how will an application decide which one to use, what are the
> > pros and cons ?
> 
> I think it's a case of "read the platform documentation". A Pi 5 will
> do both, but we very strongly recommend use of single-exposure mode.

Reading the platform documentation is what I would really like
developers *not* to do. It precludes writing portable applications, when
libcamera's goal is to abstract the platform as much as possible. This
being said, we all know that we can't make all platforms look exactly
the same (unless we aim for the lowest common denominator of not
covering any real use case at all), so this is more of a design
principle than a goal we want to fully achieve.

As discussed separately, I think we're trying to design a generic HDR
API based on a single platform, and without having a real idea of how
applications will want to use it. That's twice a recipe for certain
failure, and that's good news: we don't need to pretend we think we can
get it right :-) Let's proceed with what we have, see how users like or
dislike it (I'm sure you'll witness that on your forums), how future
platforms will compare, and we'll then fix the design mistakes.

> > > > > +        - name: HdrModeNight
> > > > > +          value: 4
> > > > > +          description: |
> > > > > +            Multiple frames will be combined to produce "night mode" images.
> > > >
> > > > MdrModeMultiExposureUnmerged has been documented to describe what that
> > > > is doing. HdrModeMultiExposure, and HdrModeSingleExposure are more self
> > > > explanatory
> >
> > It feels a bit like we're mixing different concepts in the same control,
> > but I'm OK with it for now.
> >
> > What does bother me though is that there's no definition of HDR
> > anywhere. I think that needs to be fixed, in order to clearly set the
> > boundaries of what falls in the category of HDR-related controls, and
> > what is entirely separate.
> 
> I think that's a fair point, though it's quite hard to describe.
> 
> For us, it's a process of producing multiple frames at either a single
> or multiple different exposures, and then optionally processing them
> using methods that may include merging/fusing frames, and various
> types of tonemapping.
> 
> But basically yeah, it's a real mixed bag of imaging techniques. We
> want to bring to bear any functionality or tricks that we have in our
> pipeline to accomplish certain outcomes, and we can probably expect
> other vendors to take a similar view.
> 
> > > > - but HdrModeNight might need more explaining.
> > > >
> > > > Does this just mean sequential images are stacked? Are there different
> > > > exposures? Are there any other expectations that a user may need or
> > > > assumptions they could get wrong?
> > > >
> > > > Or is it just 'Hey do anything you like to make pictures look better at
> > > > night time or in low light conditions'?.
> > >
> > > It means this last thing. I don't feel I can judge what any other
> > > vendor will do HDR-wise, so these modes are the most generic "hooks" I
> > > can think of that stand a fair chance of vendors at large being able
> > > to hang their HDR implementations off them.
> > >
> > > For night mode in particular - it's "do whatever it is that you do".
> > > Though there's an implicit "probably do it with some kind of frame
> > > merging or HDR-like procedure", because I think a number of vendors do
> > > indeed do this. But there's no knowing for sure. "Night modes" might
> > > be completely different things too,
> >
> > If it's not HDR, then does it really belong to this control ?
> 
> Our implementation of "night mode" is a form of HDR. But I can't speak
> for other vendors. If their "night mode" isn't a form of HDR, then I'd
> expect them not to implement this here. Short of tuning the whole "HDR
> Mode" thing into a "Special Vendor Imaging Mode", though I'm not sure
> if we really want to go there just yet!
> 
> Generally I think a big problem in this area is that we have no clue
> what any other vendor will want to do, so it's hard to know how
> specific or precise we can be. I appreciate this is a general
> libcamera problem, and as soon as we get beyond the trivial "set the
> exposure to X" it becomes quite hard to know how to deal with any of
> it.
> 
> > > like we have this magic HDR mode
> > > in the imx708 sensor which just doesn't fit in here.
> >
> > What is that magic HDR mode ?
> 
> This is the in-sensor HDR mode, i.e. everything happens in the sensor.
> Multiple exposures are merged and tonemapped, and turned back into raw
> Bayer to be sent to the Pi. This is where we need V4L2 to indicate
> special capabilities of its camera modes, but that's a whole other can
> of worms.
> 
> > > > > +
> > > > > +  - 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).
> >
> > You mention short and long here, but there's also a medium value below.
> > If the implementation uses two exposures only, which ones will it be ?
> > This should be documented. Also, in the single exposure case, will the
> > channel be set to HdrChannelNone ? This should also be documented.
> > Similarly, in the HdrModeMultiExposure mode, what will be reported here,
> > given that all images will be the result of merging multiple exposures ?
> 
> I expect other HDR implementations would be quite likely to use a
> "medium channel". So I don't mind dropping it... though I think at
> some point someone would probably want to add it back. Maybe even us
> one day! I can see arguments for "very short" and "very long" too...
> 
> In our implementation the HdrChannel is helpful because you can switch
> seemlessly between the "off", "single-exposure" and "night" modes. It
> obviously takes a few frames for each change to happen, but the
> HdrChannel tells you exactly when you get a frame in the mode you've
> requested. I would certainly want to preserve this.
> 
> Actually we aren't reporting the HdrMode currently, though I think we
> clearly should. But knowing the long/short channel is still useful in
> the "multi-exposure" mode. In some respects it's just that "this is
> what the code does", but It's certainly useful to see the "long"
> channel image come through. When you see that, then you know the
> merging has happened. Even knowing the HdrMode isn't enough - because
> the first frame in the "multi-exposure" mode might not be the result
> of merging two frames (it might be made entirely from the short
> frame). There is some degree of overlap with the "IQ stability"
> control here, so these questions need to be addressed really quite
> urgently.
> 
> > > > > +
> > > > > +      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.
> > > > > +        - name: HdrChannelNight
> > > > > +          value: 4
> > > > > +          description: |
> > > > > +            This frame has been used to produce a "night mode" image.
> >
> > This reminds me of the three-states boolean:
> > https://thedailywtf.com/articles/What_Is_Truth_0x3f_.
> >
> > > > What does it mean if a 'single' frame has been used to produce a night
> > > > mode image. Does that mean it's blended with previous frames? Or is this
> > > > frame only 'one' of a set?
> > >
> > > It's hard to say what this really means, as I think different vendors
> > > will do all kinds of different things. Unfortunately I suspect the HDR
> > > or "night mode" is likely to be very vendor specific, and we may find
> > > they work quite differently.
> > >
> > > In our case, all it really means is that the exposure for the frame
> > > was calculated by AEC/AGC running with its "night mode" settings. It
> > > will have been combined with previous "night mode" frames (if any).
> >
> > If I understand you correctly, this is single exposure HDR, but with AGC
> > running in night mode. Do you then need this value ? It seems to me that
> > you could report HdrChannelNone here, and applications would still know
> > that night mode is being used from the value of HdrMode.
> 
> Yes, I'm not sure. We aren't currently reporting back the value of
> HdrMode, though as I've said we need to do that. With that, reporting
> the night mode channel as being the "short" channel feels quite
> natural to me, because that's what it is!

If a single channel is used, is it meaningful to report which channel
that is ?

> > > > "Frame has been used to produce" makes it sound like the result of using
> > > > this frame will be output later and maybe this frame should be discarded
> > > > because it's not the "final" night mode image ?
> > >
> > > To some extent this is me using vague language because I can't say for
> > > sure what anyone else will do. Actually it does so happen that in our
> > > "night mode" you will need to discard a few frames to give the image
> > > merging time to work (you could take the very first image - but it
> > > will be noisy). But other vendors may not do it like this. Or they
> > > may. But probably not. Who knows!
> > >
> > > But I'm definitely open to rewording any of this if we can agree on
> > > the level of vaguess/specificness that's required...
> >
> > I want controls to be defined in an unambiguous way, for the benefit of
> > application developers who will then know what they get, and IPA
> > developers who will know what to implement. There's some leeway in the
> > sense that implementations can be given some latitude, but that latitude
> > needs to be documented with clear boundaries.
> >
> > As guessing what other platforms would do, I would rather be more
> > precise here and document the use case(s) you want to target. When we
> > will implement support for other platforms, we'll revisit it.
> 
> In principle I'm happy to document "what the Pi does". Though I worry
> that it is unlikely that other platforms would want to do the same
> things and use the same mechanisms. But maybe we wait just for them to
> catch us up... (I'm guessing this might take a while) ?

Or to do something else entirely. We need more data points, whatever the
data will be, and we'll then revisit these controls.

> > > > > +
> > > > >    # ----------------------------------------------------------------------------
> > > > >    # Draft controls section
> > > > >