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

19147 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 medium) 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 | 75 ++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

Hi David,

Thank you for the patch.

On Thu, Oct 19, 2023 at 12:52:20PM +0100, David Plowman via libcamera-devel wrote:
> We add an HdrMode control (to enable and disable HDR processing)
> and an HdrChannel, which indicates what kind of HDR frame (short, long
> or medium) 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 | 75 ++++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index f2e542f4..c3232abf 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -774,6 +774,81 @@ 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. HDR techniques typically include multiple exposure, image
> +        fusion and tone mapping techniques to improve the dynamic range of the
> +        resulting images.
> +
> +        When using an HDR mode, images are tagged to indicate which HDR channel
> +        (long, medium or short) they come from.
> +
> +        \sa HdrChannel
> +
> +      enum:
> +        - name: HdrModeOff
> +          value: 0
> +          description: |
> +            HDR is disabled. The HDR channel, if present, will report
> +            HdrChannelNone.

Stating what HDR channel is used is an improvement compared to the
previous version, but there's still an option left to implementors here:
reporting HdrChannelNone, or not reporting HdrChannel at all. Unless you
see a reason to allow both, I would pick the latter:

            HDR is disabled. Metadata for this frame will not include the
	    HdrChannel control.

> +        - 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. Each image will be tagged with the
> +            correct HDR channel, indicating what kind of exposure (long, medium
> +            or short) it is.  The expectation is that, if necessary, the
> +            application can merge them to create HDR images for itself.

You mention here long, medium and short. Does this mean there will
always be three channels ?

> +        - name: HdrModeMultiExposure
> +          value: 2
> +          description: |
> +            Multiple exposures will be generated and merged to create HDR
> +            images. Each image will be tagged with the HDR channel (long, medium
> +            or short) that arrived and which caused this image to be output.
> +        - name: HdrModeSingleExposure
> +          value: 3
> +          description: |
> +            Multiple frames all at a single exposure will be used to create HDR
> +            images. These images should be reported as all corresponding to the
> +            HDR short channel.
> +        - name: HdrModeNight
> +          value: 4
> +          description: |
> +            Multiple frames will be combined to produce "night mode"
> +            images. Images will be tagged as belonging either to the long,
> +            medium or short HDR channel according to the implementation.

Does this mean that night more will always use multi-exposure, or that
it is implementation-defined ?

> +
> +  - 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). An application can monitor
> +        the HDR channel to discover when the differently exposed images have
> +        arrived.
> +
> +      enum:
> +        - name: HdrChannelNone
> +          value: 0
> +          description: |
> +            This image does not correspond to any of the captures used to create
> +            an HDR image.

As indicated above, do we need this, or should we not report HdrChannel
when HDR is disabled ?

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

Hi Laurent

Thanks for the suggestions.

On Thu, 19 Oct 2023 at 21:52, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Thu, Oct 19, 2023 at 12:52:20PM +0100, David Plowman via libcamera-devel wrote:
> > We add an HdrMode control (to enable and disable HDR processing)
> > and an HdrChannel, which indicates what kind of HDR frame (short, long
> > or medium) 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 | 75 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 75 insertions(+)
> >
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index f2e542f4..c3232abf 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -774,6 +774,81 @@ 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. HDR techniques typically include multiple exposure, image
> > +        fusion and tone mapping techniques to improve the dynamic range of the
> > +        resulting images.
> > +
> > +        When using an HDR mode, images are tagged to indicate which HDR channel
> > +        (long, medium or short) they come from.
> > +
> > +        \sa HdrChannel
> > +
> > +      enum:
> > +        - name: HdrModeOff
> > +          value: 0
> > +          description: |
> > +            HDR is disabled. The HDR channel, if present, will report
> > +            HdrChannelNone.
>
> Stating what HDR channel is used is an improvement compared to the
> previous version, but there's still an option left to implementors here:
> reporting HdrChannelNone, or not reporting HdrChannel at all. Unless you
> see a reason to allow both, I would pick the latter:
>
>             HDR is disabled. Metadata for this frame will not include the
>             HdrChannel control.

Yes, I think that's fine.

>
> > +        - 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. Each image will be tagged with the
> > +            correct HDR channel, indicating what kind of exposure (long, medium
> > +            or short) it is.  The expectation is that, if necessary, the
> > +            application can merge them to create HDR images for itself.
>
> You mention here long, medium and short. Does this mean there will
> always be three channels ?

No - it's whatever the implementation wants to do. We don't use
medium, for example. I think it's quite likely that some vendors would
want other channels, such as "very short" and "very long". Maybe the
description can avoid implying that all channels will appear.

>
> > +        - name: HdrModeMultiExposure
> > +          value: 2
> > +          description: |
> > +            Multiple exposures will be generated and merged to create HDR
> > +            images. Each image will be tagged with the HDR channel (long, medium
> > +            or short) that arrived and which caused this image to be output.
> > +        - name: HdrModeSingleExposure
> > +          value: 3
> > +          description: |
> > +            Multiple frames all at a single exposure will be used to create HDR
> > +            images. These images should be reported as all corresponding to the
> > +            HDR short channel.
> > +        - name: HdrModeNight
> > +          value: 4
> > +          description: |
> > +            Multiple frames will be combined to produce "night mode"
> > +            images. Images will be tagged as belonging either to the long,
> > +            medium or short HDR channel according to the implementation.
>
> Does this mean that night more will always use multi-exposure, or that
> it is implementation-defined ?

I really think that needs to be implementation defined. Our night mode
is single-exposure, but I can't possibly predict what anyone else
would want to do.

>
> > +
> > +  - 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). An application can monitor
> > +        the HDR channel to discover when the differently exposed images have
> > +        arrived.
> > +
> > +      enum:
> > +        - name: HdrChannelNone
> > +          value: 0
> > +          description: |
> > +            This image does not correspond to any of the captures used to create
> > +            an HDR image.
>
> As indicated above, do we need this, or should we not report HdrChannel
> when HDR is disabled ?

Actually I'd quite like to keep this, even if it's not reported when HDR is off.

One use case is multi-exposure HDR on a Pi 4. You can't merge images
so how would you get a viewfinder? You could intersperse some
"ordinary AGC" frames with your long/short/whatever frames. You might
want to label these "HdrChannelNone". I suppose you could label them
"medium", but then maybe you're using "medium" for other frames that
your HDR implementation requires. Like everything else, it all falls a
bit into the "who knows what anyone will do" category.

I guess there's also an outside chance that some implementations can't
flick exposures instantaneously and accurately like we can, so maybe
it would suit them too. Or we could have an
"HdrChannelCouldntMakeThisWorkProperly" value? Having wrestled with
all this for a while I can see the attraction! :)

I'll send out another version today with the changes in line with all
the above. Obviously everyone please shout if we want to do anything
different.

Thanks!
David

>
> > +        - 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
> >
>
> --
> Regards,
>
> Laurent Pinchart

Hi David,

On Fri, Oct 20, 2023 at 09:37:28AM +0100, David Plowman wrote:
> On Thu, 19 Oct 2023 at 21:52, Laurent Pinchart wrote:
> > On Thu, Oct 19, 2023 at 12:52:20PM +0100, David Plowman via libcamera-devel wrote:
> > > We add an HdrMode control (to enable and disable HDR processing)
> > > and an HdrChannel, which indicates what kind of HDR frame (short, long
> > > or medium) 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 | 75 ++++++++++++++++++++++++++++++++++
> > >  1 file changed, 75 insertions(+)
> > >
> > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > index f2e542f4..c3232abf 100644
> > > --- a/src/libcamera/control_ids.yaml
> > > +++ b/src/libcamera/control_ids.yaml
> > > @@ -774,6 +774,81 @@ 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. HDR techniques typically include multiple exposure, image
> > > +        fusion and tone mapping techniques to improve the dynamic range of the
> > > +        resulting images.
> > > +
> > > +        When using an HDR mode, images are tagged to indicate which HDR channel
> > > +        (long, medium or short) they come from.
> > > +
> > > +        \sa HdrChannel
> > > +
> > > +      enum:
> > > +        - name: HdrModeOff
> > > +          value: 0
> > > +          description: |
> > > +            HDR is disabled. The HDR channel, if present, will report
> > > +            HdrChannelNone.
> >
> > Stating what HDR channel is used is an improvement compared to the
> > previous version, but there's still an option left to implementors here:
> > reporting HdrChannelNone, or not reporting HdrChannel at all. Unless you
> > see a reason to allow both, I would pick the latter:
> >
> >             HDR is disabled. Metadata for this frame will not include the
> >             HdrChannel control.
> 
> Yes, I think that's fine.
> 
> > > +        - 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. Each image will be tagged with the
> > > +            correct HDR channel, indicating what kind of exposure (long, medium
> > > +            or short) it is.  The expectation is that, if necessary, the
> > > +            application can merge them to create HDR images for itself.
> >
> > You mention here long, medium and short. Does this mean there will
> > always be three channels ?
> 
> No - it's whatever the implementation wants to do. We don't use
> medium, for example. I think it's quite likely that some vendors would
> want other channels, such as "very short" and "very long". Maybe the
> description can avoid implying that all channels will appear.

Shouldn't this be something that the application should control ? I'm
increasingly thinking that this shouldn't be an HDR mode, but should
instead be controlled through a per-frame control mechanism. Is there
any reason this can't be done, not right now (I don't want to delay this
patch unnecessarily), but at some point in the future ?

> > > +        - name: HdrModeMultiExposure
> > > +          value: 2
> > > +          description: |
> > > +            Multiple exposures will be generated and merged to create HDR
> > > +            images. Each image will be tagged with the HDR channel (long, medium
> > > +            or short) that arrived and which caused this image to be output.
> > > +        - name: HdrModeSingleExposure
> > > +          value: 3
> > > +          description: |
> > > +            Multiple frames all at a single exposure will be used to create HDR
> > > +            images. These images should be reported as all corresponding to the
> > > +            HDR short channel.
> > > +        - name: HdrModeNight
> > > +          value: 4
> > > +          description: |
> > > +            Multiple frames will be combined to produce "night mode"
> > > +            images. Images will be tagged as belonging either to the long,
> > > +            medium or short HDR channel according to the implementation.
> >
> > Does this mean that night more will always use multi-exposure, or that
> > it is implementation-defined ?
> 
> I really think that needs to be implementation defined. Our night mode
> is single-exposure, but I can't possibly predict what anyone else
> would want to do.

Won't it be problematic for applications if they don't know what
HdrChannel values they will get ? How do you expect HdrChannel to be
used in the HDR modes where the camera combines images (all but the
Unmerged 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). An application can monitor
> > > +        the HDR channel to discover when the differently exposed images have
> > > +        arrived.
> > > +
> > > +      enum:
> > > +        - name: HdrChannelNone
> > > +          value: 0
> > > +          description: |
> > > +            This image does not correspond to any of the captures used to create
> > > +            an HDR image.
> >
> > As indicated above, do we need this, or should we not report HdrChannel
> > when HDR is disabled ?
> 
> Actually I'd quite like to keep this, even if it's not reported when HDR is off.
> 
> One use case is multi-exposure HDR on a Pi 4. You can't merge images
> so how would you get a viewfinder? You could intersperse some
> "ordinary AGC" frames with your long/short/whatever frames. You might
> want to label these "HdrChannelNone". I suppose you could label them
> "medium", but then maybe you're using "medium" for other frames that
> your HDR implementation requires. Like everything else, it all falls a
> bit into the "who knows what anyone will do" category.

This makes me believe even more strongly that the unmerged mode
shouldn't be an HDR mode. We're trying to cram too many assumptions
about the application needs here.

If you want to keep HdrChannelNone for the time being until we drop
HdrMultiExposureUnmerged, then this needs better documentation. Until I
read your reply I had no idea that the unmerged mode would produce
images for the "None" channel. If this isn't documented properly, it
won't be usable for applications.

Depending on the priorities (I assume the Pi 5 HDR modes will be more
interesting than the unmerged operation on Pi 4), we could also drop the
unmerged mode for now, and design a solution better for application-side
HDR.

> I guess there's also an outside chance that some implementations can't
> flick exposures instantaneously and accurately like we can, so maybe
> it would suit them too. Or we could have an
> "HdrChannelCouldntMakeThisWorkProperly" value? Having wrestled with
> all this for a while I can see the attraction! :)
> 
> I'll send out another version today with the changes in line with all
> the above. Obviously everyone please shout if we want to do anything
> different.
> 
> > > +        - 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
> > >

HI Laurent

On Fri, 20 Oct 2023 at 10:55, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> On Fri, Oct 20, 2023 at 09:37:28AM +0100, David Plowman wrote:
> > On Thu, 19 Oct 2023 at 21:52, Laurent Pinchart wrote:
> > > On Thu, Oct 19, 2023 at 12:52:20PM +0100, David Plowman via libcamera-devel wrote:
> > > > We add an HdrMode control (to enable and disable HDR processing)
> > > > and an HdrChannel, which indicates what kind of HDR frame (short, long
> > > > or medium) 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 | 75 ++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 75 insertions(+)
> > > >
> > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > > index f2e542f4..c3232abf 100644
> > > > --- a/src/libcamera/control_ids.yaml
> > > > +++ b/src/libcamera/control_ids.yaml
> > > > @@ -774,6 +774,81 @@ 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. HDR techniques typically include multiple exposure, image
> > > > +        fusion and tone mapping techniques to improve the dynamic range of the
> > > > +        resulting images.
> > > > +
> > > > +        When using an HDR mode, images are tagged to indicate which HDR channel
> > > > +        (long, medium or short) they come from.
> > > > +
> > > > +        \sa HdrChannel
> > > > +
> > > > +      enum:
> > > > +        - name: HdrModeOff
> > > > +          value: 0
> > > > +          description: |
> > > > +            HDR is disabled. The HDR channel, if present, will report
> > > > +            HdrChannelNone.
> > >
> > > Stating what HDR channel is used is an improvement compared to the
> > > previous version, but there's still an option left to implementors here:
> > > reporting HdrChannelNone, or not reporting HdrChannel at all. Unless you
> > > see a reason to allow both, I would pick the latter:
> > >
> > >             HDR is disabled. Metadata for this frame will not include the
> > >             HdrChannel control.
> >
> > Yes, I think that's fine.
> >
> > > > +        - 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. Each image will be tagged with the
> > > > +            correct HDR channel, indicating what kind of exposure (long, medium
> > > > +            or short) it is.  The expectation is that, if necessary, the
> > > > +            application can merge them to create HDR images for itself.
> > >
> > > You mention here long, medium and short. Does this mean there will
> > > always be three channels ?
> >
> > No - it's whatever the implementation wants to do. We don't use
> > medium, for example. I think it's quite likely that some vendors would
> > want other channels, such as "very short" and "very long". Maybe the
> > description can avoid implying that all channels will appear.
>
> Shouldn't this be something that the application should control ? I'm
> increasingly thinking that this shouldn't be an HDR mode, but should
> instead be controlled through a per-frame control mechanism. Is there
> any reason this can't be done, not right now (I don't want to delay this
> patch unnecessarily), but at some point in the future ?

Yes, I'd be happy with an API for multi-channel AGC, which is what
this really is. This was actually where I started, though it seemed
somewhat controversial so I thought it might be more palatable
presented like this instead. But either way works for me.

I'm very keen to handle this inside our algorithms, so that
applications don't explicitly have to switch AGC channels all the
time. That's a more reliable approach (there will be fewer failures to
switch on every frame), and saves the application from a whole world
of pain and complication.

Though I'm all in favour of per-frame controls for lots of other
reasons. We've been very keen to see that move forward for quite a
long time now!

>
> > > > +        - name: HdrModeMultiExposure
> > > > +          value: 2
> > > > +          description: |
> > > > +            Multiple exposures will be generated and merged to create HDR
> > > > +            images. Each image will be tagged with the HDR channel (long, medium
> > > > +            or short) that arrived and which caused this image to be output.
> > > > +        - name: HdrModeSingleExposure
> > > > +          value: 3
> > > > +          description: |
> > > > +            Multiple frames all at a single exposure will be used to create HDR
> > > > +            images. These images should be reported as all corresponding to the
> > > > +            HDR short channel.
> > > > +        - name: HdrModeNight
> > > > +          value: 4
> > > > +          description: |
> > > > +            Multiple frames will be combined to produce "night mode"
> > > > +            images. Images will be tagged as belonging either to the long,
> > > > +            medium or short HDR channel according to the implementation.
> > >
> > > Does this mean that night more will always use multi-exposure, or that
> > > it is implementation-defined ?
> >
> > I really think that needs to be implementation defined. Our night mode
> > is single-exposure, but I can't possibly predict what anyone else
> > would want to do.
>
> Won't it be problematic for applications if they don't know what
> HdrChannel values they will get ? How do you expect HdrChannel to be
> used in the HDR modes where the camera combines images (all but the
> Unmerged mode) ?

Really, what I'm hoping is that the IQ stability control will indicate
when it's OK to capture the frame, and this will be set when all the
exposures that the implementation wants to use have arrived. So at
that point the channel information will be of interest to some folks,
but not essential to general applications. But obviously the IQ
stability control discussion needs to progress.

>
> > > > +
> > > > +  - 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). An application can monitor
> > > > +        the HDR channel to discover when the differently exposed images have
> > > > +        arrived.
> > > > +
> > > > +      enum:
> > > > +        - name: HdrChannelNone
> > > > +          value: 0
> > > > +          description: |
> > > > +            This image does not correspond to any of the captures used to create
> > > > +            an HDR image.
> > >
> > > As indicated above, do we need this, or should we not report HdrChannel
> > > when HDR is disabled ?
> >
> > Actually I'd quite like to keep this, even if it's not reported when HDR is off.
> >
> > One use case is multi-exposure HDR on a Pi 4. You can't merge images
> > so how would you get a viewfinder? You could intersperse some
> > "ordinary AGC" frames with your long/short/whatever frames. You might
> > want to label these "HdrChannelNone". I suppose you could label them
> > "medium", but then maybe you're using "medium" for other frames that
> > your HDR implementation requires. Like everything else, it all falls a
> > bit into the "who knows what anyone will do" category.
>
> This makes me believe even more strongly that the unmerged mode
> shouldn't be an HDR mode. We're trying to cram too many assumptions
> about the application needs here.
>
> If you want to keep HdrChannelNone for the time being until we drop
> HdrMultiExposureUnmerged, then this needs better documentation. Until I
> read your reply I had no idea that the unmerged mode would produce
> images for the "None" channel. If this isn't documented properly, it
> won't be usable for applications.
>
> Depending on the priorities (I assume the Pi 5 HDR modes will be more
> interesting than the unmerged operation on Pi 4), we could also drop the
> unmerged mode for now, and design a solution better for application-side
> HDR.

I'm definitely very keen to move forward all the discussions on
multi-channel AGC, IQ stability, per-frame controls and HDR. I am
aware that I want to be very flexible about many aspects of HDR, and I
explicitly do not want to tie our users down with "it will work this
way", I want them to be able to implement the strategies that work
best for them. So maybe vendor-specific controls are another topic
that needs to be added to this list.

For the moment, however, Pi 5 is upon us and realistically, I think we
have no ability to change anything at this point. I'm definitely open
to suggestions on how to rationalise things after our release.

David

>
> > I guess there's also an outside chance that some implementations can't
> > flick exposures instantaneously and accurately like we can, so maybe
> > it would suit them too. Or we could have an
> > "HdrChannelCouldntMakeThisWorkProperly" value? Having wrestled with
> > all this for a while I can see the attraction! :)
> >
> > I'll send out another version today with the changes in line with all
> > the above. Obviously everyone please shout if we want to do anything
> > different.
> >
> > > > +        - 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
> > > >
>
> --
> Regards,
>
> Laurent Pinchart

Hi David,

On Fri, Oct 20, 2023 at 12:09:36PM +0100, David Plowman wrote:
> On Fri, 20 Oct 2023 at 10:55, Laurent Pinchart wrote:
> > On Fri, Oct 20, 2023 at 09:37:28AM +0100, David Plowman wrote:
> > > On Thu, 19 Oct 2023 at 21:52, Laurent Pinchart wrote:
> > > > On Thu, Oct 19, 2023 at 12:52:20PM +0100, David Plowman via libcamera-devel wrote:
> > > > > We add an HdrMode control (to enable and disable HDR processing)
> > > > > and an HdrChannel, which indicates what kind of HDR frame (short, long
> > > > > or medium) 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 | 75 ++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 75 insertions(+)
> > > > >
> > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > > > index f2e542f4..c3232abf 100644
> > > > > --- a/src/libcamera/control_ids.yaml
> > > > > +++ b/src/libcamera/control_ids.yaml
> > > > > @@ -774,6 +774,81 @@ 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. HDR techniques typically include multiple exposure, image
> > > > > +        fusion and tone mapping techniques to improve the dynamic range of the
> > > > > +        resulting images.
> > > > > +
> > > > > +        When using an HDR mode, images are tagged to indicate which HDR channel
> > > > > +        (long, medium or short) they come from.
> > > > > +
> > > > > +        \sa HdrChannel
> > > > > +
> > > > > +      enum:
> > > > > +        - name: HdrModeOff
> > > > > +          value: 0
> > > > > +          description: |
> > > > > +            HDR is disabled. The HDR channel, if present, will report
> > > > > +            HdrChannelNone.
> > > >
> > > > Stating what HDR channel is used is an improvement compared to the
> > > > previous version, but there's still an option left to implementors here:
> > > > reporting HdrChannelNone, or not reporting HdrChannel at all. Unless you
> > > > see a reason to allow both, I would pick the latter:
> > > >
> > > >             HDR is disabled. Metadata for this frame will not include the
> > > >             HdrChannel control.
> > >
> > > Yes, I think that's fine.
> > >
> > > > > +        - 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. Each image will be tagged with the
> > > > > +            correct HDR channel, indicating what kind of exposure (long, medium
> > > > > +            or short) it is.  The expectation is that, if necessary, the
> > > > > +            application can merge them to create HDR images for itself.
> > > >
> > > > You mention here long, medium and short. Does this mean there will
> > > > always be three channels ?
> > >
> > > No - it's whatever the implementation wants to do. We don't use
> > > medium, for example. I think it's quite likely that some vendors would
> > > want other channels, such as "very short" and "very long". Maybe the
> > > description can avoid implying that all channels will appear.
> >
> > Shouldn't this be something that the application should control ? I'm
> > increasingly thinking that this shouldn't be an HDR mode, but should
> > instead be controlled through a per-frame control mechanism. Is there
> > any reason this can't be done, not right now (I don't want to delay this
> > patch unnecessarily), but at some point in the future ?
> 
> Yes, I'd be happy with an API for multi-channel AGC, which is what
> this really is. This was actually where I started, though it seemed
> somewhat controversial so I thought it might be more palatable
> presented like this instead. But either way works for me.
>
> I'm very keen to handle this inside our algorithms, so that
> applications don't explicitly have to switch AGC channels all the
> time. That's a more reliable approach (there will be fewer failures to
> switch on every frame), and saves the application from a whole world
> of pain and complication.
>
> Though I'm all in favour of per-frame controls for lots of other
> reasons. We've been very keen to see that move forward for quite a
> long time now!

Both make sense. I'm thinking that we may need some kind of "controls
scheduling" API to help applications alternate between different
settings, in addition to the full manual mode offered by per-frame
controls. I don't know yet how that would look like though.

> > > > > +        - name: HdrModeMultiExposure
> > > > > +          value: 2
> > > > > +          description: |
> > > > > +            Multiple exposures will be generated and merged to create HDR
> > > > > +            images. Each image will be tagged with the HDR channel (long, medium
> > > > > +            or short) that arrived and which caused this image to be output.
> > > > > +        - name: HdrModeSingleExposure
> > > > > +          value: 3
> > > > > +          description: |
> > > > > +            Multiple frames all at a single exposure will be used to create HDR
> > > > > +            images. These images should be reported as all corresponding to the
> > > > > +            HDR short channel.
> > > > > +        - name: HdrModeNight
> > > > > +          value: 4
> > > > > +          description: |
> > > > > +            Multiple frames will be combined to produce "night mode"
> > > > > +            images. Images will be tagged as belonging either to the long,
> > > > > +            medium or short HDR channel according to the implementation.
> > > >
> > > > Does this mean that night more will always use multi-exposure, or that
> > > > it is implementation-defined ?
> > >
> > > I really think that needs to be implementation defined. Our night mode
> > > is single-exposure, but I can't possibly predict what anyone else
> > > would want to do.
> >
> > Won't it be problematic for applications if they don't know what
> > HdrChannel values they will get ? How do you expect HdrChannel to be
> > used in the HDR modes where the camera combines images (all but the
> > Unmerged mode) ?
> 
> Really, what I'm hoping is that the IQ stability control will indicate
> when it's OK to capture the frame, and this will be set when all the
> exposures that the implementation wants to use have arrived. So at
> that point the channel information will be of interest to some folks,
> but not essential to general applications. But obviously the IQ
> stability control discussion needs to progress.

Those are two different issues, aren't they ? The point I was trying to
make is that I think the HdrChannel metadata will not be useful for
applications if we don't standardize at least a bit what it will report.
Uou gave an example where there would be a HdrChannelNone once per
"group" of exposures, to be used as a viewfinder. If applications need
that, they need to be able to rely on it being present, or at least know
if it will be, or have a way to select it.

What I'm missing here is a design, it seems to be more of a prototype to
see what will happen. It's fine to prototype things, but I would also
like to think about a longer term design. At the very least, how can we
get feedback on what will be useful to users, and when will we revisit
this ?

> > > > > +
> > > > > +  - 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). An application can monitor
> > > > > +        the HDR channel to discover when the differently exposed images have
> > > > > +        arrived.
> > > > > +
> > > > > +      enum:
> > > > > +        - name: HdrChannelNone
> > > > > +          value: 0
> > > > > +          description: |
> > > > > +            This image does not correspond to any of the captures used to create
> > > > > +            an HDR image.
> > > >
> > > > As indicated above, do we need this, or should we not report HdrChannel
> > > > when HDR is disabled ?
> > >
> > > Actually I'd quite like to keep this, even if it's not reported when HDR is off.
> > >
> > > One use case is multi-exposure HDR on a Pi 4. You can't merge images
> > > so how would you get a viewfinder? You could intersperse some
> > > "ordinary AGC" frames with your long/short/whatever frames. You might
> > > want to label these "HdrChannelNone". I suppose you could label them
> > > "medium", but then maybe you're using "medium" for other frames that
> > > your HDR implementation requires. Like everything else, it all falls a
> > > bit into the "who knows what anyone will do" category.
> >
> > This makes me believe even more strongly that the unmerged mode
> > shouldn't be an HDR mode. We're trying to cram too many assumptions
> > about the application needs here.
> >
> > If you want to keep HdrChannelNone for the time being until we drop
> > HdrMultiExposureUnmerged, then this needs better documentation. Until I
> > read your reply I had no idea that the unmerged mode would produce
> > images for the "None" channel. If this isn't documented properly, it
> > won't be usable for applications.
> >
> > Depending on the priorities (I assume the Pi 5 HDR modes will be more
> > interesting than the unmerged operation on Pi 4), we could also drop the
> > unmerged mode for now, and design a solution better for application-side
> > HDR.
> 
> I'm definitely very keen to move forward all the discussions on
> multi-channel AGC, IQ stability, per-frame controls and HDR. I am
> aware that I want to be very flexible about many aspects of HDR, and I
> explicitly do not want to tie our users down with "it will work this
> way", I want them to be able to implement the strategies that work
> best for them. So maybe vendor-specific controls are another topic
> that needs to be added to this list.
> 
> For the moment, however, Pi 5 is upon us and realistically, I think we
> have no ability to change anything at this point. I'm definitely open
> to suggestions on how to rationalise things after our release.

Would you prefer focussing on Pi 5 first and postponing
HdrModeMultiExposureUnmerged, or bundling them all together ? If your
focus is in Pi 5 and you won't have time to release an implementation of
HdrModeMultiExposureUnmerged with corresponding application-side support
in the short term, maybe it would be better to merge the camera-side HDR
support right now, and revisit HdrModeMultiExposureUnmerged when you'll
have time to work on its implementation ? To be perfectly clear here, I
think HdrModeMultiExposureUnmerged is less well-defined than the other
modes, and thus needs more work, but I'm also not opposed to merging it
early if we can come up with a plan to then improve it.

> > > I guess there's also an outside chance that some implementations can't
> > > flick exposures instantaneously and accurately like we can, so maybe
> > > it would suit them too. Or we could have an
> > > "HdrChannelCouldntMakeThisWorkProperly" value? Having wrestled with
> > > all this for a while I can see the attraction! :)
> > >
> > > I'll send out another version today with the changes in line with all
> > > the above. Obviously everyone please shout if we want to do anything
> > > different.
> > >
> > > > > +        - 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
> > > > >

Hi Laurent


On Fri, 20 Oct 2023 at 12:26, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> On Fri, Oct 20, 2023 at 12:09:36PM +0100, David Plowman wrote:
> > On Fri, 20 Oct 2023 at 10:55, Laurent Pinchart wrote:
> > > On Fri, Oct 20, 2023 at 09:37:28AM +0100, David Plowman wrote:
> > > > On Thu, 19 Oct 2023 at 21:52, Laurent Pinchart wrote:
> > > > > On Thu, Oct 19, 2023 at 12:52:20PM +0100, David Plowman via libcamera-devel wrote:
> > > > > > We add an HdrMode control (to enable and disable HDR processing)
> > > > > > and an HdrChannel, which indicates what kind of HDR frame (short, long
> > > > > > or medium) 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 | 75 ++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 75 insertions(+)
> > > > > >
> > > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > > > > index f2e542f4..c3232abf 100644
> > > > > > --- a/src/libcamera/control_ids.yaml
> > > > > > +++ b/src/libcamera/control_ids.yaml
> > > > > > @@ -774,6 +774,81 @@ 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. HDR techniques typically include multiple exposure, image
> > > > > > +        fusion and tone mapping techniques to improve the dynamic range of the
> > > > > > +        resulting images.
> > > > > > +
> > > > > > +        When using an HDR mode, images are tagged to indicate which HDR channel
> > > > > > +        (long, medium or short) they come from.
> > > > > > +
> > > > > > +        \sa HdrChannel
> > > > > > +
> > > > > > +      enum:
> > > > > > +        - name: HdrModeOff
> > > > > > +          value: 0
> > > > > > +          description: |
> > > > > > +            HDR is disabled. The HDR channel, if present, will report
> > > > > > +            HdrChannelNone.
> > > > >
> > > > > Stating what HDR channel is used is an improvement compared to the
> > > > > previous version, but there's still an option left to implementors here:
> > > > > reporting HdrChannelNone, or not reporting HdrChannel at all. Unless you
> > > > > see a reason to allow both, I would pick the latter:
> > > > >
> > > > >             HDR is disabled. Metadata for this frame will not include the
> > > > >             HdrChannel control.
> > > >
> > > > Yes, I think that's fine.
> > > >
> > > > > > +        - 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. Each image will be tagged with the
> > > > > > +            correct HDR channel, indicating what kind of exposure (long, medium
> > > > > > +            or short) it is.  The expectation is that, if necessary, the
> > > > > > +            application can merge them to create HDR images for itself.
> > > > >
> > > > > You mention here long, medium and short. Does this mean there will
> > > > > always be three channels ?
> > > >
> > > > No - it's whatever the implementation wants to do. We don't use
> > > > medium, for example. I think it's quite likely that some vendors would
> > > > want other channels, such as "very short" and "very long". Maybe the
> > > > description can avoid implying that all channels will appear.
> > >
> > > Shouldn't this be something that the application should control ? I'm
> > > increasingly thinking that this shouldn't be an HDR mode, but should
> > > instead be controlled through a per-frame control mechanism. Is there
> > > any reason this can't be done, not right now (I don't want to delay this
> > > patch unnecessarily), but at some point in the future ?
> >
> > Yes, I'd be happy with an API for multi-channel AGC, which is what
> > this really is. This was actually where I started, though it seemed
> > somewhat controversial so I thought it might be more palatable
> > presented like this instead. But either way works for me.
> >
> > I'm very keen to handle this inside our algorithms, so that
> > applications don't explicitly have to switch AGC channels all the
> > time. That's a more reliable approach (there will be fewer failures to
> > switch on every frame), and saves the application from a whole world
> > of pain and complication.
> >
> > Though I'm all in favour of per-frame controls for lots of other
> > reasons. We've been very keen to see that move forward for quite a
> > long time now!
>
> Both make sense. I'm thinking that we may need some kind of "controls
> scheduling" API to help applications alternate between different
> settings, in addition to the full manual mode offered by per-frame
> controls. I don't know yet how that would look like though.

Control scheduling is an interesting idea. I'm not sure how it relates
to AGC because AGC/AE controls are always a bit special because of the
frame delays that are incurred in the sensor. Controls also tend to
wind up at the back of the request queue which is another debatable
feature for us, and not a place we'd be wanting exposure/gain updates
to languish. So certainly things to think about.

>
> > > > > > +        - name: HdrModeMultiExposure
> > > > > > +          value: 2
> > > > > > +          description: |
> > > > > > +            Multiple exposures will be generated and merged to create HDR
> > > > > > +            images. Each image will be tagged with the HDR channel (long, medium
> > > > > > +            or short) that arrived and which caused this image to be output.
> > > > > > +        - name: HdrModeSingleExposure
> > > > > > +          value: 3
> > > > > > +          description: |
> > > > > > +            Multiple frames all at a single exposure will be used to create HDR
> > > > > > +            images. These images should be reported as all corresponding to the
> > > > > > +            HDR short channel.
> > > > > > +        - name: HdrModeNight
> > > > > > +          value: 4
> > > > > > +          description: |
> > > > > > +            Multiple frames will be combined to produce "night mode"
> > > > > > +            images. Images will be tagged as belonging either to the long,
> > > > > > +            medium or short HDR channel according to the implementation.
> > > > >
> > > > > Does this mean that night more will always use multi-exposure, or that
> > > > > it is implementation-defined ?
> > > >
> > > > I really think that needs to be implementation defined. Our night mode
> > > > is single-exposure, but I can't possibly predict what anyone else
> > > > would want to do.
> > >
> > > Won't it be problematic for applications if they don't know what
> > > HdrChannel values they will get ? How do you expect HdrChannel to be
> > > used in the HDR modes where the camera combines images (all but the
> > > Unmerged mode) ?
> >
> > Really, what I'm hoping is that the IQ stability control will indicate
> > when it's OK to capture the frame, and this will be set when all the
> > exposures that the implementation wants to use have arrived. So at
> > that point the channel information will be of interest to some folks,
> > but not essential to general applications. But obviously the IQ
> > stability control discussion needs to progress.
>
> Those are two different issues, aren't they ? The point I was trying to
> make is that I think the HdrChannel metadata will not be useful for
> applications if we don't standardize at least a bit what it will report.
> Uou gave an example where there would be a HdrChannelNone once per
> "group" of exposures, to be used as a viewfinder. If applications need
> that, they need to be able to rely on it being present, or at least know
> if it will be, or have a way to select it.

I agree standardisation is good here. I also think that beyond some
obvious cases (long/medium/short) it will be difficult. I'm already
expecting "very short" and "very long" at some point. I see a need for
"this isn't HDR at all but is for preview", maybe that's just "this is
a normal AGC image". I think some platforms might just want a long run
of exposures where some might be the same, some might be different,
and the only way to name them will be "0", "1" and so on.

We've had this problem to some extent elsewhere and have adopted a
"custom" value. But it's not a great solution, and I don't think it
really flies here when there could be many "custom" values.

>
> What I'm missing here is a design, it seems to be more of a prototype to
> see what will happen. It's fine to prototype things, but I would also
> like to think about a longer term design. At the very least, how can we
> get feedback on what will be useful to users, and when will we revisit
> this ?

I wouldn't disagree with any of that, though I think experience tells
us that the only real way to get feedback is to put stuff out there
and see how people react. We aren't short of wider camera forums where
getting meaningful engagement from other parties is difficult! To be
fair, Pi users are very good in this respect, though I think most
would engage more actively when there's stuff to play with. But hard
to say!

>
> > > > > > +
> > > > > > +  - 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). An application can monitor
> > > > > > +        the HDR channel to discover when the differently exposed images have
> > > > > > +        arrived.
> > > > > > +
> > > > > > +      enum:
> > > > > > +        - name: HdrChannelNone
> > > > > > +          value: 0
> > > > > > +          description: |
> > > > > > +            This image does not correspond to any of the captures used to create
> > > > > > +            an HDR image.
> > > > >
> > > > > As indicated above, do we need this, or should we not report HdrChannel
> > > > > when HDR is disabled ?
> > > >
> > > > Actually I'd quite like to keep this, even if it's not reported when HDR is off.
> > > >
> > > > One use case is multi-exposure HDR on a Pi 4. You can't merge images
> > > > so how would you get a viewfinder? You could intersperse some
> > > > "ordinary AGC" frames with your long/short/whatever frames. You might
> > > > want to label these "HdrChannelNone". I suppose you could label them
> > > > "medium", but then maybe you're using "medium" for other frames that
> > > > your HDR implementation requires. Like everything else, it all falls a
> > > > bit into the "who knows what anyone will do" category.
> > >
> > > This makes me believe even more strongly that the unmerged mode
> > > shouldn't be an HDR mode. We're trying to cram too many assumptions
> > > about the application needs here.
> > >
> > > If you want to keep HdrChannelNone for the time being until we drop
> > > HdrMultiExposureUnmerged, then this needs better documentation. Until I
> > > read your reply I had no idea that the unmerged mode would produce
> > > images for the "None" channel. If this isn't documented properly, it
> > > won't be usable for applications.
> > >
> > > Depending on the priorities (I assume the Pi 5 HDR modes will be more
> > > interesting than the unmerged operation on Pi 4), we could also drop the
> > > unmerged mode for now, and design a solution better for application-side
> > > HDR.
> >
> > I'm definitely very keen to move forward all the discussions on
> > multi-channel AGC, IQ stability, per-frame controls and HDR. I am
> > aware that I want to be very flexible about many aspects of HDR, and I
> > explicitly do not want to tie our users down with "it will work this
> > way", I want them to be able to implement the strategies that work
> > best for them. So maybe vendor-specific controls are another topic
> > that needs to be added to this list.
> >
> > For the moment, however, Pi 5 is upon us and realistically, I think we
> > have no ability to change anything at this point. I'm definitely open
> > to suggestions on how to rationalise things after our release.
>
> Would you prefer focussing on Pi 5 first and postponing
> HdrModeMultiExposureUnmerged, or bundling them all together ? If your
> focus is in Pi 5 and you won't have time to release an implementation of
> HdrModeMultiExposureUnmerged with corresponding application-side support
> in the short term, maybe it would be better to merge the camera-side HDR
> support right now, and revisit HdrModeMultiExposureUnmerged when you'll
> have time to work on its implementation ? To be perfectly clear here, I
> think HdrModeMultiExposureUnmerged is less well-defined than the other
> modes, and thus needs more work, but I'm also not opposed to merging it
> early if we can come up with a plan to then improve it.

For us, the release we're making in a day or two is a fait accompli at
this point (Pi 5s are being shipped, I hear), and all this stuff is in
it. All these modes are implemented and work. So from our point of
view, bundling everything that we've done together would minimse
divergence. Though we can live with it too, at least temporarily.

But as I said, I'm more than happy to revisit stuff. I think
multi-channel AGC is probably a better answer than "unmerged" HDR, but
we need to agree an approach.

I think many platforms would react with horror to multi-channel AGC.
They'd take the view that they'll just drive multiple different
exposures explicitly, and not worry about multiple channels. Though
you still have to decide if they need to adjust dynamically and how
you switch between them, how they don't get too far apart, and how to
make it jump from one exposure to another without adapting slowly. So
it's all the same problems, just without exposing a general solution
that enables it. But it's definitely more viable if you're only doing
still image HDR. I can't escape wondering whether the dawn of vendor
extensions is drawing closer.

So lots to think about!

David

>
> > > > I guess there's also an outside chance that some implementations can't
> > > > flick exposures instantaneously and accurately like we can, so maybe
> > > > it would suit them too. Or we could have an
> > > > "HdrChannelCouldntMakeThisWorkProperly" value? Having wrestled with
> > > > all this for a while I can see the attraction! :)
> > > >
> > > > I'll send out another version today with the changes in line with all
> > > > the above. Obviously everyone please shout if we want to do anything
> > > > different.
> > > >
> > > > > > +        - 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
> > > > > >
>
> --
> Regards,
>
> Laurent Pinchart

Hi David,

On Fri, Oct 20, 2023 at 01:11:04PM +0100, David Plowman wrote:
> On Fri, 20 Oct 2023 at 12:26, Laurent Pinchart wrote:
> > On Fri, Oct 20, 2023 at 12:09:36PM +0100, David Plowman wrote:
> > > On Fri, 20 Oct 2023 at 10:55, Laurent Pinchart wrote:
> > > > On Fri, Oct 20, 2023 at 09:37:28AM +0100, David Plowman wrote:
> > > > > On Thu, 19 Oct 2023 at 21:52, Laurent Pinchart wrote:
> > > > > > On Thu, Oct 19, 2023 at 12:52:20PM +0100, David Plowman via libcamera-devel wrote:
> > > > > > > We add an HdrMode control (to enable and disable HDR processing)
> > > > > > > and an HdrChannel, which indicates what kind of HDR frame (short, long
> > > > > > > or medium) 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 | 75 ++++++++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 75 insertions(+)
> > > > > > >
> > > > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > > > > > index f2e542f4..c3232abf 100644
> > > > > > > --- a/src/libcamera/control_ids.yaml
> > > > > > > +++ b/src/libcamera/control_ids.yaml
> > > > > > > @@ -774,6 +774,81 @@ 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. HDR techniques typically include multiple exposure, image
> > > > > > > +        fusion and tone mapping techniques to improve the dynamic range of the
> > > > > > > +        resulting images.
> > > > > > > +
> > > > > > > +        When using an HDR mode, images are tagged to indicate which HDR channel
> > > > > > > +        (long, medium or short) they come from.
> > > > > > > +
> > > > > > > +        \sa HdrChannel
> > > > > > > +
> > > > > > > +      enum:
> > > > > > > +        - name: HdrModeOff
> > > > > > > +          value: 0
> > > > > > > +          description: |
> > > > > > > +            HDR is disabled. The HDR channel, if present, will report
> > > > > > > +            HdrChannelNone.
> > > > > >
> > > > > > Stating what HDR channel is used is an improvement compared to the
> > > > > > previous version, but there's still an option left to implementors here:
> > > > > > reporting HdrChannelNone, or not reporting HdrChannel at all. Unless you
> > > > > > see a reason to allow both, I would pick the latter:
> > > > > >
> > > > > >             HDR is disabled. Metadata for this frame will not include the
> > > > > >             HdrChannel control.
> > > > >
> > > > > Yes, I think that's fine.
> > > > >
> > > > > > > +        - 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. Each image will be tagged with the
> > > > > > > +            correct HDR channel, indicating what kind of exposure (long, medium
> > > > > > > +            or short) it is.  The expectation is that, if necessary, the
> > > > > > > +            application can merge them to create HDR images for itself.
> > > > > >
> > > > > > You mention here long, medium and short. Does this mean there will
> > > > > > always be three channels ?
> > > > >
> > > > > No - it's whatever the implementation wants to do. We don't use
> > > > > medium, for example. I think it's quite likely that some vendors would
> > > > > want other channels, such as "very short" and "very long". Maybe the
> > > > > description can avoid implying that all channels will appear.
> > > >
> > > > Shouldn't this be something that the application should control ? I'm
> > > > increasingly thinking that this shouldn't be an HDR mode, but should
> > > > instead be controlled through a per-frame control mechanism. Is there
> > > > any reason this can't be done, not right now (I don't want to delay this
> > > > patch unnecessarily), but at some point in the future ?
> > >
> > > Yes, I'd be happy with an API for multi-channel AGC, which is what
> > > this really is. This was actually where I started, though it seemed
> > > somewhat controversial so I thought it might be more palatable
> > > presented like this instead. But either way works for me.
> > >
> > > I'm very keen to handle this inside our algorithms, so that
> > > applications don't explicitly have to switch AGC channels all the
> > > time. That's a more reliable approach (there will be fewer failures to
> > > switch on every frame), and saves the application from a whole world
> > > of pain and complication.
> > >
> > > Though I'm all in favour of per-frame controls for lots of other
> > > reasons. We've been very keen to see that move forward for quite a
> > > long time now!
> >
> > Both make sense. I'm thinking that we may need some kind of "controls
> > scheduling" API to help applications alternate between different
> > settings, in addition to the full manual mode offered by per-frame
> > controls. I don't know yet how that would look like though.
> 
> Control scheduling is an interesting idea. I'm not sure how it relates
> to AGC because AGC/AE controls are always a bit special because of the
> frame delays that are incurred in the sensor. Controls also tend to
> wind up at the back of the request queue which is another debatable
> feature for us, and not a place we'd be wanting exposure/gain updates
> to languish. So certainly things to think about.

Another point to keep in mind is that, while application-side HDR with
staggered exposures in its simplest form from a libcamera point of view
would see the application specify per-frame manual exposures, I think we
need to provide a way to still run the libcamera-side AEC/AGC. Maybe
setting the ExposureValue per frame would be such a way, to enable
generation of darker and lighter frames based on the "normal" exposure
calculated by the AEC/AGC algorithm. Maybe we will also need more/other
controls for this.

> > > > > > > +        - name: HdrModeMultiExposure
> > > > > > > +          value: 2
> > > > > > > +          description: |
> > > > > > > +            Multiple exposures will be generated and merged to create HDR
> > > > > > > +            images. Each image will be tagged with the HDR channel (long, medium
> > > > > > > +            or short) that arrived and which caused this image to be output.
> > > > > > > +        - name: HdrModeSingleExposure
> > > > > > > +          value: 3
> > > > > > > +          description: |
> > > > > > > +            Multiple frames all at a single exposure will be used to create HDR
> > > > > > > +            images. These images should be reported as all corresponding to the
> > > > > > > +            HDR short channel.
> > > > > > > +        - name: HdrModeNight
> > > > > > > +          value: 4
> > > > > > > +          description: |
> > > > > > > +            Multiple frames will be combined to produce "night mode"
> > > > > > > +            images. Images will be tagged as belonging either to the long,
> > > > > > > +            medium or short HDR channel according to the implementation.
> > > > > >
> > > > > > Does this mean that night more will always use multi-exposure, or that
> > > > > > it is implementation-defined ?
> > > > >
> > > > > I really think that needs to be implementation defined. Our night mode
> > > > > is single-exposure, but I can't possibly predict what anyone else
> > > > > would want to do.
> > > >
> > > > Won't it be problematic for applications if they don't know what
> > > > HdrChannel values they will get ? How do you expect HdrChannel to be
> > > > used in the HDR modes where the camera combines images (all but the
> > > > Unmerged mode) ?
> > >
> > > Really, what I'm hoping is that the IQ stability control will indicate
> > > when it's OK to capture the frame, and this will be set when all the
> > > exposures that the implementation wants to use have arrived. So at
> > > that point the channel information will be of interest to some folks,
> > > but not essential to general applications. But obviously the IQ
> > > stability control discussion needs to progress.
> >
> > Those are two different issues, aren't they ? The point I was trying to
> > make is that I think the HdrChannel metadata will not be useful for
> > applications if we don't standardize at least a bit what it will report.
> > Uou gave an example where there would be a HdrChannelNone once per
> > "group" of exposures, to be used as a viewfinder. If applications need
> > that, they need to be able to rely on it being present, or at least know
> > if it will be, or have a way to select it.
> 
> I agree standardisation is good here. I also think that beyond some
> obvious cases (long/medium/short) it will be difficult. I'm already
> expecting "very short" and "very long" at some point. I see a need for
> "this isn't HDR at all but is for preview", maybe that's just "this is
> a normal AGC image". I think some platforms might just want a long run
> of exposures where some might be the same, some might be different,
> and the only way to name them will be "0", "1" and so on.

Could we at least decide on basic rules that govern what channels should
be used, depending on the number of channels ? For instance, if we were
to define 5 channels (very short, short, medium, long, very long), and a
camera used 3 channels, it would be horrible to ask applications
developers to be prepared to handle all possible combinations. Neither
would it make much sense, in my opinion, to allow cameras to use "very
short, long, very long" in that case. The exposures are relative to each
other, there's no rule to govern the delta between them, so I think we
can at least set some rules on the ordering.

For the single-exposure case, does it even make sense to report a
channel, given that a single one is used ?

For the multi-exposure cases, can we tell which channels will be used
based on the number of channels ? If you want some images to be reported
with channel "None", can we document this, and explain what those images
will be ?

> We've had this problem to some extent elsewhere and have adopted a
> "custom" value. But it's not a great solution, and I don't think it
> really flies here when there could be many "custom" values.
> 
> > What I'm missing here is a design, it seems to be more of a prototype to
> > see what will happen. It's fine to prototype things, but I would also
> > like to think about a longer term design. At the very least, how can we
> > get feedback on what will be useful to users, and when will we revisit
> > this ?
> 
> I wouldn't disagree with any of that, though I think experience tells
> us that the only real way to get feedback is to put stuff out there
> and see how people react. We aren't short of wider camera forums where
> getting meaningful engagement from other parties is difficult! To be
> fair, Pi users are very good in this respect, though I think most
> would engage more actively when there's stuff to play with. But hard
> to say!

I agree, getting it in the hands of users is certainly a way to get
feedback (how constructive that feedback is is a different question
:-)).

> > > > > > > +
> > > > > > > +  - 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). An application can monitor
> > > > > > > +        the HDR channel to discover when the differently exposed images have
> > > > > > > +        arrived.
> > > > > > > +
> > > > > > > +      enum:
> > > > > > > +        - name: HdrChannelNone
> > > > > > > +          value: 0
> > > > > > > +          description: |
> > > > > > > +            This image does not correspond to any of the captures used to create
> > > > > > > +            an HDR image.
> > > > > >
> > > > > > As indicated above, do we need this, or should we not report HdrChannel
> > > > > > when HDR is disabled ?
> > > > >
> > > > > Actually I'd quite like to keep this, even if it's not reported when HDR is off.
> > > > >
> > > > > One use case is multi-exposure HDR on a Pi 4. You can't merge images
> > > > > so how would you get a viewfinder? You could intersperse some
> > > > > "ordinary AGC" frames with your long/short/whatever frames. You might
> > > > > want to label these "HdrChannelNone". I suppose you could label them
> > > > > "medium", but then maybe you're using "medium" for other frames that
> > > > > your HDR implementation requires. Like everything else, it all falls a
> > > > > bit into the "who knows what anyone will do" category.
> > > >
> > > > This makes me believe even more strongly that the unmerged mode
> > > > shouldn't be an HDR mode. We're trying to cram too many assumptions
> > > > about the application needs here.
> > > >
> > > > If you want to keep HdrChannelNone for the time being until we drop
> > > > HdrMultiExposureUnmerged, then this needs better documentation. Until I
> > > > read your reply I had no idea that the unmerged mode would produce
> > > > images for the "None" channel. If this isn't documented properly, it
> > > > won't be usable for applications.
> > > >
> > > > Depending on the priorities (I assume the Pi 5 HDR modes will be more
> > > > interesting than the unmerged operation on Pi 4), we could also drop the
> > > > unmerged mode for now, and design a solution better for application-side
> > > > HDR.
> > >
> > > I'm definitely very keen to move forward all the discussions on
> > > multi-channel AGC, IQ stability, per-frame controls and HDR. I am
> > > aware that I want to be very flexible about many aspects of HDR, and I
> > > explicitly do not want to tie our users down with "it will work this
> > > way", I want them to be able to implement the strategies that work
> > > best for them. So maybe vendor-specific controls are another topic
> > > that needs to be added to this list.
> > >
> > > For the moment, however, Pi 5 is upon us and realistically, I think we
> > > have no ability to change anything at this point. I'm definitely open
> > > to suggestions on how to rationalise things after our release.
> >
> > Would you prefer focussing on Pi 5 first and postponing
> > HdrModeMultiExposureUnmerged, or bundling them all together ? If your
> > focus is in Pi 5 and you won't have time to release an implementation of
> > HdrModeMultiExposureUnmerged with corresponding application-side support
> > in the short term, maybe it would be better to merge the camera-side HDR
> > support right now, and revisit HdrModeMultiExposureUnmerged when you'll
> > have time to work on its implementation ? To be perfectly clear here, I
> > think HdrModeMultiExposureUnmerged is less well-defined than the other
> > modes, and thus needs more work, but I'm also not opposed to merging it
> > early if we can come up with a plan to then improve it.
> 
> For us, the release we're making in a day or two is a fait accompli at
> this point (Pi 5s are being shipped, I hear), and all this stuff is in
> it. All these modes are implemented and work. So from our point of
> view, bundling everything that we've done together would minimse
> divergence. Though we can live with it too, at least temporarily.

OK, fine with me.

> But as I said, I'm more than happy to revisit stuff. I think
> multi-channel AGC is probably a better answer than "unmerged" HDR, but
> we need to agree an approach.
> 
> I think many platforms would react with horror to multi-channel AGC.
> They'd take the view that they'll just drive multiple different
> exposures explicitly, and not worry about multiple channels. Though
> you still have to decide if they need to adjust dynamically and how
> you switch between them, how they don't get too far apart, and how to
> make it jump from one exposure to another without adapting slowly. So
> it's all the same problems, just without exposing a general solution
> that enables it. But it's definitely more viable if you're only doing
> still image HDR. I can't escape wondering whether the dawn of vendor
> extensions is drawing closer.

It's always drawing closer, but I think we're still far away from it for
HDR specifically. This is just the starting point.

> So lots to think about!
> 
> > > > > I guess there's also an outside chance that some implementations can't
> > > > > flick exposures instantaneously and accurately like we can, so maybe
> > > > > it would suit them too. Or we could have an
> > > > > "HdrChannelCouldntMakeThisWorkProperly" value? Having wrestled with
> > > > > all this for a while I can see the attraction! :)
> > > > >
> > > > > I'll send out another version today with the changes in line with all
> > > > > the above. Obviously everyone please shout if we want to do anything
> > > > > different.
> > > > >
> > > > > > > +        - 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
> > > > > > >

Hi David,

To make sure we're not both waiting on each other, there are questions
for you below in my previous reply that I'd like to figure out to make
progress on this patch.

On Fri, Oct 20, 2023 at 04:18:45PM +0300, Laurent Pinchart via libcamera-devel wrote:
> On Fri, Oct 20, 2023 at 01:11:04PM +0100, David Plowman wrote:
> > On Fri, 20 Oct 2023 at 12:26, Laurent Pinchart wrote:
> > > On Fri, Oct 20, 2023 at 12:09:36PM +0100, David Plowman wrote:
> > > > On Fri, 20 Oct 2023 at 10:55, Laurent Pinchart wrote:
> > > > > On Fri, Oct 20, 2023 at 09:37:28AM +0100, David Plowman wrote:
> > > > > > On Thu, 19 Oct 2023 at 21:52, Laurent Pinchart wrote:
> > > > > > > On Thu, Oct 19, 2023 at 12:52:20PM +0100, David Plowman via libcamera-devel wrote:
> > > > > > > > We add an HdrMode control (to enable and disable HDR processing)
> > > > > > > > and an HdrChannel, which indicates what kind of HDR frame (short, long
> > > > > > > > or medium) 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 | 75 ++++++++++++++++++++++++++++++++++
> > > > > > > >  1 file changed, 75 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > > > > > > index f2e542f4..c3232abf 100644
> > > > > > > > --- a/src/libcamera/control_ids.yaml
> > > > > > > > +++ b/src/libcamera/control_ids.yaml
> > > > > > > > @@ -774,6 +774,81 @@ 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. HDR techniques typically include multiple exposure, image
> > > > > > > > +        fusion and tone mapping techniques to improve the dynamic range of the
> > > > > > > > +        resulting images.
> > > > > > > > +
> > > > > > > > +        When using an HDR mode, images are tagged to indicate which HDR channel
> > > > > > > > +        (long, medium or short) they come from.
> > > > > > > > +
> > > > > > > > +        \sa HdrChannel
> > > > > > > > +
> > > > > > > > +      enum:
> > > > > > > > +        - name: HdrModeOff
> > > > > > > > +          value: 0
> > > > > > > > +          description: |
> > > > > > > > +            HDR is disabled. The HDR channel, if present, will report
> > > > > > > > +            HdrChannelNone.
> > > > > > >
> > > > > > > Stating what HDR channel is used is an improvement compared to the
> > > > > > > previous version, but there's still an option left to implementors here:
> > > > > > > reporting HdrChannelNone, or not reporting HdrChannel at all. Unless you
> > > > > > > see a reason to allow both, I would pick the latter:
> > > > > > >
> > > > > > >             HDR is disabled. Metadata for this frame will not include the
> > > > > > >             HdrChannel control.
> > > > > >
> > > > > > Yes, I think that's fine.
> > > > > >
> > > > > > > > +        - 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. Each image will be tagged with the
> > > > > > > > +            correct HDR channel, indicating what kind of exposure (long, medium
> > > > > > > > +            or short) it is.  The expectation is that, if necessary, the
> > > > > > > > +            application can merge them to create HDR images for itself.
> > > > > > >
> > > > > > > You mention here long, medium and short. Does this mean there will
> > > > > > > always be three channels ?
> > > > > >
> > > > > > No - it's whatever the implementation wants to do. We don't use
> > > > > > medium, for example. I think it's quite likely that some vendors would
> > > > > > want other channels, such as "very short" and "very long". Maybe the
> > > > > > description can avoid implying that all channels will appear.
> > > > >
> > > > > Shouldn't this be something that the application should control ? I'm
> > > > > increasingly thinking that this shouldn't be an HDR mode, but should
> > > > > instead be controlled through a per-frame control mechanism. Is there
> > > > > any reason this can't be done, not right now (I don't want to delay this
> > > > > patch unnecessarily), but at some point in the future ?
> > > >
> > > > Yes, I'd be happy with an API for multi-channel AGC, which is what
> > > > this really is. This was actually where I started, though it seemed
> > > > somewhat controversial so I thought it might be more palatable
> > > > presented like this instead. But either way works for me.
> > > >
> > > > I'm very keen to handle this inside our algorithms, so that
> > > > applications don't explicitly have to switch AGC channels all the
> > > > time. That's a more reliable approach (there will be fewer failures to
> > > > switch on every frame), and saves the application from a whole world
> > > > of pain and complication.
> > > >
> > > > Though I'm all in favour of per-frame controls for lots of other
> > > > reasons. We've been very keen to see that move forward for quite a
> > > > long time now!
> > >
> > > Both make sense. I'm thinking that we may need some kind of "controls
> > > scheduling" API to help applications alternate between different
> > > settings, in addition to the full manual mode offered by per-frame
> > > controls. I don't know yet how that would look like though.
> > 
> > Control scheduling is an interesting idea. I'm not sure how it relates
> > to AGC because AGC/AE controls are always a bit special because of the
> > frame delays that are incurred in the sensor. Controls also tend to
> > wind up at the back of the request queue which is another debatable
> > feature for us, and not a place we'd be wanting exposure/gain updates
> > to languish. So certainly things to think about.
> 
> Another point to keep in mind is that, while application-side HDR with
> staggered exposures in its simplest form from a libcamera point of view
> would see the application specify per-frame manual exposures, I think we
> need to provide a way to still run the libcamera-side AEC/AGC. Maybe
> setting the ExposureValue per frame would be such a way, to enable
> generation of darker and lighter frames based on the "normal" exposure
> calculated by the AEC/AGC algorithm. Maybe we will also need more/other
> controls for this.
> 
> > > > > > > > +        - name: HdrModeMultiExposure
> > > > > > > > +          value: 2
> > > > > > > > +          description: |
> > > > > > > > +            Multiple exposures will be generated and merged to create HDR
> > > > > > > > +            images. Each image will be tagged with the HDR channel (long, medium
> > > > > > > > +            or short) that arrived and which caused this image to be output.
> > > > > > > > +        - name: HdrModeSingleExposure
> > > > > > > > +          value: 3
> > > > > > > > +          description: |
> > > > > > > > +            Multiple frames all at a single exposure will be used to create HDR
> > > > > > > > +            images. These images should be reported as all corresponding to the
> > > > > > > > +            HDR short channel.
> > > > > > > > +        - name: HdrModeNight
> > > > > > > > +          value: 4
> > > > > > > > +          description: |
> > > > > > > > +            Multiple frames will be combined to produce "night mode"
> > > > > > > > +            images. Images will be tagged as belonging either to the long,
> > > > > > > > +            medium or short HDR channel according to the implementation.
> > > > > > >
> > > > > > > Does this mean that night more will always use multi-exposure, or that
> > > > > > > it is implementation-defined ?
> > > > > >
> > > > > > I really think that needs to be implementation defined. Our night mode
> > > > > > is single-exposure, but I can't possibly predict what anyone else
> > > > > > would want to do.
> > > > >
> > > > > Won't it be problematic for applications if they don't know what
> > > > > HdrChannel values they will get ? How do you expect HdrChannel to be
> > > > > used in the HDR modes where the camera combines images (all but the
> > > > > Unmerged mode) ?
> > > >
> > > > Really, what I'm hoping is that the IQ stability control will indicate
> > > > when it's OK to capture the frame, and this will be set when all the
> > > > exposures that the implementation wants to use have arrived. So at
> > > > that point the channel information will be of interest to some folks,
> > > > but not essential to general applications. But obviously the IQ
> > > > stability control discussion needs to progress.
> > >
> > > Those are two different issues, aren't they ? The point I was trying to
> > > make is that I think the HdrChannel metadata will not be useful for
> > > applications if we don't standardize at least a bit what it will report.
> > > Uou gave an example where there would be a HdrChannelNone once per
> > > "group" of exposures, to be used as a viewfinder. If applications need
> > > that, they need to be able to rely on it being present, or at least know
> > > if it will be, or have a way to select it.
> > 
> > I agree standardisation is good here. I also think that beyond some
> > obvious cases (long/medium/short) it will be difficult. I'm already
> > expecting "very short" and "very long" at some point. I see a need for
> > "this isn't HDR at all but is for preview", maybe that's just "this is
> > a normal AGC image". I think some platforms might just want a long run
> > of exposures where some might be the same, some might be different,
> > and the only way to name them will be "0", "1" and so on.
> 
> Could we at least decide on basic rules that govern what channels should
> be used, depending on the number of channels ? For instance, if we were
> to define 5 channels (very short, short, medium, long, very long), and a
> camera used 3 channels, it would be horrible to ask applications
> developers to be prepared to handle all possible combinations. Neither
> would it make much sense, in my opinion, to allow cameras to use "very
> short, long, very long" in that case. The exposures are relative to each
> other, there's no rule to govern the delta between them, so I think we
> can at least set some rules on the ordering.
> 
> For the single-exposure case, does it even make sense to report a
> channel, given that a single one is used ?
> 
> For the multi-exposure cases, can we tell which channels will be used
> based on the number of channels ? If you want some images to be reported
> with channel "None", can we document this, and explain what those images
> will be ?
> 
> > We've had this problem to some extent elsewhere and have adopted a
> > "custom" value. But it's not a great solution, and I don't think it
> > really flies here when there could be many "custom" values.
> > 
> > > What I'm missing here is a design, it seems to be more of a prototype to
> > > see what will happen. It's fine to prototype things, but I would also
> > > like to think about a longer term design. At the very least, how can we
> > > get feedback on what will be useful to users, and when will we revisit
> > > this ?
> > 
> > I wouldn't disagree with any of that, though I think experience tells
> > us that the only real way to get feedback is to put stuff out there
> > and see how people react. We aren't short of wider camera forums where
> > getting meaningful engagement from other parties is difficult! To be
> > fair, Pi users are very good in this respect, though I think most
> > would engage more actively when there's stuff to play with. But hard
> > to say!
> 
> I agree, getting it in the hands of users is certainly a way to get
> feedback (how constructive that feedback is is a different question
> :-)).
> 
> > > > > > > > +
> > > > > > > > +  - 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). An application can monitor
> > > > > > > > +        the HDR channel to discover when the differently exposed images have
> > > > > > > > +        arrived.
> > > > > > > > +
> > > > > > > > +      enum:
> > > > > > > > +        - name: HdrChannelNone
> > > > > > > > +          value: 0
> > > > > > > > +          description: |
> > > > > > > > +            This image does not correspond to any of the captures used to create
> > > > > > > > +            an HDR image.
> > > > > > >
> > > > > > > As indicated above, do we need this, or should we not report HdrChannel
> > > > > > > when HDR is disabled ?
> > > > > >
> > > > > > Actually I'd quite like to keep this, even if it's not reported when HDR is off.
> > > > > >
> > > > > > One use case is multi-exposure HDR on a Pi 4. You can't merge images
> > > > > > so how would you get a viewfinder? You could intersperse some
> > > > > > "ordinary AGC" frames with your long/short/whatever frames. You might
> > > > > > want to label these "HdrChannelNone". I suppose you could label them
> > > > > > "medium", but then maybe you're using "medium" for other frames that
> > > > > > your HDR implementation requires. Like everything else, it all falls a
> > > > > > bit into the "who knows what anyone will do" category.
> > > > >
> > > > > This makes me believe even more strongly that the unmerged mode
> > > > > shouldn't be an HDR mode. We're trying to cram too many assumptions
> > > > > about the application needs here.
> > > > >
> > > > > If you want to keep HdrChannelNone for the time being until we drop
> > > > > HdrMultiExposureUnmerged, then this needs better documentation. Until I
> > > > > read your reply I had no idea that the unmerged mode would produce
> > > > > images for the "None" channel. If this isn't documented properly, it
> > > > > won't be usable for applications.
> > > > >
> > > > > Depending on the priorities (I assume the Pi 5 HDR modes will be more
> > > > > interesting than the unmerged operation on Pi 4), we could also drop the
> > > > > unmerged mode for now, and design a solution better for application-side
> > > > > HDR.
> > > >
> > > > I'm definitely very keen to move forward all the discussions on
> > > > multi-channel AGC, IQ stability, per-frame controls and HDR. I am
> > > > aware that I want to be very flexible about many aspects of HDR, and I
> > > > explicitly do not want to tie our users down with "it will work this
> > > > way", I want them to be able to implement the strategies that work
> > > > best for them. So maybe vendor-specific controls are another topic
> > > > that needs to be added to this list.
> > > >
> > > > For the moment, however, Pi 5 is upon us and realistically, I think we
> > > > have no ability to change anything at this point. I'm definitely open
> > > > to suggestions on how to rationalise things after our release.
> > >
> > > Would you prefer focussing on Pi 5 first and postponing
> > > HdrModeMultiExposureUnmerged, or bundling them all together ? If your
> > > focus is in Pi 5 and you won't have time to release an implementation of
> > > HdrModeMultiExposureUnmerged with corresponding application-side support
> > > in the short term, maybe it would be better to merge the camera-side HDR
> > > support right now, and revisit HdrModeMultiExposureUnmerged when you'll
> > > have time to work on its implementation ? To be perfectly clear here, I
> > > think HdrModeMultiExposureUnmerged is less well-defined than the other
> > > modes, and thus needs more work, but I'm also not opposed to merging it
> > > early if we can come up with a plan to then improve it.
> > 
> > For us, the release we're making in a day or two is a fait accompli at
> > this point (Pi 5s are being shipped, I hear), and all this stuff is in
> > it. All these modes are implemented and work. So from our point of
> > view, bundling everything that we've done together would minimse
> > divergence. Though we can live with it too, at least temporarily.
> 
> OK, fine with me.
> 
> > But as I said, I'm more than happy to revisit stuff. I think
> > multi-channel AGC is probably a better answer than "unmerged" HDR, but
> > we need to agree an approach.
> > 
> > I think many platforms would react with horror to multi-channel AGC.
> > They'd take the view that they'll just drive multiple different
> > exposures explicitly, and not worry about multiple channels. Though
> > you still have to decide if they need to adjust dynamically and how
> > you switch between them, how they don't get too far apart, and how to
> > make it jump from one exposure to another without adapting slowly. So
> > it's all the same problems, just without exposing a general solution
> > that enables it. But it's definitely more viable if you're only doing
> > still image HDR. I can't escape wondering whether the dawn of vendor
> > extensions is drawing closer.
> 
> It's always drawing closer, but I think we're still far away from it for
> HDR specifically. This is just the starting point.
> 
> > So lots to think about!
> > 
> > > > > > I guess there's also an outside chance that some implementations can't
> > > > > > flick exposures instantaneously and accurately like we can, so maybe
> > > > > > it would suit them too. Or we could have an
> > > > > > "HdrChannelCouldntMakeThisWorkProperly" value? Having wrestled with
> > > > > > all this for a while I can see the attraction! :)
> > > > > >
> > > > > > I'll send out another version today with the changes in line with all
> > > > > > the above. Obviously everyone please shout if we want to do anything
> > > > > > different.
> > > > > >
> > > > > > > > +        - 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
> > > > > > > >

Hi Laurent

Yes, I was meaning to answer, but also just wanted to explain a few
more general things. Perhaps these are things to consider slightly
beyond the immediate patches.

The first thing I wanted to add is that I suspect the things I want to
do could well be regarded as "platform specific". I don't think we
should be too surprised, my aim after all is to help people get all
the best features out of their Pis, even when these features may be
specific to Pis in various ways.

The other point is that I think Raspberry Pi are not "typical" camera
users. Other folks are wanting to write still or video capture
applications, whereas I'm trying to give our users a toolkit of
techniques so that they can experiment with imaging for themselves.
Inevitably, these will include Raspberry Pi techniques. This will
become trickier once we get beyond the "easy" features, like "set the
exposure".

HDR may be a case in point. I'm keen to expose the way we have
implemented HDR to our users. Whilst I've made certain decisions
("I'll meter the short channel like this...") I don't expect that to
suit all our users. So I want them to be able to change it. I want
them to be able to define their own AGC channels, for whatever
use-cases they have. Many of our users have specific and quite
technical use cases that I know nothing about and can't predict, so
things need to be flexible and configurable. To a large extent this
may be via the tuning file, but I expect there will be some external
controls necessary too.

Now, supposing we find another vendor who wants to implement HDR, I
think there could be some issues. At a high level, things might look
the same ("turn on HDR mode", "use some metadata to tell me that the
HDR images are good to capture"), but after that I'm quite doubtful. I
think any other vendor would look at what Raspberry Pi have done and
say "Yeah, no. Just no. We're using our HDR that we've spent the last
100 person-years developing, thank you very much". (And possibly make
everything proprietary and expose only the simplest API that gives
minimal control to users - but we don't know.)

Anyway, hopefully that provides a bit of the context here, and so now
let me try to comment on the more specific questions!

On Tue, 24 Oct 2023 at 09:21, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> To make sure we're not both waiting on each other, there are questions
> for you below in my previous reply that I'd like to figure out to make
> progress on this patch.
>
> On Fri, Oct 20, 2023 at 04:18:45PM +0300, Laurent Pinchart via libcamera-devel wrote:
> > On Fri, Oct 20, 2023 at 01:11:04PM +0100, David Plowman wrote:
> > > On Fri, 20 Oct 2023 at 12:26, Laurent Pinchart wrote:
> > > > On Fri, Oct 20, 2023 at 12:09:36PM +0100, David Plowman wrote:
> > > > > On Fri, 20 Oct 2023 at 10:55, Laurent Pinchart wrote:
> > > > > > On Fri, Oct 20, 2023 at 09:37:28AM +0100, David Plowman wrote:
> > > > > > > On Thu, 19 Oct 2023 at 21:52, Laurent Pinchart wrote:
> > > > > > > > On Thu, Oct 19, 2023 at 12:52:20PM +0100, David Plowman via libcamera-devel wrote:
> > > > > > > > > We add an HdrMode control (to enable and disable HDR processing)
> > > > > > > > > and an HdrChannel, which indicates what kind of HDR frame (short, long
> > > > > > > > > or medium) 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 | 75 ++++++++++++++++++++++++++++++++++
> > > > > > > > >  1 file changed, 75 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > > > > > > > index f2e542f4..c3232abf 100644
> > > > > > > > > --- a/src/libcamera/control_ids.yaml
> > > > > > > > > +++ b/src/libcamera/control_ids.yaml
> > > > > > > > > @@ -774,6 +774,81 @@ 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. HDR techniques typically include multiple exposure, image
> > > > > > > > > +        fusion and tone mapping techniques to improve the dynamic range of the
> > > > > > > > > +        resulting images.
> > > > > > > > > +
> > > > > > > > > +        When using an HDR mode, images are tagged to indicate which HDR channel
> > > > > > > > > +        (long, medium or short) they come from.
> > > > > > > > > +
> > > > > > > > > +        \sa HdrChannel
> > > > > > > > > +
> > > > > > > > > +      enum:
> > > > > > > > > +        - name: HdrModeOff
> > > > > > > > > +          value: 0
> > > > > > > > > +          description: |
> > > > > > > > > +            HDR is disabled. The HDR channel, if present, will report
> > > > > > > > > +            HdrChannelNone.
> > > > > > > >
> > > > > > > > Stating what HDR channel is used is an improvement compared to the
> > > > > > > > previous version, but there's still an option left to implementors here:
> > > > > > > > reporting HdrChannelNone, or not reporting HdrChannel at all. Unless you
> > > > > > > > see a reason to allow both, I would pick the latter:
> > > > > > > >
> > > > > > > >             HDR is disabled. Metadata for this frame will not include the
> > > > > > > >             HdrChannel control.
> > > > > > >
> > > > > > > Yes, I think that's fine.
> > > > > > >
> > > > > > > > > +        - 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. Each image will be tagged with the
> > > > > > > > > +            correct HDR channel, indicating what kind of exposure (long, medium
> > > > > > > > > +            or short) it is.  The expectation is that, if necessary, the
> > > > > > > > > +            application can merge them to create HDR images for itself.
> > > > > > > >
> > > > > > > > You mention here long, medium and short. Does this mean there will
> > > > > > > > always be three channels ?
> > > > > > >
> > > > > > > No - it's whatever the implementation wants to do. We don't use
> > > > > > > medium, for example. I think it's quite likely that some vendors would
> > > > > > > want other channels, such as "very short" and "very long". Maybe the
> > > > > > > description can avoid implying that all channels will appear.
> > > > > >
> > > > > > Shouldn't this be something that the application should control ? I'm
> > > > > > increasingly thinking that this shouldn't be an HDR mode, but should
> > > > > > instead be controlled through a per-frame control mechanism. Is there
> > > > > > any reason this can't be done, not right now (I don't want to delay this
> > > > > > patch unnecessarily), but at some point in the future ?
> > > > >
> > > > > Yes, I'd be happy with an API for multi-channel AGC, which is what
> > > > > this really is. This was actually where I started, though it seemed
> > > > > somewhat controversial so I thought it might be more palatable
> > > > > presented like this instead. But either way works for me.
> > > > >
> > > > > I'm very keen to handle this inside our algorithms, so that
> > > > > applications don't explicitly have to switch AGC channels all the
> > > > > time. That's a more reliable approach (there will be fewer failures to
> > > > > switch on every frame), and saves the application from a whole world
> > > > > of pain and complication.
> > > > >
> > > > > Though I'm all in favour of per-frame controls for lots of other
> > > > > reasons. We've been very keen to see that move forward for quite a
> > > > > long time now!
> > > >
> > > > Both make sense. I'm thinking that we may need some kind of "controls
> > > > scheduling" API to help applications alternate between different
> > > > settings, in addition to the full manual mode offered by per-frame
> > > > controls. I don't know yet how that would look like though.
> > >
> > > Control scheduling is an interesting idea. I'm not sure how it relates
> > > to AGC because AGC/AE controls are always a bit special because of the
> > > frame delays that are incurred in the sensor. Controls also tend to
> > > wind up at the back of the request queue which is another debatable
> > > feature for us, and not a place we'd be wanting exposure/gain updates
> > > to languish. So certainly things to think about.
> >
> > Another point to keep in mind is that, while application-side HDR with
> > staggered exposures in its simplest form from a libcamera point of view
> > would see the application specify per-frame manual exposures, I think we
> > need to provide a way to still run the libcamera-side AEC/AGC. Maybe
> > setting the ExposureValue per frame would be such a way, to enable
> > generation of darker and lighter frames based on the "normal" exposure
> > calculated by the AEC/AGC algorithm. Maybe we will also need more/other
> > controls for this.

I think application-side HDR is quite feasible for still image
capture, and would expect many users to want to do it this way,
possibly using different AGC "channels", possibly doing it all "by
hand".

I'm not persuaded by application-side HDR for video capture as I think
it would be difficult to use. In fact, I think most of our users would
actually give up if they had to do this for themselves.

Metering for HDR is an interesting question. Having worked on this for
a bit now, I've come to the conclusion that using ev ('ExposureValue')
to meter for different channels is a poor solution as it forces you to
under-expose even in cases where you didn't need to. A much better
idea (and what I've implemented) is to be able to target different AGC
channels at different parts of the image histogram. But these are just
my decisions - they may not even suit all Pi users, and certainly not
any other HDR implementor!

> >
> > > > > > > > > +        - name: HdrModeMultiExposure
> > > > > > > > > +          value: 2
> > > > > > > > > +          description: |
> > > > > > > > > +            Multiple exposures will be generated and merged to create HDR
> > > > > > > > > +            images. Each image will be tagged with the HDR channel (long, medium
> > > > > > > > > +            or short) that arrived and which caused this image to be output.
> > > > > > > > > +        - name: HdrModeSingleExposure
> > > > > > > > > +          value: 3
> > > > > > > > > +          description: |
> > > > > > > > > +            Multiple frames all at a single exposure will be used to create HDR
> > > > > > > > > +            images. These images should be reported as all corresponding to the
> > > > > > > > > +            HDR short channel.
> > > > > > > > > +        - name: HdrModeNight
> > > > > > > > > +          value: 4
> > > > > > > > > +          description: |
> > > > > > > > > +            Multiple frames will be combined to produce "night mode"
> > > > > > > > > +            images. Images will be tagged as belonging either to the long,
> > > > > > > > > +            medium or short HDR channel according to the implementation.
> > > > > > > >
> > > > > > > > Does this mean that night more will always use multi-exposure, or that
> > > > > > > > it is implementation-defined ?
> > > > > > >
> > > > > > > I really think that needs to be implementation defined. Our night mode
> > > > > > > is single-exposure, but I can't possibly predict what anyone else
> > > > > > > would want to do.
> > > > > >
> > > > > > Won't it be problematic for applications if they don't know what
> > > > > > HdrChannel values they will get ? How do you expect HdrChannel to be
> > > > > > used in the HDR modes where the camera combines images (all but the
> > > > > > Unmerged mode) ?
> > > > >
> > > > > Really, what I'm hoping is that the IQ stability control will indicate
> > > > > when it's OK to capture the frame, and this will be set when all the
> > > > > exposures that the implementation wants to use have arrived. So at
> > > > > that point the channel information will be of interest to some folks,
> > > > > but not essential to general applications. But obviously the IQ
> > > > > stability control discussion needs to progress.
> > > >
> > > > Those are two different issues, aren't they ? The point I was trying to
> > > > make is that I think the HdrChannel metadata will not be useful for
> > > > applications if we don't standardize at least a bit what it will report.
> > > > Uou gave an example where there would be a HdrChannelNone once per
> > > > "group" of exposures, to be used as a viewfinder. If applications need
> > > > that, they need to be able to rely on it being present, or at least know
> > > > if it will be, or have a way to select it.
> > >
> > > I agree standardisation is good here. I also think that beyond some
> > > obvious cases (long/medium/short) it will be difficult. I'm already
> > > expecting "very short" and "very long" at some point. I see a need for
> > > "this isn't HDR at all but is for preview", maybe that's just "this is
> > > a normal AGC image". I think some platforms might just want a long run
> > > of exposures where some might be the same, some might be different,
> > > and the only way to name them will be "0", "1" and so on.
> >
> > Could we at least decide on basic rules that govern what channels should
> > be used, depending on the number of channels ? For instance, if we were
> > to define 5 channels (very short, short, medium, long, very long), and a
> > camera used 3 channels, it would be horrible to ask applications
> > developers to be prepared to handle all possible combinations. Neither
> > would it make much sense, in my opinion, to allow cameras to use "very
> > short, long, very long" in that case. The exposures are relative to each
> > other, there's no rule to govern the delta between them, so I think we
> > can at least set some rules on the ordering.

As I think I said earlier, I believe there is a "high-level" HDR
interface, where you basically just turn HDR on, and wait for some
metadata on the image that says "it's ready now". This makes for an
easy interface, and lets any vendor implement HDR in whatever way they
want.

At that point, the HDR channel information is just for those who are
curious, you don't "need" it. Though I really want to tag all our
images with exactly what channel they are. We'll have users who want
to monitor the exposures on the individual channels so that they can
understand what's going on.

The precise order in which channels come out also becomes fairly
unimportant now, too. I could also imagine other vendors not wanting
to divulge any channel information at all, so maybe it needs to be a
purely optional thing. I think quite a few vendors regard HDR as
"secret sauce" and may be highly secretive about their
implementations.

> >
> > For the single-exposure case, does it even make sense to report a
> > channel, given that a single one is used ?

I could potentially go either way here, though where I'm actually
doing short channel metering I'd probably still nudge towards calling
it the "short" channel.

> >
> > For the multi-exposure cases, can we tell which channels will be used
> > based on the number of channels ? If you want some images to be reported
> > with channel "None", can we document this, and explain what those images
> > will be ?

I'm not sure about this one. I could imagine someone saying they want
to run in a mode where the frames come out in the order "short none
long none" so that they can get a half framerate viewfinder. (Or they
could ask for "short long none" and put up with a 1/3 rate viewfinder
- their choice.)

Perhaps we could have metadata that lists what channels are going to
be used, and also what the "cycle length" is after which the same
sequence of channels repeats (bearing in mind that interrupts can be
missed and channels "go missing" if you're unlucky)? But I'm very
prepared to accept that this kind of thing may not be sensible for
anyone other than Raspberry Pi!

I'm sorry if that possibly complicates everything. Perhaps there's
some mileage in agreeing the "simple HDR" API initially, and leaving
the rest for further discussion, and possibly vendor specific controls
where we think Raspberry Pi's requirements are not a good general fit?

Thanks!

David

> >
> > > We've had this problem to some extent elsewhere and have adopted a
> > > "custom" value. But it's not a great solution, and I don't think it
> > > really flies here when there could be many "custom" values.
> > >
> > > > What I'm missing here is a design, it seems to be more of a prototype to
> > > > see what will happen. It's fine to prototype things, but I would also
> > > > like to think about a longer term design. At the very least, how can we
> > > > get feedback on what will be useful to users, and when will we revisit
> > > > this ?
> > >
> > > I wouldn't disagree with any of that, though I think experience tells
> > > us that the only real way to get feedback is to put stuff out there
> > > and see how people react. We aren't short of wider camera forums where
> > > getting meaningful engagement from other parties is difficult! To be
> > > fair, Pi users are very good in this respect, though I think most
> > > would engage more actively when there's stuff to play with. But hard
> > > to say!
> >
> > I agree, getting it in the hands of users is certainly a way to get
> > feedback (how constructive that feedback is is a different question
> > :-)).
> >
> > > > > > > > > +
> > > > > > > > > +  - 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). An application can monitor
> > > > > > > > > +        the HDR channel to discover when the differently exposed images have
> > > > > > > > > +        arrived.
> > > > > > > > > +
> > > > > > > > > +      enum:
> > > > > > > > > +        - name: HdrChannelNone
> > > > > > > > > +          value: 0
> > > > > > > > > +          description: |
> > > > > > > > > +            This image does not correspond to any of the captures used to create
> > > > > > > > > +            an HDR image.
> > > > > > > >
> > > > > > > > As indicated above, do we need this, or should we not report HdrChannel
> > > > > > > > when HDR is disabled ?
> > > > > > >
> > > > > > > Actually I'd quite like to keep this, even if it's not reported when HDR is off.
> > > > > > >
> > > > > > > One use case is multi-exposure HDR on a Pi 4. You can't merge images
> > > > > > > so how would you get a viewfinder? You could intersperse some
> > > > > > > "ordinary AGC" frames with your long/short/whatever frames. You might
> > > > > > > want to label these "HdrChannelNone". I suppose you could label them
> > > > > > > "medium", but then maybe you're using "medium" for other frames that
> > > > > > > your HDR implementation requires. Like everything else, it all falls a
> > > > > > > bit into the "who knows what anyone will do" category.
> > > > > >
> > > > > > This makes me believe even more strongly that the unmerged mode
> > > > > > shouldn't be an HDR mode. We're trying to cram too many assumptions
> > > > > > about the application needs here.
> > > > > >
> > > > > > If you want to keep HdrChannelNone for the time being until we drop
> > > > > > HdrMultiExposureUnmerged, then this needs better documentation. Until I
> > > > > > read your reply I had no idea that the unmerged mode would produce
> > > > > > images for the "None" channel. If this isn't documented properly, it
> > > > > > won't be usable for applications.
> > > > > >
> > > > > > Depending on the priorities (I assume the Pi 5 HDR modes will be more
> > > > > > interesting than the unmerged operation on Pi 4), we could also drop the
> > > > > > unmerged mode for now, and design a solution better for application-side
> > > > > > HDR.
> > > > >
> > > > > I'm definitely very keen to move forward all the discussions on
> > > > > multi-channel AGC, IQ stability, per-frame controls and HDR. I am
> > > > > aware that I want to be very flexible about many aspects of HDR, and I
> > > > > explicitly do not want to tie our users down with "it will work this
> > > > > way", I want them to be able to implement the strategies that work
> > > > > best for them. So maybe vendor-specific controls are another topic
> > > > > that needs to be added to this list.
> > > > >
> > > > > For the moment, however, Pi 5 is upon us and realistically, I think we
> > > > > have no ability to change anything at this point. I'm definitely open
> > > > > to suggestions on how to rationalise things after our release.
> > > >
> > > > Would you prefer focussing on Pi 5 first and postponing
> > > > HdrModeMultiExposureUnmerged, or bundling them all together ? If your
> > > > focus is in Pi 5 and you won't have time to release an implementation of
> > > > HdrModeMultiExposureUnmerged with corresponding application-side support
> > > > in the short term, maybe it would be better to merge the camera-side HDR
> > > > support right now, and revisit HdrModeMultiExposureUnmerged when you'll
> > > > have time to work on its implementation ? To be perfectly clear here, I
> > > > think HdrModeMultiExposureUnmerged is less well-defined than the other
> > > > modes, and thus needs more work, but I'm also not opposed to merging it
> > > > early if we can come up with a plan to then improve it.
> > >
> > > For us, the release we're making in a day or two is a fait accompli at
> > > this point (Pi 5s are being shipped, I hear), and all this stuff is in
> > > it. All these modes are implemented and work. So from our point of
> > > view, bundling everything that we've done together would minimse
> > > divergence. Though we can live with it too, at least temporarily.
> >
> > OK, fine with me.
> >
> > > But as I said, I'm more than happy to revisit stuff. I think
> > > multi-channel AGC is probably a better answer than "unmerged" HDR, but
> > > we need to agree an approach.
> > >
> > > I think many platforms would react with horror to multi-channel AGC.
> > > They'd take the view that they'll just drive multiple different
> > > exposures explicitly, and not worry about multiple channels. Though
> > > you still have to decide if they need to adjust dynamically and how
> > > you switch between them, how they don't get too far apart, and how to
> > > make it jump from one exposure to another without adapting slowly. So
> > > it's all the same problems, just without exposing a general solution
> > > that enables it. But it's definitely more viable if you're only doing
> > > still image HDR. I can't escape wondering whether the dawn of vendor
> > > extensions is drawing closer.
> >
> > It's always drawing closer, but I think we're still far away from it for
> > HDR specifically. This is just the starting point.
> >
> > > So lots to think about!
> > >
> > > > > > > I guess there's also an outside chance that some implementations can't
> > > > > > > flick exposures instantaneously and accurately like we can, so maybe
> > > > > > > it would suit them too. Or we could have an
> > > > > > > "HdrChannelCouldntMakeThisWorkProperly" value? Having wrestled with
> > > > > > > all this for a while I can see the attraction! :)
> > > > > > >
> > > > > > > I'll send out another version today with the changes in line with all
> > > > > > > the above. Obviously everyone please shout if we want to do anything
> > > > > > > different.
> > > > > > >
> > > > > > > > > +        - 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
> > > > > > > > >
>
> --
> Regards,
>
> Laurent Pinchart