[libcamera-devel] [PATCH v2 07/18] libcamera: software_isp: Add SwStats base class

[Hans de Goede]

Hi Jacopo,

On 2/12/24 17:08, Jacopo Mondi wrote:
> Hi Hans
> 
> On Thu, Feb 08, 2024 at 05:13:58PM +0100, Hans de Goede wrote:
>> Hi Jacopo,
>>
>> Thank you for the review.
>>
>> On 1/31/24 13:44, Jacopo Mondi wrote:

<snip>

>>>> +	/**
>>>> +	 * \brief The memory used per pixel in bits.
>>>
>>> Usually called 'bitdepth' in other parts of the library
>>
>> The way bitDepth is used in e.g. bayerFormat is different
>> from this though.
>>
>> This is not the number of significant bits per pixel,
>> this is the number of buts used in memory.
> 
> Thanks for the typo, made me laugh
> 
>>
>> So for e.g. bayerFormat.bitDepth = 10, this is 16 for
>> unpacked data (1 pixel per 16 bit memory word) and
>> 10 for packed data (4 pixels per 5 bytes of memory).
> 
> Oh, I see, so it's not the size on the wire, but the size in memory
> once unpacked.

Ack.

>> I'm open to a better name then bpp and also I'm open
>> to improve the brief, since I was hoping that the brief
>> explained (already) how this is different from e.g.
>> bayerFormat.bitDepth .
>>
> 
> Stupid question, will this ever be != 16 ?

Yes there are 10bpp packed in memory formats and this is actually
the first format both Andrey and I used for testing.

For these this bpp value is 10 and paternSize_.width =
set to 4 so that all sizes / x coords get rounded down
to a multiple of 4, which gives us 5 bytes bounderies
in memory. 

> You can expand the description by saying this value represents the
> size of the memory word where one pixel sample is stored

but is is not necessarily a wordsize, see the 10 bpp in
case of the packed fmt.

> in and
> explicitly state this is different from the raw image bitdepth (the
> bit size of the sample). Raw image bitdepth is what I tried to
say succinctly with "precision". Given the above I think that my:

    unsigned int bpp; /* Memory used per pixel, not precision */

is not that bad and I'm going to stick with that for v3.



> Quoting the v4l2 documentation for the memory RAW formats for
> more inspiration:
> 
>         These four pixel formats are raw sRGB / Bayer formats with 10
>         bits per sample. Each sample is stored in a 16-bit word, with
>         6 unused high bits filled with zeros. Each n-pixel row
>         contains n/2 green samples and n/2 blue or red samples, with
>         alternating red and blue rows. Bytes are stored in memory in
>         little endian order. They are conventionally described as
>         GRGR... BGBG..., RGRG... GBGB..., etc. Below is an example of
>         one of these formats:

Right so this described the unpacked in memory format there also is
a packed fmt...

<snip>

>>>> diff --git a/src/libcamera/software_isp/swstats.cpp b/src/libcamera/software_isp/swstats.cpp
>>>> new file mode 100644
>>>> index 00000000..e65a7ada
>>>> --- /dev/null
>>>> +++ b/src/libcamera/software_isp/swstats.cpp
>>>> @@ -0,0 +1,22 @@
>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>> +/*
>>>> + * Copyright (C) 2023, Linaro Ltd
>>>> + * Copyright (C) 2023, Red Hat Inc.
>>>> + *
>>>> + * Authors:
>>>> + * Hans de Goede <hdegoede at redhat.com>
>>>> + *
>>>> + * swstats.cpp - software statistics base class
>>>> + */
>>>> +
>>>> +#include "libcamera/internal/software_isp/swstats.h"
>>>> +
>>>> +namespace libcamera {
>>>> +
>>>> +LOG_DEFINE_CATEGORY(SwStats)
>>>> +
>>>> +SwStats::~SwStats()
>>>> +{
>>>> +}
>>>
>>> You'll need a .cpp file anyway for comments, but why having a lot of
>>> inline code in the header (which I would move to the .cpp file, as it
>>> doesn't depend on any template resolution) and have the empty
>>> constructor here ?
>>
>> Note this is an empty destructor not constructor and
>> the destructor is declared virtual (pure virtual even)
>> in the .h file.
> 
> yeah sorry, typo
> 
>>
>> So I did not expect to have to implement a destructor in the base-class
>> at all, but even with it being pure-virtual the derived SwStatsCpu class
>> destructor will chain up to the base-class destructor leading to a
>> linker error about that missing from the vtable for the base class
>> unless I add this.
>>
>> This might just be me being inexperienced with c++, so if there
>> is another way to solve this suggestions are welcome.
>>
> 
> My point was that most if not all functions implementation should go
> the .cpp file and not in the header. But maybe I missed something ?

Ah I see. That is a mostly valid point. Except for the processLine0()
and processLine2() methods which we want to keep inline for
performance reasons.

I'll move all of the non performance critical code to the .cpp file.

Regards,

Hans