[libcamera-devel] [PATCH v2 1/2] libcamera: Implement digital zoom

[Kieran Bingham]

Hi David,

On 21/07/2020 12:27, David Plowman wrote:
> Hi again Kieran, everyone
> 
> So I've been contemplating some of this a bit more since yesterday,


I've been trying to give this a bit more thought too... more discussion
below:


> and I wonder if I'm confusing everything by giving that contentious
> function the name "getSensorCrop". It makes it sound like it's a
> property of the sensor when it really isn't - it can change whenever
> the sensor mode chosen changes, or the size of the requested output
> changes. It's an interaction between the sensor mode and the behaviour
> of the pipeline handler, so I wonder if we should be coining a
> different name for it?
> 
> How about....
> 
> getInputCrop
> getPipelineCrop
> getPipelineSensorCrop
> getPipelineSensorArea
> 
> Of those I think I prefer the last one, it's the area of the sensor
> that the pipeline handler has chosen that we can use. But does anyone
> have any better suggestions?
> 


I think I actually like getInputCrop(or is it setInputCrop?)

Perhaps I'm getting confused as to whether this addition is to set or
get the zoom window.



> I guess I'm also still unclear as to where to store it - I'm assuming
> that the SensorInfo isn't the right place for something that can
> change like this. (?)


I seem to still be confused as to what data this is representing.

I.e. is it:

A) A crop which reduces the output of the sensor (sensorOutputCrop) and
further restricts the data output by the sensor reducing the overall
window further than the analogCrop.

B) A crop which restricts the input to the ISP/Scaler (scalerInputCrop)
and must be smaller than the analogCrop from the sensor



Also, I think I have been confused by the fact that there is a Rectangle
DigitalZoom control added in this patch, which should come later, and
will 'use'/'set' the crop being defined by this patch?


Maybe a video call to discuss this sometime might help me understand
what parts you are referring to.



> Thanks!
> David
> 
> On Mon, 20 Jul 2020 at 08:19, David Plowman
> <david.plowman at raspberrypi.com> wrote:
>>
>> Hi Kieran
>>
>> Thanks for your feedback. Apologies for the delay in replying - I've
>> been away for the past week!
>>
>> I agree there's still some stuff to figure out here! Let me try and
>> answer some of the questions (and probably pose some more!):
>>
>> On Fri, 10 Jul 2020 at 14:12, Kieran Bingham
>> <kieran.bingham at ideasonboard.com> wrote:
>>>
>>> Hi David,
>>>
>>> On 09/07/2020 10:15, David Plowman wrote:
>>>> These changes add a Digital Zoom control, tacking a rectangle as its
>>>
>>> s/tacking/taking/
>>>
>>>> argument, indicating the region of the sensor output that will be
>>>> zoomed up to the final output size.
>>>>
>>>> Additionally, we need have a method returning the "sensorCrop" which
>>>
>>> s/need have/need to have/
>>>
>>>> gives the dimensions of the sensor output within which we can pan and
>>>> zoom.
>>>
>>>
>>> This commit message describes implementing digital zoom, but *this*
>>> commit is only dealing with the representaion of a crop region in the
>>> (CameraSensor) which can be used to implement digital zoom.
>>>
>>> Perhaps this commit should be titled:
>>>  "libcamera: camera_sensor: Support cropping regions"
>>>
>>> (with that being the assumption that my comments below about moving this
>>> to the CameraSensor class are correct)
>>>
>>>
>>>
>>>> ---
>>>>  include/libcamera/camera.h                    |  2 ++
>>>>  include/libcamera/internal/pipeline_handler.h |  4 +++
>>>>  src/libcamera/camera.cpp                      | 26 +++++++++++++++++++
>>>>  src/libcamera/control_ids.yaml                | 10 +++++++
>>>>  4 files changed, 42 insertions(+)
>>>>
>>>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
>>>> index 4d1a4a9..dd07f7a 100644
>>>> --- a/include/libcamera/camera.h
>>>> +++ b/include/libcamera/camera.h
>>>> @@ -92,6 +92,8 @@ public:
>>>>       std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles = {});
>>>>       int configure(CameraConfiguration *config);
>>>>
>>>> +     Size const &getSensorCrop() const;
>>>> +
>>>>       Request *createRequest(uint64_t cookie = 0);
>>>>       int queueRequest(Request *request);
>>>>
>>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>>>> index 22e629a..37e069a 100644
>>>> --- a/include/libcamera/internal/pipeline_handler.h
>>>> +++ b/include/libcamera/internal/pipeline_handler.h
>>>> @@ -89,6 +89,8 @@ public:
>>>>
>>>>       const char *name() const { return name_; }
>>>>
>>>> +     Size const &getSensorCrop() const { return sensorCrop_; }
>>>> +
>>>>  protected:
>>>>       void registerCamera(std::shared_ptr<Camera> camera,
>>>>                           std::unique_ptr<CameraData> data);
>>>> @@ -100,6 +102,8 @@ protected:
>>>>
>>>>       CameraManager *manager_;
>>>>
>>>> +     Size sensorCrop_;
>>>> +
>>>
>>> I think this needs to go in the CameraSensor class...
>>> It's 'per sensor' not 'per pipeline handler' right?
>>
>> Maybe this is the billion dollar question. See below...!


I might also have meant CameraData ... but it depends still...


>>>>  private:
>>>>       void mediaDeviceDisconnected(MediaDevice *media);
>>>>       virtual void disconnect();
>>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
>>>> index 69a1b44..6614c93 100644
>>>> --- a/src/libcamera/camera.cpp
>>>> +++ b/src/libcamera/camera.cpp
>>>> @@ -793,6 +793,32 @@ int Camera::configure(CameraConfiguration *config)
>>>>       return 0;
>>>>  }
>>>>
>>>> +/**
>>>> + * \brief Return the size of the sensor image being used to form the output
>>>> + *
>>>> + * This method returns the size, in pixels, of the raw image read from the
>>>> + * sensor and which is used to form the output image(s). Note that these
>>>> + * values take account of any cropping performed on the sensor output so
>>>> + * as to produce the correct aspect ratio. It would normally be necessary
>>>> + * to retrieve these values in order to calculate correct parameters for
>>>> + * digital zoom.
>>>> + *
>>>> + * Example: a sensor mode may produce a 1920x1440 output image. But if an
>>>> + * application has requested a 16:9 image, the values returned here would
>>>> + * be 1920x1080 - the largest portion of the sensor output that provides
>>>> + * the correct aspect ratio.
>>>
>>> Should this be a Rectangle? Can the crop region be positioned
>>> arbitrarily or is it always centered?
>>
>> As I've implemented this, it is just a pair of dimensions. The
>> pipeline handler may position it anywhere it likes within the array of
>> pixels output by the sensor (and takes care of any offset within
>> that), but the application doesn't need to know. The idea being that
>> the application gets to choose a *rectangle* from within the
>> dimensions it is given and doesn't have to bother itself with how it's
>> offset in the pixel array beyond that.

Surely the position is quite essential to define somehow though,
otherwise we'll get some zoom which will zoom in on the bottom right
corner and be perfectly valid, while another zoom chooses the centre,
and another the top left ... ?

I assume centreing is usually the right thing to do - but I don't know ...

If it is defined as always zooming in on the centre point then indeed a
Size would be the only thing needed to store.






>>> What is the relationship between this and the analogCrop in
>>> CameraSensorInfo? I 'think' that one is more about what the actual valid
>>> data region is from the camera, so I presume the analogCrop is sort of
>>> the 'maximum' image size?
>>
>> Yes, I think the analogCrop is the maximum usable pixel area from the
>> sensor. The crop here is what the pipeline handler decides it wants to
>> use. It's adjusted for aspect ratio (of the requested output), the
>> actual size that it wants to use (and hence the scaling). I certainly
>> agree that it might want putting somewhere else... yet it's only the
>> pipeline handler that can calculate it. It depends on the sensor, but
>> it also depends on the mode selected, the output size requested, and
>> "arbitrary" decisions made in the pipeline handler (for example, they
>> may wish to avoid marginal scaling and prefer 1:1 input to output
>> pixels).


Just to clarify further, do you expect that this control changes the
mode of the camera at all? I.e. does it crop the data coming from the
sensor? Or is this purely applying the input crop to the rescaler ?


>>>> + *
>>>> + * \context This function is \threadsafe. It will only return valid
>>>> + * (non-zero) values when the camera has been configured.
>>>> + *
>>>> + * \return The dimensions of the sensor image in use.
>>>> + */
>>>> +
>>>> +Size const &Camera::getSensorCrop() const
>>>> +{
>>>> +     return p_->pipe_->getSensorCrop();
>>>
>>> Aha, more indication to me that this should go in to the Camera class,
>>> and it would save the indirection required here...
>>
>> Indeed. But given that the pipeline handler calculates it, can we have
>> it poke its value into the Camera class? Or is that weird if the
>> number depends on rather more than just the camera? (And will change
>> every time we start the sensor in a different mode.)

Hrm - ok so that depends on something I assumed. I assumed that this
would be setting a property on the sensor as well, but now I realise I
think this is just defining a crop which will determine what window to
read from at the input to the rescaler.


>> Another solution to this that I've talked about previously is simply
>> to go use ratios, and avoid pixel coordinates altogether. Then you
>> just don't need this function. The counter argument is that an
>> application can't control digital pan/zoom down to the precise pixel -
>> which sounds appealing. Yet I think real applications will have in
>> mind a _rate_ at which they wish to work (e.g. pan across half the
>> field of view in 5 seconds) at which point ratios are quite natural.

Yes, I think I am used to seeing zoom in terms of a multiplier, but that
won't allow positioning/panning within the region.





>>
>> Best regards
>> David
>>
>>>
>>>
>>>> +}
>>>> +
>>>>  /**
>>>>   * \brief Create a request object for the camera
>>>>   * \param[in] cookie Opaque cookie for application use
>>>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
>>>> index 988b501..5a099d5 100644
>>>> --- a/src/libcamera/control_ids.yaml
>>>> +++ b/src/libcamera/control_ids.yaml
>>>> @@ -262,4 +262,14 @@ controls:
>>>>          In this respect, it is not necessarily aimed at providing a way to
>>>>          implement a focus algorithm by the application, rather an indication of
>>>>          how in-focus a frame is.
>>>> +
>>>> +  - DigitalZoom:
>>>> +      type: Rectangle
>>>> +      description: |
>>>> +        Sets the portion of the full sensor image, in pixels, that will be
>>>> +        used for digital zoom. That is, this part of the sensor output will
>>>> +        be scaled up to make the full size output image (and everything else
>>>> +        discarded). To obtain the "full sensor image" that is available, the
>>>> +        method Camera::getOutputCrop() should be called once the camera is

Aha, so the DigitalZoom is a rectangle that it can zoom in on... I think
I confused this DigitalZoom rectangle with the getSensorCrop() above.

But that also highlights a confusion in this control documentation, Did
you mean getSensorCrop when you stated getOutputCrop above?


I think this actual DigitalZoom control addition perhaps shouldn't be in
this patch, and should be in a patch on it's own after, or in patch 2/2
in the series?

Oh, and now I see both patches have the same title - That might need
fixing too to clarify intent of each patch ;-) It probably is also why I
have been confused. I thought this patch was implementing more of the
digital zoom than I realised, and actually the zooming part is in the
next patch...





>>>> +        configured. An application may pan and zoom within this rectangle.
>>>>  ...
>>>>
>>>
>>> --
>>> Regards
>>> --
>>> Kieran

-- 
Regards
--
Kieran