<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
</head>
<body>
<div style="color: rgb(33, 33, 33); background-color: rgb(255, 255, 255);" dir="auto">
Hi La<span>urent,</span></div>
<div style="color: rgb(33, 33, 33); background-color: rgb(255, 255, 255);" dir="auto">
<span><br>
</span></div>
<div style="color: rgb(33, 33, 33); background-color: rgb(255, 255, 255);" dir="auto">
<span>Yes, these changes do make sense and look good to me : )<span></span></span></div>
<div style="color: rgb(33, 33, 33); background-color: rgb(255, 255, 255);" dir="auto">
<span><br>
</span></div>
<div style="color: rgb(33, 33, 33); background-color: rgb(255, 255, 255);" dir="auto">
<span>Thanks for the explanations too! They're very useful to me.</span></div>
<div id="ms-outlook-mobile-signature" dir="auto">
<div><br>
</div>
</div>
<div id="mail-editor-reference-message-container" dir="auto"><br>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" style="font-size: 11pt;"><strong>From:</strong> Laurent Pinchart <laurent.pinchart@ideasonboard.com><br>
<strong>Sent:</strong> Sunday, March 5, 2023, 12:48 PM<br>
<strong>To:</strong> Suhrid Subramaniam <suhridsubramaniam@gmail.com><br>
<strong>Cc:</strong> libcamera-devel@lists.libcamera.org <libcamera-devel@lists.libcamera.org>; Suhrid Subramaniam <Suhrid.Subramaniam@mediatek.com><br>
<strong>Subject:</strong> Re: [libcamera-devel] [libcamera-devel, v3, 1/1] libcamera: converter: Check converter validity<br>
</div>
<br>
<meta name="Generator" content="Microsoft Exchange Server">
<!-- converted from text --><font size="2"><span style="font-size:11pt;">
<div class="PlainText">Hi Suhrid,<br>
<br>
Thank you for the patch.<br>
<br>
On Thu, Mar 02, 2023 at 11:06:29AM -0800, Suhrid Subramaniam via libcamera-devel wrote:<br>
> - In cases where ConverterFactoryBase::create returns a nullptr,<br>
> converter_->isValid() causes a segmentation fault.<br>
> <br>
> - Solve this by checking if converter_ is a nullptr.<br>
> <br>
> - Additionally, check for converter validity in the create function itself<br>
> and return a nullptr if the converter is invalid.<br>
<br>
The commit message can be improved by avoiding the bullet list:<br>
<br>
The ConverterFactoryBase::create() function returns a nullptr when no<br>
converter is found. The only caller, SimpleCameraData::init(), checks if<br>
the converter is valid with isValid(), but doesn't check if the pointer<br>
is null, which can lead to a crash.<br>
<br>
We could check both pointer validity and converter validity in the<br>
caller, but to limit the complexity in callers, it is better to check<br>
the converter validity in the create() function and return a null<br>
pointer when no valid converter is found.<br>
<br>
> Signed-off-by: Suhrid Subramaniam <suhrid.subramaniam@mediatek.com><br>
> ---<br>
> src/libcamera/converter.cpp | 3 ++-<br>
> src/libcamera/pipeline/simple/simple.cpp | 2 +-<br>
> 2 files changed, 3 insertions(+), 2 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp<br>
> index 3de39cff..38141da6 100644<br>
> --- a/src/libcamera/converter.cpp<br>
> +++ b/src/libcamera/converter.cpp<br>
> @@ -226,8 +226,9 @@ std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice *media)<br>
> << "Creating converter from "<br>
> << factory->name_ << " factory with "<br>
> << (it == compatibles.end() ? "no" : media->driver()) << " alias.";<br>
> + std::unique_ptr<Converter> converter_ = factory->createInstance(media);<br>
<br>
The variable should be named 'converter' as it's a local variable. We<br>
use trailing underscores for class members only.<br>
<br>
> <br>
> - return factory->createInstance(media);<br>
> + return converter_->isValid() ? std::move(converter_) : nullptr;<br>
<br>
The std::move() can prevent return value optimization. It would be best<br>
to write<br>
<br>
if (!converter_->isValid())<br>
return nullptr;<br>
<br>
return converter_;<br>
<br>
We could even keep iterating over the other factories:<br>
<br>
if (converter_->isValid())<br>
return converter;<br>
<br>
It may not be very useful at this point, but it wouldn't hurt and could<br>
possibly help later.<br>
<br>
If you're fine with these changes, there's no need to submit a new<br>
version, I can apply them locally.<br>
<br>
> }<br>
> <br>
> return nullptr;<br>
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp<br>
> index 24ded4db..2423ec10 100644<br>
> --- a/src/libcamera/pipeline/simple/simple.cpp<br>
> +++ b/src/libcamera/pipeline/simple/simple.cpp<br>
> @@ -493,7 +493,7 @@ int SimpleCameraData::init()<br>
> MediaDevice *converter = pipe->converter();<br>
> if (converter) {<br>
> converter_ = ConverterFactoryBase::create(converter);<br>
> - if (!converter_->isValid()) {<br>
> + if (!converter_) {<br>
> LOG(SimplePipeline, Warning)<br>
> << "Failed to create converter, disabling format conversion";<br>
> converter_.reset();<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</div>
</span></font><br>
</div>
</body>
</html>
<!--type:text--><!--{--><pre>************* MEDIATEK Confidentiality Notice ********************
The information contained in this e-mail message (including any
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be
conveyed only to the designated recipient(s). Any use, dissemination,
distribution, printing, retaining or copying of this e-mail (including its
attachments) by unintended recipient(s) is strictly prohibited and may
be unlawful. If you are not an intended recipient of this e-mail, or believe
that you have received this e-mail in error, please notify the sender
immediately (by replying to this e-mail), delete any and all copies of
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
</pre><!--}-->