[libcamera-devel,RFC,0/2] libcamera: converter: introduce software converter for debayering and AWB

18924 mbox series

Here is a draft implementation of Bayer demosaicing which follows the
Converter interface and runs on CPU.

It also includes a naive version of Grey World AWB. Just for demo purposes
(to make the final image looking a bit nicer). Otherwise, Converter isn't
the right place for AWB - only the statistics should be gathered here,
and the rest belongs to an IPA module.

Currently this software converter supports single output only, but adding
the second stream for statistics is under consideration.

As libcamera::Converter currently assumes a media device underneath the
convertor, I wasn't able to avoid hacking the simple pipeline handler to make
it work with the software converter. For the same reason ConverterFactory
is not used for now.

Only RAW10P format from the sensor is currently supported, but adding more
Bayer formats wouldn't be a problem. Out of 10 bits, only 8 most significant
ones are used to lessen the load on CPU. Simple bilinear interpolation is
used for the same reason.

AWB simplifications:
- a naive implementation of "Grey World" algorithm: all pixels are used (no
  brightest and darkest pixels excluded from the calculations)
- to lessen the load on CPU, works on raw Bayer data and takes red values from
  red pixels, blue values from blue, and green values from green pixels only. 
- to lessen the load on CPU, the red/green/blue gains calculated from the
  current frame data are applied to the next frame. These gains are purely
  in software (no V4L2 controls are set).

No performance analysis or tuning have been done yet. On RB5 board this
software convertor gives:
  ~ 5 fps at 3278x2462 (camera sensor runs at 15 fps)
  ~ 18..19 fps at 1918x1078 (out of 30 fps)
  ~ 18..19 fps at 1638x1230 (out of 30 fps)
  ~ 30 fps at 638x478 (out of 30 fps)
(The resolutions above are the output ones; demosaic filter drops 1 pixel
from each side of the frame, so that 3280x2464 from the camera sensor becomes
3278x2462 etc)

Andrey Konovalov (2):
  libcamera: converter: add software converter
  [DNI] libcamera: pipeline: simple: a hack to use sotware converter
    with qcom-camss

 .../internal/converter/converter_softw.h      |  90 ++++
 src/libcamera/converter/converter_softw.cpp   | 430 ++++++++++++++++++
 src/libcamera/converter/meson.build           |   3 +-
 src/libcamera/pipeline/simple/simple.cpp      |  22 +-
 4 files changed, 542 insertions(+), 3 deletions(-)
 create mode 100644 include/libcamera/internal/converter/converter_softw.h
 create mode 100644 src/libcamera/converter/converter_softw.cpp

Hi Andrey,

Thank you for this work.

I'm very interested by this series, mostly because I would like to add
YUYV -> NV12 pixel format conversion in this same class (to simplify
some Android use cases I'm working on).

I'm not familiar enough with the libcamera codebase to judge how well
this is implemented but I'm looking forward to see comments on this patchset.

On dim., août 06, 2023 at 21:01, Andrey Konovalov via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote:

> Here is a draft implementation of Bayer demosaicing which follows the
> Converter interface and runs on CPU.
>
> It also includes a naive version of Grey World AWB. Just for demo purposes
> (to make the final image looking a bit nicer). Otherwise, Converter isn't
> the right place for AWB - only the statistics should be gathered here,
> and the rest belongs to an IPA module.
>
> Currently this software converter supports single output only, but adding
> the second stream for statistics is under consideration.
>
> As libcamera::Converter currently assumes a media device underneath the
> convertor, I wasn't able to avoid hacking the simple pipeline handler to make
> it work with the software converter. For the same reason ConverterFactory
> is not used for now.
>
> Only RAW10P format from the sensor is currently supported, but adding more
> Bayer formats wouldn't be a problem. Out of 10 bits, only 8 most significant
> ones are used to lessen the load on CPU. Simple bilinear interpolation is
> used for the same reason.
>
> AWB simplifications:
> - a naive implementation of "Grey World" algorithm: all pixels are used (no
>   brightest and darkest pixels excluded from the calculations)
> - to lessen the load on CPU, works on raw Bayer data and takes red values from
>   red pixels, blue values from blue, and green values from green pixels only. 
> - to lessen the load on CPU, the red/green/blue gains calculated from the
>   current frame data are applied to the next frame. These gains are purely
>   in software (no V4L2 controls are set).
>
> No performance analysis or tuning have been done yet. On RB5 board this
> software convertor gives:
>   ~ 5 fps at 3278x2462 (camera sensor runs at 15 fps)
>   ~ 18..19 fps at 1918x1078 (out of 30 fps)
>   ~ 18..19 fps at 1638x1230 (out of 30 fps)
>   ~ 30 fps at 638x478 (out of 30 fps)
> (The resolutions above are the output ones; demosaic filter drops 1 pixel
> from each side of the frame, so that 3280x2464 from the camera sensor becomes
> 3278x2462 etc)

Could you detail a bit how this was tested? Did you validate this using
"cam" ?

Also, is there a reason for testing this with real hardware?

Can't this be validated/developped upon the Virtual Media Controller
Driver (vimc) ?

https://docs.kernel.org/admin-guide/media/vimc.html

>
> Andrey Konovalov (2):
>   libcamera: converter: add software converter
>   [DNI] libcamera: pipeline: simple: a hack to use sotware converter
>     with qcom-camss
>
>  .../internal/converter/converter_softw.h      |  90 ++++
>  src/libcamera/converter/converter_softw.cpp   | 430 ++++++++++++++++++
>  src/libcamera/converter/meson.build           |   3 +-
>  src/libcamera/pipeline/simple/simple.cpp      |  22 +-
>  4 files changed, 542 insertions(+), 3 deletions(-)
>  create mode 100644 include/libcamera/internal/converter/converter_softw.h
>  create mode 100644 src/libcamera/converter/converter_softw.cpp
>
> -- 
> 2.34.1

Hi Mattijs,

On 30.08.2023 14:30, Mattijs Korpershoek wrote:
> Hi Andrey,
> 
> Thank you for this work.
> 
> I'm very interested by this series, mostly because I would like to add
> YUYV -> NV12 pixel format conversion in this same class (to simplify
> some Android use cases I'm working on).

So you only need wider choice of format conversions supported. That's fine.

My use case is different in that I'd also like the software Converter to calculate
some stats (and it would probably make sense to add a separate output in the converter
for that). Need to think if stats calculations should be made optional (configurable)
in the software converter, or if it makes more sense to have a software Converter with
the stats and a separate one w/o the stats.

The thing is that in the current implementation for performance reasons debayering and
the stats calculations are done in one single pass (vs going through all the pixel twice:
first for the debayering, and then to collect the stats). So making the stats calculations
conditional would have significant effect on the performance. But the current implementation
might change in the future. E.g. the set of pixels used for stats calculations could be
not the whole pixel array.

Thanks,
Andrey

> I'm not familiar enough with the libcamera codebase to judge how well
> this is implemented but I'm looking forward to see comments on this patchset.
> 
> On dim., août 06, 2023 at 21:01, Andrey Konovalov via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote:
> 
>> Here is a draft implementation of Bayer demosaicing which follows the
>> Converter interface and runs on CPU.
>>
>> It also includes a naive version of Grey World AWB. Just for demo purposes
>> (to make the final image looking a bit nicer). Otherwise, Converter isn't
>> the right place for AWB - only the statistics should be gathered here,
>> and the rest belongs to an IPA module.
>>
>> Currently this software converter supports single output only, but adding
>> the second stream for statistics is under consideration.
>>
>> As libcamera::Converter currently assumes a media device underneath the
>> convertor, I wasn't able to avoid hacking the simple pipeline handler to make
>> it work with the software converter. For the same reason ConverterFactory
>> is not used for now.
>>
>> Only RAW10P format from the sensor is currently supported, but adding more
>> Bayer formats wouldn't be a problem. Out of 10 bits, only 8 most significant
>> ones are used to lessen the load on CPU. Simple bilinear interpolation is
>> used for the same reason.
>>
>> AWB simplifications:
>> - a naive implementation of "Grey World" algorithm: all pixels are used (no
>>    brightest and darkest pixels excluded from the calculations)
>> - to lessen the load on CPU, works on raw Bayer data and takes red values from
>>    red pixels, blue values from blue, and green values from green pixels only.
>> - to lessen the load on CPU, the red/green/blue gains calculated from the
>>    current frame data are applied to the next frame. These gains are purely
>>    in software (no V4L2 controls are set).
>>
>> No performance analysis or tuning have been done yet. On RB5 board this
>> software convertor gives:
>>    ~ 5 fps at 3278x2462 (camera sensor runs at 15 fps)
>>    ~ 18..19 fps at 1918x1078 (out of 30 fps)
>>    ~ 18..19 fps at 1638x1230 (out of 30 fps)
>>    ~ 30 fps at 638x478 (out of 30 fps)
>> (The resolutions above are the output ones; demosaic filter drops 1 pixel
>> from each side of the frame, so that 3280x2464 from the camera sensor becomes
>> 3278x2462 etc)
> 
> Could you detail a bit how this was tested? Did you validate this using
> "cam" ?
> 
> Also, is there a reason for testing this with real hardware?
> 
> Can't this be validated/developped upon the Virtual Media Controller
> Driver (vimc) ?
> 
> https://docs.kernel.org/admin-guide/media/vimc.html
> 
>>
>> Andrey Konovalov (2):
>>    libcamera: converter: add software converter
>>    [DNI] libcamera: pipeline: simple: a hack to use sotware converter
>>      with qcom-camss
>>
>>   .../internal/converter/converter_softw.h      |  90 ++++
>>   src/libcamera/converter/converter_softw.cpp   | 430 ++++++++++++++++++
>>   src/libcamera/converter/meson.build           |   3 +-
>>   src/libcamera/pipeline/simple/simple.cpp      |  22 +-
>>   4 files changed, 542 insertions(+), 3 deletions(-)
>>   create mode 100644 include/libcamera/internal/converter/converter_softw.h
>>   create mode 100644 src/libcamera/converter/converter_softw.cpp
>>
>> -- 
>> 2.34.1

Hi Mattijs,

On Wed, Aug 30, 2023 at 01:30:23PM +0200, Mattijs Korpershoek via libcamera-devel wrote:
> Hi Andrey,
>
> Thank you for this work.
>
> I'm very interested by this series, mostly because I would like to add
> YUYV -> NV12 pixel format conversion in this same class (to simplify
> some Android use cases I'm working on).

Have you considered using libYUV for such task ? It also feels this
can be contained in the Android HAL layer where we already use libYUV ?

Thanks
   j

>
> I'm not familiar enough with the libcamera codebase to judge how well
> this is implemented but I'm looking forward to see comments on this patchset.
>
> On dim., août 06, 2023 at 21:01, Andrey Konovalov via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote:
>
> > Here is a draft implementation of Bayer demosaicing which follows the
> > Converter interface and runs on CPU.
> >
> > It also includes a naive version of Grey World AWB. Just for demo purposes
> > (to make the final image looking a bit nicer). Otherwise, Converter isn't
> > the right place for AWB - only the statistics should be gathered here,
> > and the rest belongs to an IPA module.
> >
> > Currently this software converter supports single output only, but adding
> > the second stream for statistics is under consideration.
> >
> > As libcamera::Converter currently assumes a media device underneath the
> > convertor, I wasn't able to avoid hacking the simple pipeline handler to make
> > it work with the software converter. For the same reason ConverterFactory
> > is not used for now.
> >
> > Only RAW10P format from the sensor is currently supported, but adding more
> > Bayer formats wouldn't be a problem. Out of 10 bits, only 8 most significant
> > ones are used to lessen the load on CPU. Simple bilinear interpolation is
> > used for the same reason.
> >
> > AWB simplifications:
> > - a naive implementation of "Grey World" algorithm: all pixels are used (no
> >   brightest and darkest pixels excluded from the calculations)
> > - to lessen the load on CPU, works on raw Bayer data and takes red values from
> >   red pixels, blue values from blue, and green values from green pixels only.
> > - to lessen the load on CPU, the red/green/blue gains calculated from the
> >   current frame data are applied to the next frame. These gains are purely
> >   in software (no V4L2 controls are set).
> >
> > No performance analysis or tuning have been done yet. On RB5 board this
> > software convertor gives:
> >   ~ 5 fps at 3278x2462 (camera sensor runs at 15 fps)
> >   ~ 18..19 fps at 1918x1078 (out of 30 fps)
> >   ~ 18..19 fps at 1638x1230 (out of 30 fps)
> >   ~ 30 fps at 638x478 (out of 30 fps)
> > (The resolutions above are the output ones; demosaic filter drops 1 pixel
> > from each side of the frame, so that 3280x2464 from the camera sensor becomes
> > 3278x2462 etc)
>
> Could you detail a bit how this was tested? Did you validate this using
> "cam" ?
>
> Also, is there a reason for testing this with real hardware?
>
> Can't this be validated/developped upon the Virtual Media Controller
> Driver (vimc) ?
>
> https://docs.kernel.org/admin-guide/media/vimc.html
>
> >
> > Andrey Konovalov (2):
> >   libcamera: converter: add software converter
> >   [DNI] libcamera: pipeline: simple: a hack to use sotware converter
> >     with qcom-camss
> >
> >  .../internal/converter/converter_softw.h      |  90 ++++
> >  src/libcamera/converter/converter_softw.cpp   | 430 ++++++++++++++++++
> >  src/libcamera/converter/meson.build           |   3 +-
> >  src/libcamera/pipeline/simple/simple.cpp      |  22 +-
> >  4 files changed, 542 insertions(+), 3 deletions(-)
> >  create mode 100644 include/libcamera/internal/converter/converter_softw.h
> >  create mode 100644 src/libcamera/converter/converter_softw.cpp
> >
> > --
> > 2.34.1

Hi Jacopo,

On mer., août 30, 2023 at 16:38, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:

> Hi Mattijs,
>
> On Wed, Aug 30, 2023 at 01:30:23PM +0200, Mattijs Korpershoek via libcamera-devel wrote:
>> Hi Andrey,
>>
>> Thank you for this work.
>>
>> I'm very interested by this series, mostly because I would like to add
>> YUYV -> NV12 pixel format conversion in this same class (to simplify
>> some Android use cases I'm working on).
>
> Have you considered using libYUV for such task ? It also feels this
> can be contained in the Android HAL layer where we already use libYUV ?

Yes, using libYUV is what I plan to do for this.

I will look into doing this only in the Android HAL layer but it feels
easier to plumb this conversion as a Converter class.

>
> Thanks
>    j
>
>>
>> I'm not familiar enough with the libcamera codebase to judge how well
>> this is implemented but I'm looking forward to see comments on this patchset.
>>
>> On dim., août 06, 2023 at 21:01, Andrey Konovalov via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote:
>>
>> > Here is a draft implementation of Bayer demosaicing which follows the
>> > Converter interface and runs on CPU.
>> >
>> > It also includes a naive version of Grey World AWB. Just for demo purposes
>> > (to make the final image looking a bit nicer). Otherwise, Converter isn't
>> > the right place for AWB - only the statistics should be gathered here,
>> > and the rest belongs to an IPA module.
>> >
>> > Currently this software converter supports single output only, but adding
>> > the second stream for statistics is under consideration.
>> >
>> > As libcamera::Converter currently assumes a media device underneath the
>> > convertor, I wasn't able to avoid hacking the simple pipeline handler to make
>> > it work with the software converter. For the same reason ConverterFactory
>> > is not used for now.
>> >
>> > Only RAW10P format from the sensor is currently supported, but adding more
>> > Bayer formats wouldn't be a problem. Out of 10 bits, only 8 most significant
>> > ones are used to lessen the load on CPU. Simple bilinear interpolation is
>> > used for the same reason.
>> >
>> > AWB simplifications:
>> > - a naive implementation of "Grey World" algorithm: all pixels are used (no
>> >   brightest and darkest pixels excluded from the calculations)
>> > - to lessen the load on CPU, works on raw Bayer data and takes red values from
>> >   red pixels, blue values from blue, and green values from green pixels only.
>> > - to lessen the load on CPU, the red/green/blue gains calculated from the
>> >   current frame data are applied to the next frame. These gains are purely
>> >   in software (no V4L2 controls are set).
>> >
>> > No performance analysis or tuning have been done yet. On RB5 board this
>> > software convertor gives:
>> >   ~ 5 fps at 3278x2462 (camera sensor runs at 15 fps)
>> >   ~ 18..19 fps at 1918x1078 (out of 30 fps)
>> >   ~ 18..19 fps at 1638x1230 (out of 30 fps)
>> >   ~ 30 fps at 638x478 (out of 30 fps)
>> > (The resolutions above are the output ones; demosaic filter drops 1 pixel
>> > from each side of the frame, so that 3280x2464 from the camera sensor becomes
>> > 3278x2462 etc)
>>
>> Could you detail a bit how this was tested? Did you validate this using
>> "cam" ?
>>
>> Also, is there a reason for testing this with real hardware?
>>
>> Can't this be validated/developped upon the Virtual Media Controller
>> Driver (vimc) ?
>>
>> https://docs.kernel.org/admin-guide/media/vimc.html
>>
>> >
>> > Andrey Konovalov (2):
>> >   libcamera: converter: add software converter
>> >   [DNI] libcamera: pipeline: simple: a hack to use sotware converter
>> >     with qcom-camss
>> >
>> >  .../internal/converter/converter_softw.h      |  90 ++++
>> >  src/libcamera/converter/converter_softw.cpp   | 430 ++++++++++++++++++
>> >  src/libcamera/converter/meson.build           |   3 +-
>> >  src/libcamera/pipeline/simple/simple.cpp      |  22 +-
>> >  4 files changed, 542 insertions(+), 3 deletions(-)
>> >  create mode 100644 include/libcamera/internal/converter/converter_softw.h
>> >  create mode 100644 src/libcamera/converter/converter_softw.cpp
>> >
>> > --
>> > 2.34.1

Hi Mattijs,

Sorry, I've missed the second part of you email.

On 30.08.2023 14:30, Mattijs Korpershoek wrote:
> Hi Andrey,
> 
> Thank you for this work.
> 
> I'm very interested by this series, mostly because I would like to add
> YUYV -> NV12 pixel format conversion in this same class (to simplify
> some Android use cases I'm working on).
> 
> I'm not familiar enough with the libcamera codebase to judge how well
> this is implemented but I'm looking forward to see comments on this patchset.
> 
> On dim., août 06, 2023 at 21:01, Andrey Konovalov via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote:
> 
>> Here is a draft implementation of Bayer demosaicing which follows the
>> Converter interface and runs on CPU.
>>
>> It also includes a naive version of Grey World AWB. Just for demo purposes
>> (to make the final image looking a bit nicer). Otherwise, Converter isn't
>> the right place for AWB - only the statistics should be gathered here,
>> and the rest belongs to an IPA module.
>>
>> Currently this software converter supports single output only, but adding
>> the second stream for statistics is under consideration.
>>
>> As libcamera::Converter currently assumes a media device underneath the
>> convertor, I wasn't able to avoid hacking the simple pipeline handler to make
>> it work with the software converter. For the same reason ConverterFactory
>> is not used for now.
>>
>> Only RAW10P format from the sensor is currently supported, but adding more
>> Bayer formats wouldn't be a problem. Out of 10 bits, only 8 most significant
>> ones are used to lessen the load on CPU. Simple bilinear interpolation is
>> used for the same reason.
>>
>> AWB simplifications:
>> - a naive implementation of "Grey World" algorithm: all pixels are used (no
>>    brightest and darkest pixels excluded from the calculations)
>> - to lessen the load on CPU, works on raw Bayer data and takes red values from
>>    red pixels, blue values from blue, and green values from green pixels only.
>> - to lessen the load on CPU, the red/green/blue gains calculated from the
>>    current frame data are applied to the next frame. These gains are purely
>>    in software (no V4L2 controls are set).
>>
>> No performance analysis or tuning have been done yet. On RB5 board this
>> software convertor gives:
>>    ~ 5 fps at 3278x2462 (camera sensor runs at 15 fps)
>>    ~ 18..19 fps at 1918x1078 (out of 30 fps)
>>    ~ 18..19 fps at 1638x1230 (out of 30 fps)
>>    ~ 30 fps at 638x478 (out of 30 fps)
>> (The resolutions above are the output ones; demosaic filter drops 1 pixel
>> from each side of the frame, so that 3280x2464 from the camera sensor becomes
>> 3278x2462 etc)
> 
> Could you detail a bit how this was tested? Did you validate this using
> "cam" ?

I've used cam, but mostly qcam.

> Also, is there a reason for testing this with real hardware?
> 
> Can't this be validated/developped upon the Virtual Media Controller
> Driver (vimc) ?
> 
> https://docs.kernel.org/admin-guide/media/vimc.html

The format conversion part can be tested without real hardware.
Using vimc should be possible, but updating the pipeline handler would be required.
vimc uses its own pipeline handler, and it doesn't support the Converter class.

Thanks,
Andrey

>> Andrey Konovalov (2):
>>    libcamera: converter: add software converter
>>    [DNI] libcamera: pipeline: simple: a hack to use sotware converter
>>      with qcom-camss
>>
>>   .../internal/converter/converter_softw.h      |  90 ++++
>>   src/libcamera/converter/converter_softw.cpp   | 430 ++++++++++++++++++
>>   src/libcamera/converter/meson.build           |   3 +-
>>   src/libcamera/pipeline/simple/simple.cpp      |  22 +-
>>   4 files changed, 542 insertions(+), 3 deletions(-)
>>   create mode 100644 include/libcamera/internal/converter/converter_softw.h
>>   create mode 100644 src/libcamera/converter/converter_softw.cpp
>>
>> -- 
>> 2.34.1

On mer., août 30, 2023 at 17:01, Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote:

> Hi Jacopo,
>
> On mer., août 30, 2023 at 16:38, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
>> Hi Mattijs,
>>
>> On Wed, Aug 30, 2023 at 01:30:23PM +0200, Mattijs Korpershoek via libcamera-devel wrote:
>>> Hi Andrey,
>>>
>>> Thank you for this work.
>>>
>>> I'm very interested by this series, mostly because I would like to add
>>> YUYV -> NV12 pixel format conversion in this same class (to simplify
>>> some Android use cases I'm working on).
>>
>> Have you considered using libYUV for such task ? It also feels this
>> can be contained in the Android HAL layer where we already use libYUV ?
>
> Yes, using libYUV is what I plan to do for this.
>
> I will look into doing this only in the Android HAL layer but it feels
> easier to plumb this conversion as a Converter class.

I've given this some more thought.

If I would implement this as a Converter class but contain it in the
Android HAL layer, simple pipeline won't be able to use it (because we don't want
libcamera.so to depend on libcamera-hal.so)

Some options I see:

1. keep converter_sw as part of libcamera.so and improve it with libyuv conversion
   -> adds libyuv dependency, which could be made optional

2. add an android specific pipeline in libcamera-hal.so which would be
   based on the simple pipeline.

1. Sounds more maintainable in the long run to me.

Andrey, Bryan, Srinivas, I know this discussion is a bit out of scope of
the initial patch. If this generates too much noise for you let me know
if I should create another thread/drop you from this one.

>
>>
>> Thanks
>>    j
>>
>>>
>>> I'm not familiar enough with the libcamera codebase to judge how well
>>> this is implemented but I'm looking forward to see comments on this patchset.
>>>
>>> On dim., août 06, 2023 at 21:01, Andrey Konovalov via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote:
>>>
>>> > Here is a draft implementation of Bayer demosaicing which follows the
>>> > Converter interface and runs on CPU.
>>> >
>>> > It also includes a naive version of Grey World AWB. Just for demo purposes
>>> > (to make the final image looking a bit nicer). Otherwise, Converter isn't
>>> > the right place for AWB - only the statistics should be gathered here,
>>> > and the rest belongs to an IPA module.
>>> >
>>> > Currently this software converter supports single output only, but adding
>>> > the second stream for statistics is under consideration.
>>> >
>>> > As libcamera::Converter currently assumes a media device underneath the
>>> > convertor, I wasn't able to avoid hacking the simple pipeline handler to make
>>> > it work with the software converter. For the same reason ConverterFactory
>>> > is not used for now.
>>> >
>>> > Only RAW10P format from the sensor is currently supported, but adding more
>>> > Bayer formats wouldn't be a problem. Out of 10 bits, only 8 most significant
>>> > ones are used to lessen the load on CPU. Simple bilinear interpolation is
>>> > used for the same reason.
>>> >
>>> > AWB simplifications:
>>> > - a naive implementation of "Grey World" algorithm: all pixels are used (no
>>> >   brightest and darkest pixels excluded from the calculations)
>>> > - to lessen the load on CPU, works on raw Bayer data and takes red values from
>>> >   red pixels, blue values from blue, and green values from green pixels only.
>>> > - to lessen the load on CPU, the red/green/blue gains calculated from the
>>> >   current frame data are applied to the next frame. These gains are purely
>>> >   in software (no V4L2 controls are set).
>>> >
>>> > No performance analysis or tuning have been done yet. On RB5 board this
>>> > software convertor gives:
>>> >   ~ 5 fps at 3278x2462 (camera sensor runs at 15 fps)
>>> >   ~ 18..19 fps at 1918x1078 (out of 30 fps)
>>> >   ~ 18..19 fps at 1638x1230 (out of 30 fps)
>>> >   ~ 30 fps at 638x478 (out of 30 fps)
>>> > (The resolutions above are the output ones; demosaic filter drops 1 pixel
>>> > from each side of the frame, so that 3280x2464 from the camera sensor becomes
>>> > 3278x2462 etc)
>>>
>>> Could you detail a bit how this was tested? Did you validate this using
>>> "cam" ?
>>>
>>> Also, is there a reason for testing this with real hardware?
>>>
>>> Can't this be validated/developped upon the Virtual Media Controller
>>> Driver (vimc) ?
>>>
>>> https://docs.kernel.org/admin-guide/media/vimc.html
>>>
>>> >
>>> > Andrey Konovalov (2):
>>> >   libcamera: converter: add software converter
>>> >   [DNI] libcamera: pipeline: simple: a hack to use sotware converter
>>> >     with qcom-camss
>>> >
>>> >  .../internal/converter/converter_softw.h      |  90 ++++
>>> >  src/libcamera/converter/converter_softw.cpp   | 430 ++++++++++++++++++
>>> >  src/libcamera/converter/meson.build           |   3 +-
>>> >  src/libcamera/pipeline/simple/simple.cpp      |  22 +-
>>> >  4 files changed, 542 insertions(+), 3 deletions(-)
>>> >  create mode 100644 include/libcamera/internal/converter/converter_softw.h
>>> >  create mode 100644 src/libcamera/converter/converter_softw.cpp
>>> >
>>> > --
>>> > 2.34.1

Hi Mattijs

On Tue, Sep 05, 2023 at 04:01:41PM +0200, Mattijs Korpershoek wrote:
> On mer., août 30, 2023 at 17:01, Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote:
>
> > Hi Jacopo,
> >
> > On mer., août 30, 2023 at 16:38, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
> >
> >> Hi Mattijs,
> >>
> >> On Wed, Aug 30, 2023 at 01:30:23PM +0200, Mattijs Korpershoek via libcamera-devel wrote:
> >>> Hi Andrey,
> >>>
> >>> Thank you for this work.
> >>>
> >>> I'm very interested by this series, mostly because I would like to add
> >>> YUYV -> NV12 pixel format conversion in this same class (to simplify
> >>> some Android use cases I'm working on).
> >>
> >> Have you considered using libYUV for such task ? It also feels this
> >> can be contained in the Android HAL layer where we already use libYUV ?
> >
> > Yes, using libYUV is what I plan to do for this.
> >
> > I will look into doing this only in the Android HAL layer but it feels
> > easier to plumb this conversion as a Converter class.
>
> I've given this some more thought.
>
> If I would implement this as a Converter class but contain it in the
> Android HAL layer, simple pipeline won't be able to use it (because we don't want
> libcamera.so to depend on libcamera-hal.so)
>
> Some options I see:
>
> 1. keep converter_sw as part of libcamera.so and improve it with libyuv conversion
>    -> adds libyuv dependency, which could be made optional
>
> 2. add an android specific pipeline in libcamera-hal.so which would be
>    based on the simple pipeline.
>
> 1. Sounds more maintainable in the long run to me.
>
> Andrey, Bryan, Srinivas, I know this discussion is a bit out of scope of
> the initial patch. If this generates too much noise for you let me know
> if I should create another thread/drop you from this one.
>

In my head these are two rather distinct things. The softISP
implementation does basically provide an IPA module, which generates
and consume statistics to calculate the parameters used to tune the
(software) image processing. It is plugged to the simple pipeline
handler as a Converter and, when it comes to format conversion will
likely do RAW->YUV through debayering and color space conversion.

Your use case seems very similar to what is already implemented as the
PostProcessor class in the Android HAL, which so far only duplicates
YUV streams but could be expanded to support format conversion between
YUV formats. A long time has passed since last time I worked on the
HAL and I might be missing something of course.

IOW I don't see the softISP dealing with YUV-to-YUV format conversion.

If you really want to explore the Converter class and add a plugin to
the Simple pipeline that produces NV12 when your system can only do
YUYV, I feel like this would be a different effort than this one ?

> >
> >>
> >> Thanks
> >>    j
> >>
> >>>
> >>> I'm not familiar enough with the libcamera codebase to judge how well
> >>> this is implemented but I'm looking forward to see comments on this patchset.
> >>>
> >>> On dim., août 06, 2023 at 21:01, Andrey Konovalov via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote:
> >>>
> >>> > Here is a draft implementation of Bayer demosaicing which follows the
> >>> > Converter interface and runs on CPU.
> >>> >
> >>> > It also includes a naive version of Grey World AWB. Just for demo purposes
> >>> > (to make the final image looking a bit nicer). Otherwise, Converter isn't
> >>> > the right place for AWB - only the statistics should be gathered here,
> >>> > and the rest belongs to an IPA module.
> >>> >
> >>> > Currently this software converter supports single output only, but adding
> >>> > the second stream for statistics is under consideration.
> >>> >
> >>> > As libcamera::Converter currently assumes a media device underneath the
> >>> > convertor, I wasn't able to avoid hacking the simple pipeline handler to make
> >>> > it work with the software converter. For the same reason ConverterFactory
> >>> > is not used for now.
> >>> >
> >>> > Only RAW10P format from the sensor is currently supported, but adding more
> >>> > Bayer formats wouldn't be a problem. Out of 10 bits, only 8 most significant
> >>> > ones are used to lessen the load on CPU. Simple bilinear interpolation is
> >>> > used for the same reason.
> >>> >
> >>> > AWB simplifications:
> >>> > - a naive implementation of "Grey World" algorithm: all pixels are used (no
> >>> >   brightest and darkest pixels excluded from the calculations)
> >>> > - to lessen the load on CPU, works on raw Bayer data and takes red values from
> >>> >   red pixels, blue values from blue, and green values from green pixels only.
> >>> > - to lessen the load on CPU, the red/green/blue gains calculated from the
> >>> >   current frame data are applied to the next frame. These gains are purely
> >>> >   in software (no V4L2 controls are set).
> >>> >
> >>> > No performance analysis or tuning have been done yet. On RB5 board this
> >>> > software convertor gives:
> >>> >   ~ 5 fps at 3278x2462 (camera sensor runs at 15 fps)
> >>> >   ~ 18..19 fps at 1918x1078 (out of 30 fps)
> >>> >   ~ 18..19 fps at 1638x1230 (out of 30 fps)
> >>> >   ~ 30 fps at 638x478 (out of 30 fps)
> >>> > (The resolutions above are the output ones; demosaic filter drops 1 pixel
> >>> > from each side of the frame, so that 3280x2464 from the camera sensor becomes
> >>> > 3278x2462 etc)
> >>>
> >>> Could you detail a bit how this was tested? Did you validate this using
> >>> "cam" ?
> >>>
> >>> Also, is there a reason for testing this with real hardware?
> >>>
> >>> Can't this be validated/developped upon the Virtual Media Controller
> >>> Driver (vimc) ?
> >>>
> >>> https://docs.kernel.org/admin-guide/media/vimc.html
> >>>
> >>> >
> >>> > Andrey Konovalov (2):
> >>> >   libcamera: converter: add software converter
> >>> >   [DNI] libcamera: pipeline: simple: a hack to use sotware converter
> >>> >     with qcom-camss
> >>> >
> >>> >  .../internal/converter/converter_softw.h      |  90 ++++
> >>> >  src/libcamera/converter/converter_softw.cpp   | 430 ++++++++++++++++++
> >>> >  src/libcamera/converter/meson.build           |   3 +-
> >>> >  src/libcamera/pipeline/simple/simple.cpp      |  22 +-
> >>> >  4 files changed, 542 insertions(+), 3 deletions(-)
> >>> >  create mode 100644 include/libcamera/internal/converter/converter_softw.h
> >>> >  create mode 100644 src/libcamera/converter/converter_softw.cpp
> >>> >
> >>> > --
> >>> > 2.34.1

Hi Jacopo,

On mar., sept. 05, 2023 at 17:10, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:

> Hi Mattijs
>
> On Tue, Sep 05, 2023 at 04:01:41PM +0200, Mattijs Korpershoek wrote:
>> On mer., août 30, 2023 at 17:01, Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote:
>>
>> > Hi Jacopo,
>> >
>> > On mer., août 30, 2023 at 16:38, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>> >
>> >> Hi Mattijs,
>> >>
>> >> On Wed, Aug 30, 2023 at 01:30:23PM +0200, Mattijs Korpershoek via libcamera-devel wrote:
>> >>> Hi Andrey,
>> >>>
>> >>> Thank you for this work.
>> >>>
>> >>> I'm very interested by this series, mostly because I would like to add
>> >>> YUYV -> NV12 pixel format conversion in this same class (to simplify
>> >>> some Android use cases I'm working on).
>> >>
>> >> Have you considered using libYUV for such task ? It also feels this
>> >> can be contained in the Android HAL layer where we already use libYUV ?
>> >
>> > Yes, using libYUV is what I plan to do for this.
>> >
>> > I will look into doing this only in the Android HAL layer but it feels
>> > easier to plumb this conversion as a Converter class.
>>
>> I've given this some more thought.
>>
>> If I would implement this as a Converter class but contain it in the
>> Android HAL layer, simple pipeline won't be able to use it (because we don't want
>> libcamera.so to depend on libcamera-hal.so)
>>
>> Some options I see:
>>
>> 1. keep converter_sw as part of libcamera.so and improve it with libyuv conversion
>>    -> adds libyuv dependency, which could be made optional
>>
>> 2. add an android specific pipeline in libcamera-hal.so which would be
>>    based on the simple pipeline.
>>
>> 1. Sounds more maintainable in the long run to me.
>>
>> Andrey, Bryan, Srinivas, I know this discussion is a bit out of scope of
>> the initial patch. If this generates too much noise for you let me know
>> if I should create another thread/drop you from this one.
>>
>
> In my head these are two rather distinct things. The softISP
> implementation does basically provide an IPA module, which generates
> and consume statistics to calculate the parameters used to tune the
> (software) image processing. It is plugged to the simple pipeline
> handler as a Converter and, when it comes to format conversion will
> likely do RAW->YUV through debayering and color space conversion.

Ok

>
> Your use case seems very similar to what is already implemented as the
> PostProcessor class in the Android HAL, which so far only duplicates
> YUV streams but could be expanded to support format conversion between
> YUV formats. A long time has passed since last time I worked on the
> HAL and I might be missing something of course.

Yes. Last time I tried, I could not figure out how to allocate some
additional internal buffers to do the conversion but I will give it
another try. For a media novice like myself, I have a difficult time
following everything in the Android HAL layer.


>
> IOW I don't see the softISP dealing with YUV-to-YUV format conversion.

Yes, I agree on that part. I was thinking of using the softISP as a
"reference implementation" for implementing another Converter based
class for doing my YUYV->NV12 conversion.

>
> If you really want to explore the Converter class and add a plugin to
> the Simple pipeline that produces NV12 when your system can only do
> YUYV, I feel like this would be a different effort than this one ?

I agree that if I go with the Converter class option, it makes more
sense to make a separate plugin/class.

I'll do some more work on this and I'll open another thread later on.

Thanks for your help!

>
>> >
>> >>
>> >> Thanks
>> >>    j
>> >>
>> >>>
>> >>> I'm not familiar enough with the libcamera codebase to judge how well
>> >>> this is implemented but I'm looking forward to see comments on this patchset.
>> >>>
>> >>> On dim., août 06, 2023 at 21:01, Andrey Konovalov via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote:
>> >>>
>> >>> > Here is a draft implementation of Bayer demosaicing which follows the
>> >>> > Converter interface and runs on CPU.
>> >>> >
>> >>> > It also includes a naive version of Grey World AWB. Just for demo purposes
>> >>> > (to make the final image looking a bit nicer). Otherwise, Converter isn't
>> >>> > the right place for AWB - only the statistics should be gathered here,
>> >>> > and the rest belongs to an IPA module.
>> >>> >
>> >>> > Currently this software converter supports single output only, but adding
>> >>> > the second stream for statistics is under consideration.
>> >>> >
>> >>> > As libcamera::Converter currently assumes a media device underneath the
>> >>> > convertor, I wasn't able to avoid hacking the simple pipeline handler to make
>> >>> > it work with the software converter. For the same reason ConverterFactory
>> >>> > is not used for now.
>> >>> >
>> >>> > Only RAW10P format from the sensor is currently supported, but adding more
>> >>> > Bayer formats wouldn't be a problem. Out of 10 bits, only 8 most significant
>> >>> > ones are used to lessen the load on CPU. Simple bilinear interpolation is
>> >>> > used for the same reason.
>> >>> >
>> >>> > AWB simplifications:
>> >>> > - a naive implementation of "Grey World" algorithm: all pixels are used (no
>> >>> >   brightest and darkest pixels excluded from the calculations)
>> >>> > - to lessen the load on CPU, works on raw Bayer data and takes red values from
>> >>> >   red pixels, blue values from blue, and green values from green pixels only.
>> >>> > - to lessen the load on CPU, the red/green/blue gains calculated from the
>> >>> >   current frame data are applied to the next frame. These gains are purely
>> >>> >   in software (no V4L2 controls are set).
>> >>> >
>> >>> > No performance analysis or tuning have been done yet. On RB5 board this
>> >>> > software convertor gives:
>> >>> >   ~ 5 fps at 3278x2462 (camera sensor runs at 15 fps)
>> >>> >   ~ 18..19 fps at 1918x1078 (out of 30 fps)
>> >>> >   ~ 18..19 fps at 1638x1230 (out of 30 fps)
>> >>> >   ~ 30 fps at 638x478 (out of 30 fps)
>> >>> > (The resolutions above are the output ones; demosaic filter drops 1 pixel
>> >>> > from each side of the frame, so that 3280x2464 from the camera sensor becomes
>> >>> > 3278x2462 etc)
>> >>>
>> >>> Could you detail a bit how this was tested? Did you validate this using
>> >>> "cam" ?
>> >>>
>> >>> Also, is there a reason for testing this with real hardware?
>> >>>
>> >>> Can't this be validated/developped upon the Virtual Media Controller
>> >>> Driver (vimc) ?
>> >>>
>> >>> https://docs.kernel.org/admin-guide/media/vimc.html
>> >>>
>> >>> >
>> >>> > Andrey Konovalov (2):
>> >>> >   libcamera: converter: add software converter
>> >>> >   [DNI] libcamera: pipeline: simple: a hack to use sotware converter
>> >>> >     with qcom-camss
>> >>> >
>> >>> >  .../internal/converter/converter_softw.h      |  90 ++++
>> >>> >  src/libcamera/converter/converter_softw.cpp   | 430 ++++++++++++++++++
>> >>> >  src/libcamera/converter/meson.build           |   3 +-
>> >>> >  src/libcamera/pipeline/simple/simple.cpp      |  22 +-
>> >>> >  4 files changed, 542 insertions(+), 3 deletions(-)
>> >>> >  create mode 100644 include/libcamera/internal/converter/converter_softw.h
>> >>> >  create mode 100644 src/libcamera/converter/converter_softw.cpp
>> >>> >
>> >>> > --
>> >>> > 2.34.1