| Message ID | 20200704133140.1738660-19-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. 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@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 ? > + > 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; > }
On Sun, Jul 05, 2020 at 12:51:03AM +0300, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. > > 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@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? Thanks j > > + > > 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 > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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; > > > }
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); + 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; }
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@ideasonboard.com> --- New in v3 --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)