[libcamera-devel] [PATCH 14/20] pipeline: rpi: Move flip handling validation code

[Jacopo Mondi]

Hi Naush

On Thu, Oct 12, 2023 at 01:55:00PM +0100, Naushir Patuck wrote:
> On Thu, 12 Oct 2023 at 13:07, Naushir Patuck <naush at raspberrypi.com> wrote:
> >
> > On Thu, 12 Oct 2023 at 11:05, Jacopo Mondi
> > <jacopo.mondi at ideasonboard.com> wrote:
> > >
> > > Hi Naush
> > >
> > > On Fri, Oct 06, 2023 at 02:19:54PM +0100, Naushir Patuck via libcamera-devel wrote:
> > > > Move the handling of Bayer order changes due to flips entirely into
> > > > platformValidate(). This removes the need for this code to be split
> > > > between platformValidate() and validate() as it is right now.
> > >
> > > Doesn't this mean the same code has to be repeated in both platform's
> > > pipelines ?
> >
> > Sadly yes :(
> >
> > I haven't figured out a clean/tidy way to do this in pipeline_base and
> > remove the duplication.  Things get a bit more complicated when user
> > packing requests interfere with the raw output as well, and packing
> > constraints are platform specific (e.g. no CSI2 packing on Pi5).  I'll
> > continue working on getting a cleaner solution, but depending on the
> > timeframe, it might have to be put on top of this work.
> >
>
> And now after spending a bit more time looking at it, I think the
> solution is actually simpler than I thought!  I have a candidate fix
> that can be used to replace this patch.  Should I send it in a v2
> together with all the other minor fixes you noted?
>

If you want, I certainly won't oppose. My proposal to fix minors when
applying still holds, but if you consider worth sending a v2 I'm all
for it!

Thanks
  j

> > Regards,
> > Naush
> >
> > >
> > > >
> > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > > ---
> > > >  .../pipeline/rpi/common/pipeline_base.cpp          | 14 --------------
> > > >  src/libcamera/pipeline/rpi/vc4/vc4.cpp             | 12 ++++++++++--
> > > >  2 files changed, 10 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > index 825eae80d014..7437d38dc9ba 100644
> > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > @@ -242,20 +242,6 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > > >               if (ret)
> > > >                       return Invalid;
> > > >
> > > > -             /*
> > > > -              * Some sensors change their Bayer order when they are h-flipped
> > > > -              * or v-flipped, according to the transform. If this one does, we
> > > > -              * must advertise the transformed Bayer order in the raw stream.
> > > > -              * Note how we must fetch the "native" (i.e. untransformed) Bayer
> > > > -              * order, because the sensor may currently be flipped!
> > > > -              */
> > > > -             BayerFormat bayer = BayerFormat::fromPixelFormat(raw.cfg->pixelFormat);
> > > > -             if (data_->flipsAlterBayerOrder_) {
> > > > -                     bayer.order = data_->nativeBayerOrder_;
> > > > -                     bayer = bayer.transform(combinedTransform_);
> > > > -             }
> > > > -             raw.cfg->pixelFormat = bayer.toPixelFormat();
> > > > -
> > > >               if (RPi::PipelineHandlerBase::updateStreamConfig(raw.cfg, raw.format))
> > > >                       status = Adjusted;
> > > >       }
> > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > > > index 233473e2fe2b..4b42ddc71115 100644
> > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > > > @@ -411,9 +411,17 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(RPi::RPiCameraConfig
> > > >               StreamConfiguration *rawStream = rawStreams[0].cfg;
> > > >               BayerFormat rawBayer = BayerFormat::fromMbusCode(rpiConfig->sensorFormat_.mbus_code);
> > > >
> > > > -             /* Handle flips to make sure to match the RAW stream format. */
> > > > -             if (flipsAlterBayerOrder_)
> > > > +             /*
> > > > +              * Some sensors change their Bayer order when they are h-flipped
> > > > +              * or v-flipped, according to the transform. If this one does, we
> > > > +              * must advertise the transformed Bayer order in the raw stream.
> > > > +              * Note how we must fetch the "native" (i.e. untransformed) Bayer
> > > > +              * order, because the sensor may currently be flipped!
> > > > +              */
> > > > +             if (flipsAlterBayerOrder_) {
> > > > +                     rawBayer.order = nativeBayerOrder_;
> > > >                       rawBayer = rawBayer.transform(rpiConfig->combinedTransform_);
> > > > +             }
> > > >
> > > >               /* Apply the user requested packing. */
> > > >               rawBayer.packing = BayerFormat::fromPixelFormat(rawStream->pixelFormat).packing;
> > > > --
> > > > 2.34.1
> > > >