[libcamera-devel] [PATCH v3] libcamera: controls: Add controls for HDR
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Oct 20 15:18:45 CEST 2023
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 at raspberrypi.com>
> > > > > > > Reviewed-by: Naushir Patuck <naush at 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
> > > > > > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list