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

[Naushir Patuck]

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?

> 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
> > >