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

[Naushir Patuck]

On Thu, 12 Oct 2023 at 15:40, Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> 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!

I've got a v2 ready to go, so it's no problem.  I'll wait for further
comments from other folks and post this tomorrow (Friday) morning if
that's ok?

Regards,
Naush


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