Re: [PATCH 1/4] media: docs: Describe pixel array properties

From: Hans Verkuil <hverkuil@xs4all.nl>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Jacopo Mondi <jacopo@jmondi.org>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Linux Media Mailing List <linux-media@vger.kernel.org>,
	Sowjanya Komatineni <skomatineni@nvidia.com>,
	Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>,
	libcamera-devel@lists.libcamera.org
Subject: Re: [PATCH 1/4] media: docs: Describe pixel array properties
Date: Thu, 6 Aug 2020 15:22:34 +0200	[thread overview]
Message-ID: <1ee9f378-8df1-7c03-2136-ce7870934443@xs4all.nl> (raw)
In-Reply-To: <20200806125445.GA16270@paasikivi.fi.intel.com>

On 06/08/2020 14:54, Sakari Ailus wrote:
> Hi Hans,
> 
> On Thu, Aug 06, 2020 at 11:58:31AM +0200, Hans Verkuil wrote:
>> On 06/08/2020 11:50, Jacopo Mondi wrote:
>>> Hi Hans,
>>>
>>> On Thu, Aug 06, 2020 at 10:05:37AM +0200, Hans Verkuil wrote:
>>>> Hi Jacopo,
>>>>
>>>> Some review comments below:
>>>>
>>>> On 05/08/2020 12:57, Jacopo Mondi wrote:
>>>>> +Analog crop rectangle
>>>>
>>>> Why analog? It's just the crop rectangle, nothing analog about it.
>>>>
>>>
>>> We used the 'analogCrop' term in libcamera to differentiate the
>>> cropping which happens on the sensor pixel array matrix to select the
>>> region to process and produce image from. Sensor with an on-board
>>> scaler can perform other cropping steps to implement, in example digital
>>> zoom, so we expect to have a 'digital crop' phase as well. RAW
>>> sensors, in example, will only have an analogCrop rectangle.
>>>
>>> Quoting the libcamera definition of analog crop:
>>>
>>>  * horizontal and vertical sizes define the portion of the pixel array which
>>>  * is read-out and provided to the sensor's internal processing pipeline, before
>>>  * any pixel sub-sampling method, such as pixel binning, skipping and averaging
>>>  * take place.
>>>
>>> should I keep it or remove it ?
>>
>> It's a very confusing term. Especially since this API can also be used with analog
>> video capture devices (Composite/S-Video) where the video signal actually is analog.
>>
>> In the V4L2 API there is no such thing as 'analog crop', so please remove it.
> 
> There isn't in the V4L2 API but I don't see why we couldn't document it
> here. Analogue crop is an established term related to raw (perhaps others,
> too?) camera sensors which describes cropping that is implemented by not
> reading parts of the pixel array.
> 
> As this documentation only applies to camera sensors, I think it's entirely
> appropriate to document this here, and using that term.
> 

It's always been called just 'crop' in the V4L2 API, so renaming it suddenly
to something else is IMHO confusing. What you can do, however, is that in the
description of the "crop rectangle" you mention that "it is also known as
"analog crop" in the context of camera sensors.

With perhaps some more extensive explanation of the term.

Regards,

	Hans