[libcamera-devel] [PATCH v2 2/5] libcamera: Add user Transform to CameraConfiguration

[Kieran Bingham]

Hi Laurent,

On 19/08/2020 02:58, Laurent Pinchart wrote:
> Hi David,
> 
> Thank you for the patch.
> 
> On Thu, Aug 06, 2020 at 05:36:36PM +0100, David Plowman wrote:
>> Add a field to the CameraConfiguration to represent a 2d transform
>> requested by the application. All pipeline handlers are amended to
>> coerce this to the Identity, marking the configuration as "adjusted"
>> if something different had been requested.
>>
>> Pipeline handlers that support Transforms can be amended subsequently.
> 
> Missing SoB here too. Maybe that was done on purpose, as the series
> isn't complete yet. It's also customary to express that with RFC/PATCH
> in the subject line (git-format-patch has a --subject-prefix options for
> that).

it also has --rfc directly as a shorthand ;-)

git format-patch -3 --cover-letter --rfc -v2

cat v2-0000-cover-letter.patch  | grep Subject
Subject: [RFC PATCH v2 0/3] *** SUBJECT HERE ***

--
Kieran


> 
> Apart from the missing documentation, this looks fine to me.
> 
>> ---
>>  include/libcamera/camera.h                         | 3 +++
>>  src/libcamera/camera.cpp                           | 2 +-
>>  src/libcamera/pipeline/ipu3/ipu3.cpp               | 5 +++++
>>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 +++++
>>  src/libcamera/pipeline/rkisp1/rkisp1.cpp           | 5 +++++
>>  src/libcamera/pipeline/simple/simple.cpp           | 5 +++++
>>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp       | 5 +++++
>>  src/libcamera/pipeline/vimc/vimc.cpp               | 5 +++++
>>  8 files changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
>> index 48d88d6..dedc1c6 100644
>> --- a/include/libcamera/camera.h
>> +++ b/include/libcamera/camera.h
>> @@ -17,6 +17,7 @@
>>  #include <libcamera/request.h>
>>  #include <libcamera/signal.h>
>>  #include <libcamera/stream.h>
>> +#include <libcamera/transform.h>
>>  
>>  namespace libcamera {
>>  
>> @@ -61,6 +62,8 @@ public:
>>  	bool empty() const;
>>  	std::size_t size() const;
>>  
>> +	Transform transform;
>> +
>>  protected:
>>  	CameraConfiguration();
>>  
>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
>> index 820fa1e..4282a02 100644
>> --- a/src/libcamera/camera.cpp
>> +++ b/src/libcamera/camera.cpp
>> @@ -93,7 +93,7 @@ LOG_DECLARE_CATEGORY(Camera)
>>   * \brief Create an empty camera configuration
>>   */
>>  CameraConfiguration::CameraConfiguration()
>> -	: config_({})
>> +	: transform(Transform::Identity), config_({})
>>  {
>>  }
>>  
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index d931ed3..a9a82a4 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -138,6 +138,11 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>>  	if (config_.empty())
>>  		return Invalid;
>>  
>> +	if (transform != Transform::Identity) {
>> +		transform = Transform::Identity;
>> +		status = Adjusted;
>> +	}
>> +
>>  	/* Cap the number of entries to the available streams. */
>>  	if (config_.size() > IPU3_MAX_STREAMS) {
>>  		config_.resize(IPU3_MAX_STREAMS);
>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> index eeaf335..236aa5c 100644
>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> @@ -400,6 +400,11 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>>  	if (config_.empty())
>>  		return Invalid;
>>  
>> +	if (transform != Transform::Identity) {
>> +		transform = Transform::Identity;
>> +		status = Adjusted;
>> +	}
>> +
>>  	unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0;
>>  	std::pair<int, Size> outSize[2];
>>  	Size maxSize;
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> index b7609cb..002f8e5 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> @@ -478,6 +478,11 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>>  	if (config_.empty())
>>  		return Invalid;
>>  
>> +	if (transform != Transform::Identity) {
>> +		transform = Transform::Identity;
>> +		status = Adjusted;
>> +	}
>> +
>>  	/* Cap the number of entries to the available streams. */
>>  	if (config_.size() > 1) {
>>  		config_.resize(1);
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index eb72e3b..10223a9 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -438,6 +438,11 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>>  	if (config_.empty())
>>  		return Invalid;
>>  
>> +	if (transform != Transform::Identity) {
>> +		transform = Transform::Identity;
>> +		status = Adjusted;
>> +	}
>> +
>>  	/* Cap the number of entries to the available streams. */
>>  	if (config_.size() > 1) {
>>  		config_.resize(1);
>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>> index bc892ec..fd14248 100644
>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>> @@ -108,6 +108,11 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()
>>  	if (config_.empty())
>>  		return Invalid;
>>  
>> +	if (transform != Transform::Identity) {
>> +		transform = Transform::Identity;
>> +		status = Adjusted;
>> +	}
>> +
>>  	/* Cap the number of entries to the available streams. */
>>  	if (config_.size() > 1) {
>>  		config_.resize(1);
>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
>> index cf244f1..bb791d6 100644
>> --- a/src/libcamera/pipeline/vimc/vimc.cpp
>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
>> @@ -130,6 +130,11 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()
>>  	if (config_.empty())
>>  		return Invalid;
>>  
>> +	if (transform != Transform::Identity) {
>> +		transform = Transform::Identity;
>> +		status = Adjusted;
>> +	}
>> +
>>  	/* Cap the number of entries to the available streams. */
>>  	if (config_.size() > 1) {
>>  		config_.resize(1);
> 

-- 
Regards
--
Kieran