<div dir="ltr"><div dir="ltr">Hi Jacopo,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 10 Dec 2020 at 16:04, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On Thu, Dec 10, 2020 at 03:17:49PM +0000, Naushir Patuck wrote:<br>
> Hi Jacopo,<br>
><br>
> Thank you for your review comments.<br>
><br>
> On Thu, 10 Dec 2020 at 14:48, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>> wrote:<br>
><br>
> > Hi Naush,<br>
> > thanks for the update<br>
> ><br>
> > On Thu, Dec 10, 2020 at 01:34:24PM +0000, Naushir Patuck wrote:<br>
> > > Add a float array control (controls::FrameDurations) to specify the<br>
> > > minimum and maximum (in that order) frame duration to be used by the<br>
> > > camera sensor.<br>
> > ><br>
> > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > > Tested-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > > ---<br>
> > > src/libcamera/control_ids.yaml | 15 +++++++++++++++<br>
> > > 1 file changed, 15 insertions(+)<br>
> > ><br>
> > > diff --git a/src/libcamera/control_ids.yaml<br>
> > b/src/libcamera/control_ids.yaml<br>
> > > index 6d6f0fee..7f1f8624 100644<br>
> > > --- a/src/libcamera/control_ids.yaml<br>
> > > +++ b/src/libcamera/control_ids.yaml<br>
> > > @@ -554,4 +554,19 @@ controls:<br>
> > > detection, additional format conversions etc) count as an<br>
> > additional<br>
> > > pipeline stage.<br>
> ><br>
> > So, I've gone through the lentghy discussions on v2 and v3.<br>
> ><br>
> > To be very honest, I think we still have some missing pieces and the<br>
> > one that concerns me the more is the interaction of this control with<br>
> > the selected AE mode and the consequences on exposure/shutter-time<br>
> > priorites. I see all controls about Exposure have their interaction<br>
> > definition defferred to a \todo item. This is of course not the ideal<br>
> > situation but adding one make the issue only slightly worse. Deferring<br>
> > these to the pipeline model definition might be an option.<br>
> ><br>
><br>
> You are entirely correct here on all ponts. There is a tightly coupled<br>
> interaction with this FrameDurations control and interaction with the AGC<br>
> algorithm. You have also thrown a spanner in the works for my plan :)<br>
> David and I have been working on exactly this and we think we have<br>
> addressed all these interactions. I have a set of patches that are ready<br>
> to be pushed as a "phase 2", but wanted to get this through first so as not<br>
> to muddy the waters too much. This set of "phase 2" patches address the<br>
> following:<br>
<br>
Ah great :)<br>
<br>
><br>
> 1) Set VBLANK ahead of any other controls. This avoids the problem of<br>
> setting EXPOSURE and having v4l2 reject the value due to stale limits.<br>
> 2) Limit VBLANK (and hence frame duration) to what the sensor mode can<br>
> actually support. This forms part of your Clipping discussion below.<br>
> 3) Add an interaction with AE when FrameDurations. This addresses your<br>
> concerns above, with AE knowing exactly what limits of shutter speed are<br>
> achievable and working around any limitations based on exposure modes<br>
> selected.<br>
<br>
These are all improvements to the RPi implementation which are<br>
certainly positive, but if I got these right, it's mostly about<br>
implementation.<br></blockquote><div><br></div><div>That's correct. The "phase 2" patches are entirely Raspberry Pi implementation specific.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Using your point 3) as example, my main concern is more where/how<br>
to specify how pipelines behaves (in the control documentation, in the<br>
pipeline handler model etc), or if we want all pipelines to behave the<br>
same, which I'm not even sure it's possible or desirable to enforce.<br></blockquote><div><br></div><div>I suppose what the best thing to do for now is put some wording in the control documentation on how it *should* interact with the AE. My next patch will add this, and we can discuss if this is appropriate or not. Maybe a larger and seperate piece of work would be to expand on this in the pipeline handler model as well?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Did I get you right and your IPA will:<br>
1) Select the appropriate exposure time using the hint provided by<br>
AeExposureMode<br>
2) Clip the selected interval in the FrameDurations limits<br></blockquote><div><br></div><div>Yes, that is precisely what happens.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
This seems more than reasonable but then the first question I have is<br>
how to make this consistent for application among different<br>
platforms/IPA implementations and how to clearly document that.<br></blockquote><div><br></div><div>The control documentation? :-)</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
This additional controls does not define that, and I think it's fair.<br>
What we should avoid is going in a direction that makes it too hard to<br>
backtrack, and I don't think there's anything that bad here.<br>
<br>
We just need more IPAs, to have a large enough base to prove these<br>
concepts against a different implementation I guess.<br>
<br>
><br>
> Perhaps I should make those changes as part of this series so you get the<br>
> full picture right now?<br>
<br>
The risk is to delay this more. I would do that on top if I got you<br>
right and these are mostly improvements specific to the RPi<br>
implementation.<br></blockquote><div><br></div><div>Agreed, this is why I did not introduce it just yet.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
><br>
> ><br>
> > I'm willing to ack this patch, but I think there are a few details I<br>
> > would like to discuss:<br>
> ><br>
> > - Clipping. We need a (per-configuration, like the ScalerCrop<br>
> > rectangle) property to provide application limits and refer to it in<br>
> > this control description. I'll address this on top, but I would<br>
> > apreciate a:<br>
> ><br>
> > \todo Refer to the frame duration limits property to describe how<br>
> > application-provided values gets clipped or how to reset the control<br>
> > value to its default.<br>
> ><br>
><br>
> This is partially addressed in my point (2) above. However, I did not add<br>
> a property, rather just did clipping based on VBLANK limits to the IPA. So<br>
> do you think we should have a "properties::SensorMaxFramerate" or similar<br>
> that provides the application the maximum possible framerate for this mode<br>
> (derived from VBLANK control limits)? As mentioned above, right now I<br>
> simply clip the user request to what the sensor mode can achieve.<br>
<br>
As an implementation it's fine, but I think we need a mechanism to allow<br>
application to access that information. At least, I know we need this<br>
for Android, so something has to be added.<br>
<br>
And I think once we have that property is fair to mention here that<br>
values outside the limits there reported will be clipped (and that's<br>
something I think we can assume for all pipelines).<br>
<br>
Then if you want to support this, you will simply have to update the<br>
Camera property to report the VBLANK limits.<br>
<br>
><br>
><br>
> ><br>
> > - This is both a control and a metadata. I think the<br>
> > description is only about 'setting' the FrameDurations, not reading<br>
> > it. We have discussed in length the former but somewhat ignored the<br>
> > latter.<br>
> ><br>
> > The 'reading' case could addressed by introducing a read-only control<br>
> > (ie FrameDuration) only to be used as metadata. But this might even be<br>
> > more confusing as people will wonder why they have to use<br>
> > 'Duration-s-' when they want a precise value and theres a 'Duration'<br>
> > (without 's') available. I'll propose an additional section but if<br>
> > you have ideas please suggest them. Also, feel free to leave this<br>
> > last part out if it turns out to be controversial and would delay the<br>
> > series any longer.<br>
> ><br>
><br>
> Yes, good point here. Returning a "FrameDuration" may be a bit redundant,<br>
> as this information is conveyed in the FrameBuffer timestamps. What do<br>
> other folks think?<br>
<br>
That's a good point. We have timestamps and durations can be<br>
calculated. I still think it would be nicer for application to<br>
have a direct way to access this as part of the Request metadata. Or<br>
we can expose the sensor timestamp from the Request and let them calculate<br>
durations..<br></blockquote><div><br></div><div></div><div>Yes, any of these options would work, and should be easy to put in place. For now, perhaps I keep "FrameDurations" metadata as what the limits are as per the user request.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
><br>
> ><br>
> > ><br>
> > > + - FrameDurations:<br>
> > > + type: float<br>
> ><br>
> > I understand this is meant to accommodate standard FPS like 29,97 FPS<br>
> > (-.-) but won't expressing this as nanoseconds with a uin64_t<br>
> > representation be capable of achieving the same. Floating point<br>
> > arithmentic is generally a bit harder and clunky to handle when doing<br>
> > calculations. I won't push if you think a float is better.<br>
> ><br>
><br>
> I only used float here as that is what we use in our IPA :) Happy to change<br>
> to int64_t based numbers.<br>
><br>
<br>
I would slightly prefer, but it's a gut feeling. I suspect you have a<br>
reasons why you used float in your IPA I am missing. If that's the<br>
case feel free to keep float here.<br></blockquote><div><br></div><div>int64_t is fine, and it somewhat mirrors ExposureTime (int32_t) which is nice. All of our controller code uses floats, so it is trivial for me to do a conversion in our IPA.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> ><br>
> > > + description: |<br>
> ><br>
> > I would:<br>
> > size: [2]<br>
> > description:<br>
> > The minimum and maximum (in that order) frame duration,<br>
> > expressed in micro-seconds.<br>
> ><br>
> > When provided by applications the control specifies the<br>
> > sensor frame duration interval the pipeline has to use. This<br>
> > could also limit the largest exposure times the sensor can<br>
> > use. For example, if a maximum frame duration of 33ms is<br>
> > requested (corresponding to 30 frames per second), the<br>
> > sensor will not be able to raise the exposure time above<br>
> > 33ms. A fixed frame duration is achieved by setting the<br>
> > minimum and maximum values to be the same.<br>
> ><br>
> > \todo Refer to the frame duration limits property to describe how<br>
> > application-provided values gets clipped or how to reset the<br>
> > control<br>
> > values to their defaults.<br>
> ><br>
> > \todo Better specify how the frame durations interact with the<br>
> > exposure control algorithm.<br>
> > \sa AeEnable<br>
> > \sa AeExposureMode<br>
> > \sa ExposureTime<br>
> ><br>
> > -----------------8< you can cut here 8<--------------------<br>
> ><br>
> > When reported by pipelines the control expresses the duration<br>
> > of the sensor frame used to produce streams part of the completed<br>
> > Request. The minimum and maximum values shall then be the same,<br>
> > as the<br>
> > sensor frame duration is a fixed parameter. The sensor frame<br>
> > duration is one of the parameter that defines the capture<br>
> > frame rate but it does not alone provide enough information<br>
> > to fully calculate it as it does not account for pipeline<br>
> > processing delays.<br>
> ><br>
> > \todo Define how to calculate the capture frame rate by<br>
> > defining controls to report additional delays introduced by<br>
> > the capture pipeline or post-processing stages (ie JPEG<br>
> > conversion, frame scaling).<br>
> ><br>
><br>
> Sure, no problem. I will use the above wording and perhaps expand a bit<br>
> more on the AE interactions.<br>
<br>
Great thanks, we can hopefully ack this sooner than later then.<br></blockquote><div><br></div><div>Will post an update with the new description text (and the other discussed changes), and we can look to see if it is more appropriate.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Thanks<br>
j<br>
<br>
><br>
><br>
> ><br>
> > > + Specifies the minimum and maximum (in that order) allowable<br>
> > frame<br>
> > > + duration, in micro-seconds, for the sensor to use. This could<br>
> > also limit<br>
> > > + the largest exposure times the sensor can use. For example, if<br>
> > a maximum<br>
> > > + frame duration of 33ms is requested (corresponding to 30 frames<br>
> > per<br>
> > > + second), the sensor will not be able raise the exposure time<br>
> > above 33ms.<br>
> ><br>
> > s/will not be able raise/will not be able to raise/<br>
> > Fixed in the above suggestions.<br>
> ><br>
><br>
> Ack.<br>
><br>
> Regards,<br>
> Naush<br>
><br>
><br>
> ><br>
> > Thanks<br>
> > j<br>
><br>
><br>
> > > + A fixed frame duration is achieved by setting the minimum and<br>
> > maximum<br>
> > > + values to be the same. Note that the sensor may not always be<br>
> > able to<br>
> > > + provide the requested frame duration limits depending on its<br>
> > mode<br>
> > > + configuration.<br>
> ><br>
> > > + \sa ExposureTime<br>
> > > + size: [2]<br>
> > > ...<br>
> > > --<br>
> > > 2.25.1<br>
> > ><br>
> > > _______________________________________________<br>
> > > libcamera-devel mailing list<br>
> > > <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> > > <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
> ><br>
</blockquote></div></div>