[libcamera-devel] [PATCH v3 18/22] libcamera: rkisp1: Fill stride and frameSize at config validation

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jul 06, 2020 at 05:16:54PM +0200, Jacopo Mondi wrote:
> On Sun, Jul 05, 2020 at 12:51:03AM +0300, Laurent Pinchart wrote:
> > On Sat, Jul 04, 2020 at 10:31:36PM +0900, Paul Elder wrote:
> > > Fill the stride and frameSize fields of the StreamConfiguration at
> > > configuration validation time instead of at camera configuration time.
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > ---
> > > New in v3
> > > ---
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 3c3f3f3..3ac7b3c 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -535,6 +535,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > >
> > >  	cfg.bufferCount = RKISP1_BUFFER_COUNT;
> > >
> > > +	const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> > > +	cfg.stride = info.stride(cfg.size.width, 0);
> > > +	cfg.frameSize = info.frameSize(cfg.size);
> >
> > Same comment as for patches 16/22 and 17/22.
> >
> > The alternative is to use V4L2VideoDevice::tryFormat() in here. Jacopo,
> > any opinion on that ?
> 
> I'm noticing that calculating the stride statically is becoming
> cumbersome...
> 
> True that we have a few pipeline handlers where we have to reverse
> those information from drivers, future pipeline handlers will be
> instrumented from the beginning with those information in place, so it
> will be less a concern.
> 
> The alternative, as you said, is to ask the video device, as that's
> indeed the less error-prone solution, as the only way to get it wrong
> is a driver bug :)
> 
> Recently I've been working to remove the assignment of streams to
> videodevices from the IPU3 pipeline handler, not because I see that as
> something frowned upon from an API perspective or for performances
> reason, but mostly because there's no infrastructure to keep that
> assignment in place between validate() and configure() and if every
> pipeline handler has to implement its own method to do that, then I
> would consider this approach "wrong" as it could easily become a
> maintainance burden I would not like to encourage.
> 
> I'm not sure I want to go there, but from the very beginning we assume
> streams gets assigned to StreamConfigurations only after
> Camera::configure() has been called. I can't find in the documentation
> any strong rationale for this, and we have absolutely no protection in
> place at the moment to prevent application from taking a pointer from
> a StreamConfiguration before it gets assigned (documentation apart).
> 
> From a V4L2 point of view, I think there's nothing preventing us from
> calling try fmt at validation time. The device is open and there's no
> other requirements afaict.
> 
> Long story short: going to the video device would be best, otherwise
> the same information shall be duplicated in multiple places. Even
> if I agree we should try to move more knowledge of the platform to
> userspace, the device requirements in terms of padding and alignement
> seems to me among the less sensible information to record outside of
> the kernel. At the same time, I would not like to see custom vector of
> streams filled in at validate() and retrieved at configure() time.
> 
> Would calling StreamConfiguration::setStream() during validation be
> that nasty in your opinion?

I don't mind, and I'm working on removing setStream() anyway :-)

> > > +
> > >  	return status;
> > >  }
> > >
> > > @@ -683,7 +687,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> > >  		return ret;
> > >
> > >  	cfg.setStream(&data->stream_);
> > > -	cfg.stride = outputFormat.planes[0].bpl;
> > >
> > >  	return 0;
> > >  }

-- 
Regards,

Laurent Pinchart