[libcamera-devel] [PATCH LIBCAMERA v3] libcamera: rkisp1: Use parametered constructor instead of default

[Kieran Bingham]

Hi Kaaira,

On 23/03/2020 13:36, Kaaira Gupta wrote:
> On Mon, Mar 23, 2020 at 01:10:33PM +0000, Kieran Bingham wrote:
>> Hi Kaaira,
>>
>> On 23/03/2020 08:44, Kaaira Gupta wrote:
>>> Use of default constructor StreamConfiguration() is deprecated, hence
>>> make it private. Make Stream class a friend of StreamConfiguration as it
>>> uses the default constructor.
>>>
>>> Replace default constructor StreamConfiguration() by it's parametered
>>
>> s/parametered/parametrized/ ? If so, both here and in the subject line.
>>
>>> counterpart by using StreamFormats in generateConfiguration() in rkisp1
>>>
>>> Also, use erase(), instead of replace() in validate() to prevent it from
>>> creating a new instance with empty constructor.
>>>
>>> Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
>>> ---
>>>
>>> Changes since v1:
>>> 	- replace resize() by erase() in validate() to prevent it from
>>> creating a new instance with empty constructor.
>>> Changes since v2:
>>> 	- Rearaange the order of declaration of friend class.
>>> 	- Add constructor implementation again to stream.cpp
>>> 	- Corrections in range of erase()
>>>
>>> include/libcamera/stream.h               |  4 ++-
>>>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 39 ++++++++++++++++--------
>>>  src/libcamera/stream.cpp                 |  6 ++--
>>>  3 files changed, 32 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
>>> index b1441f8..a379ebb 100644
>>> --- a/include/libcamera/stream.h
>>> +++ b/include/libcamera/stream.h
>>> @@ -37,7 +37,6 @@ private:
>>>  };
>>>  
>>>  struct StreamConfiguration {
>>> -	StreamConfiguration();
>>>  	StreamConfiguration(const StreamFormats &formats);
>>>  
>>>  	PixelFormat pixelFormat;
>>> @@ -52,6 +51,9 @@ struct StreamConfiguration {
>>>  	std::string toString() const;
>>>  
>>>  private:
>>> +	friend class Stream;
>>> +	StreamConfiguration();
>>> +
>>
>> Because the change to move the StreamConfiguration(); to being private
>> causes multiple breakage, I think we should have it as a separate
>> change, so this has certainly already become a series :-)
> 
> Exactly, it does cause a lot of breakage. But I am having a hard time
> figuring out what all they are because I think I haven't figured it out
> right how I can run tests on one thing at a time or maybe 'stop' the
> tests on a particular thing? Can you please help me with it? :/

I assume here you mean that one of the tests gets broken with the
change? (test/v4l2_videodevice/buffer_cache I think)

The important thing to remember here is that the change you make to the
rkisp1 should still work without the change to the StreamConfiguration
being made private, so I would recommend splitting that part out already.

You should be able to test your change to RKISP1 (by ensuring that it
compiles cleanly, - you don't have hardware to test it running) entirely
by *not* including the patch which makes StreamConfiguration private.

And indeed, we could expect a few patches that can be tested
independently before the StreamConfiguration is moved...

The series would end up looking something like this:


 - [1/n] pipeline: Remove use of .resize() in validate() calls.
 - [2/n] pipeline: rkisp1: Use paramaterized StreamConfiguration
 - [3/n] pipeline: ipu3: Use paramaterized StreamConfiguration
 - [4/n] test: Fix v4l2_videodevice/buffer_cache test
 - [5/n] stream: Make default constructor of StreamConfiguration private


Patch 1 should fix all uses of the .resize() which we know won't work
(so that's vimc, uvc, rkisp1, ipu3).

Patch 2 should then handle the rkisp1 changes as you've already identified.

You should be able to compile cleanly and run the test suite on both of
those patches independently.


Patch 3 can then apply the similar changes you've made to the RKISP..


Patch 4 should then fix the issue that will be faced in the buffer_cache
test. I haven't looked at that yet to determine the solution. Is this
the point you are currently blocked?


If there are any other fixes needed, they'd go in here, and patch 5
would come later of course...


Patch 5 should handle the privatisation of the StreamConfiguration
default constructor.



>> The series should thus fix all breakages (before it breaks) then move
>> this at the end of the series. Probably along with the removal of the \todo
>>
>>
>>>  	Stream *stream_;
>>>  	StreamFormats formats_;
>>>  };
>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>> index 2f909ce..6839e3c 100644
>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>> @@ -218,6 +218,21 @@ private:
>>>  	Camera *activeCamera_;
>>>  };
>>>  
>>> +namespace {
>>> +
>>> +static const std::array<PixelFormat, 8> formats{
>>> +	PixelFormat(DRM_FORMAT_YUYV),
>>> +	PixelFormat(DRM_FORMAT_YVYU),
>>> +	PixelFormat(DRM_FORMAT_VYUY),
>>> +	PixelFormat(DRM_FORMAT_NV16),
>>> +	PixelFormat(DRM_FORMAT_NV61),
>>> +	PixelFormat(DRM_FORMAT_NV21),
>>> +	PixelFormat(DRM_FORMAT_NV12),
>>> +	/* \todo Add support for 8-bit greyscale to DRM formats */
>>> +};
>>> +
>>> +} /* namespace */
>>> +
>>>  RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
>>>  	: pipe_(dynamic_cast<PipelineHandlerRkISP1 *>(pipe))
>>>  {
>>> @@ -430,17 +445,6 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
>>>  
>>>  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>>>  {
>>> -	static const std::array<PixelFormat, 8> formats{
>>> -		PixelFormat(DRM_FORMAT_YUYV),
>>> -		PixelFormat(DRM_FORMAT_YVYU),
>>> -		PixelFormat(DRM_FORMAT_VYUY),
>>> -		PixelFormat(DRM_FORMAT_NV16),
>>> -		PixelFormat(DRM_FORMAT_NV61),
>>> -		PixelFormat(DRM_FORMAT_NV21),
>>> -		PixelFormat(DRM_FORMAT_NV12),
>>> -		/* \todo Add support for 8-bit greyscale to DRM formats */
>>> -	};
>>> -
>>>  	const CameraSensor *sensor = data_->sensor_;
>>>  	Status status = Valid;
>>>  
>>> @@ -449,7 +453,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>>>  
>>>  	/* Cap the number of entries to the available streams. */
>>>  	if (config_.size() > 1) {
>>> -		config_.resize(1);
>>> +		config_.erase(config_.begin() + 1, config_.end());
>>
>> Interesting, how does this differ from .resize(1)?
>>
>> Ah, I see - it's required because if .resize() is used to 'increase' the
>> size, it uses the default constructor, and that is no longer available.
> 
> Yes
> 
>>
>> I think it would be useful to add a comment to explain /why/ we use
>> .erase() instead of .resize() here...
> 
> Okay I will

If you group the vimc/uvc element of this change, it could potentially
be a single patch making this change for all of the pipeline handlers at
the same time, each one doing the 'right' change and adding the same
comment or such.

That patch would then come before the changes to generateConfiguration
in this pipeline handler.


> 
>>
>>
>>>  		status = Adjusted;
>>>  	}
>>>  
>>> @@ -537,7 +541,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
>>>  	if (roles.empty())
>>>  		return config;
>>>  
>>> -	StreamConfiguration cfg{};
>>> +	std::map<PixelFormat, std::vector<SizeRange>> pixelformats;
>>> +
>>> +	for (PixelFormat pixelformat : formats) {
>>> +		std::vector<SizeRange> sizes{
>>
>> Where do the values for the following SizeRange come from? A comment to
>> indicate could be beneficial.
>>
>> Presumably these are the minimum and maximum capabilities of the ISP
>> output ?
> 
> Yes, I took them from here in validate() :
> 
>  cfg.size.width = std::max(32U, std::min(4416U, cfg.size.width));
>  cfg.size.height = std::max(16U, std::min(3312U, cfg.size.height));

Ah yes, it would be nice if these were in a common location - but I
don't think you need to worry about that for now in this patch.

>> Ideally these values should be determined from the hardware driver, but
>> maybe that's not possible right now.
>>
>>
>>> +			SizeRange{ { 32, 16 }, { 4416, 3312 } }
>>> +		};
>>> +		pixelformats[pixelformat] = sizes;
>>> +	}
>>
>> I think a newline could be added here,
>>
>>> +	StreamFormats format(pixelformats);
>>> +	StreamConfiguration cfg(format);
>>
>> And probably here to aid readability.
>>
>>>  	cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);
>>>  	cfg.size = data->sensor_->resolution();
>>>  
>>> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
>>> index ea3d214..7e41378 100644
>>> --- a/src/libcamera/stream.cpp
>>> +++ b/src/libcamera/stream.cpp
>>> @@ -275,9 +275,9 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const
>>>   */
>>>  
>>>  /**
>>> - * \todo This method is deprecated and should be removed once all pipeline
>>> - * handlers provied StreamFormats.
>>> - */
>>> +* \todo This method is deprecated and should be removed once all pipeline
>>> +* handlers provied StreamFormats.
>>> +*/
>>
>> This looks like an unintentional change (just removing some whitespace
>> on the comment?), and shouldn't be in the patch.
> 
> Yes, I'll fix all the whitespace errors.
> 
>>
>> Once the pipeline handlers all provide StreamFormats, a patch on top
>> should remove this comment of course.
>>
>>>  StreamConfiguration::StreamConfiguration()
>>>  	: pixelFormat(0), stream_(nullptr)
>>>  {
>>>
>>
>> -- 
>> Regards
>> --
>> Kieran

-- 
Regards
--
Kieran