[PATCH v2 10/18] libcamera: software_isp: Add DebayerCpu class

[Hans de Goede]

Hi Milan,

Thank you for the review I've addressed all your comments for v3.

On 1/23/24 22:23, Milan Zamazal wrote:
> Hans de Goede <hdegoede at redhat.com> writes:

<snip>

>> +int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>> +			  const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs)
>> +{
>> +	if (getInputConfig(inputCfg.pixelFormat, inputConfig_) != 0)
>> +		return -EINVAL;
>> +
>> +	if (stats_->configure(inputCfg) != 0)
>> +		return -EINVAL;
>> +
>> +	const Size &stats_pattern_size = stats_->patternSize();
>> +	if (inputConfig_.patternSize.width != stats_pattern_size.width ||
>> +	    inputConfig_.patternSize.height != stats_pattern_size.height) {
>> +		LOG(Debayer, Error)
>> +			<< "mismatching stats and debayer pattern sizes for "
>> +			<< inputCfg.pixelFormat.toString();
>> +		return -EINVAL;
>> +	}
>> +
>> +	inputConfig_.stride = inputCfg.stride;
>> +
>> +	if (outputCfgs.size() != 1) {
>> +		LOG(Debayer, Error)
>> +			<< "Unsupported number of output streams: "
>> +			<< outputCfgs.size();
>> +		return -EINVAL;
>> +	}
>> +
>> +	const StreamConfiguration &outputCfg = outputCfgs[0];
>> +	SizeRange outSizeRange = sizes(inputCfg.pixelFormat, inputCfg.size);
>> +	std::tie(outputConfig_.stride, outputConfig_.frameSize) =
>> 		strideAndFrameSize(outputCfg.pixelFormat, outputCfg.size);
>> +
>> +	if (!outSizeRange.contains(outputCfg.size) || outputConfig_.stride != outputCfg.stride) {
>> +		LOG(Debayer, Error)
>> +			<< "Invalid output size/stride: "
>> +			<< "\n  " << outputCfg.size << " (" << outSizeRange << ")"
>> +			<< "\n  " << outputCfg.stride << " (" << outputConfig_.stride << ")";
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (setDebayerFunctions(inputCfg.pixelFormat, outputCfg.pixelFormat) != 0)
>> +		return -EINVAL;
>> +
>> +	window_.x = ((inputCfg.size.width - outputCfg.size.width) / 2) &
>> +		    ~(inputConfig_.patternSize.width - 1);
>> +	window_.y = ((inputCfg.size.height - outputCfg.size.height) / 2) &
>> +		    ~(inputConfig_.patternSize.height - 1);
> 
> *Implicit* assumption that width and height are powers of 2?
> 
>> +	window_.width = outputCfg.size.width;
>> +	window_.height = outputCfg.size.height;
>> +
>> +	/* Don't pass x,y since process() already adjusts src before passing it */
>> +	stats_->setWindow(Rectangle(window_.size()));
>> +
>> +	for (unsigned int i = 0;
>> +	     i < (inputConfig_.patternSize.height + 1) && enableInputMemcpy_;
>> +	     i++) {
>> +		/* pad with patternSize.Width on both left and right side */
>> +		size_t lineLength = (window_.width + 2 * inputConfig_.patternSize.width) *
>> +				    inputConfig_.bpp / 8;
>> +
>> +		free(lineBuffers_[i]);
>> +		lineBuffers_[i] = (uint8_t *)malloc(lineLength);
>> +		if (!lineBuffers_[i])
>> +			return -ENOMEM;
> 
> What happens if malloc above fails and then free in the DebayerCpu destructor is
> called?

free(nullptr)

is a a no-op, so this scenario should not cause any problems.

Regards,

Hans