[libcamera-devel] [PATCH v2 1/4] libcamera: converter: a few fixes to ConverterFactoryBase documentation

[Andrey Konovalov]

Hi Jacopo,

Thank you for the detailed review!

On 21.09.2023 10:47, Jacopo Mondi wrote:
> Hi Andrey
> 
> On Wed, Sep 20, 2023 at 06:19:18PM +0300, Andrey Konovalov via libcamera-devel wrote:
>> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
>> ---
>>   src/libcamera/converter.cpp | 17 ++++++++++-------
>>   1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
>> index fa0f1ec8..466f8421 100644
>> --- a/src/libcamera/converter.cpp
>> +++ b/src/libcamera/converter.cpp
>> @@ -199,16 +199,18 @@ ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initiali
>>
>>   /**
>>    * \fn ConverterFactoryBase::compatibles()
>> - * \return The names compatibles
>> + * \return The compatibles
> 
> While I agree the "name compatibles" might not be super precise, just
> "the compatibles" doesn't sound any better to me.
> 
> The 'compatibles' are passed in as constructor parameters and are
> documented as
> 
>   * \param[in] compatibles Name aliases of the converter class
> 
> I would re-use part of this in
> 
> \return The list of compatible name aliases of the converter

Yes, this is much better.

>>    */
>>
>>   /**
>> - * \brief Create an instance of the converter corresponding to a named factory
>> - * \param[in] media Name of the factory
>> + * \brief Create an instance of the converter corresponding to the media device
> 
> The only thing 'media' is used for is to match on its 'driver()' name,
> so the previous documentation wasn't -that- off..

Indeed, the only place where the factory needs the "the whole" MediaDevice is when
it creates an instance of converter (as 'MediaDevice *' needs to be passed to the
Converter constructor).
But still describing an argument of 'MediaDevice *' type as "Name of the factory"
looks rather confusing to me.

>> + * \param[in] media the media device to create the converter for
>                         ^ The

Right. Will fix that.

>>    *
>>    * \return A unique pointer to a new instance of the converter subclass
>> - * corresponding to the named factory or one of its alias. Otherwise a null
>> - * pointer if no such factory exists
>> + * corresponding to the media device. The converter is created by the factory
>> + * the name or the alias of which equals to the media device driver name.
> 
> s/the name/name ?

No, I meant "the name of which"...

> I have troubles parsing this one, but I'm not a native speaker, so it
> might be me

... but this indeed is hard to get, and this part should be rephrased.

> Did you mean:
> 
>    * \return A unique pointer to a new instance of the converter
>    * subclass corresponding to the media device. The converter is
>    * created by matching the factory name or any of its compatible
>    * aliases with the media device driver name.

Yes, this sounds much more clear.

>> + * If the media device driver name doesn't match anything a null pointer is
>> + * returned.
>>    */
>>   std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice *media)
>>   {
>> @@ -236,10 +238,11 @@ std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice *media)
>>   }
>>
>>   /**
>> - * \brief Add a converter class to the registry
>> + * \brief Add a converter factory to the registry
>>    * \param[in] factory Factory to use to construct the converter class
>>    *
>> - * The caller is responsible to guarantee the uniqueness of the converter name.
>> + * The caller is responsible to guarantee the uniqueness of the converter
>> + * factory name.
> 
> These are ok

Thanks,
Andrey

>>    */
>>   void ConverterFactoryBase::registerType(ConverterFactoryBase *factory)
>>   {
>> --
>> 2.34.1
>>