| Message ID | 20190220235939.25147-3-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Thu, Feb 21, 2019 at 12:59:37AM +0100, Niklas Söderlund wrote: > The format requested by configureStreams() should exactly match what s/what$/what is/ > programmed on the video device it self. If they do not match the call s/it self/itself/ or simply s/ it self// > should fail as the application in that case will not know what > configuration actually was programmed. s/actually was/was actually/ > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/uvcvideo.cpp | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index 37a3477c2c56f3ea..b7b8ff109109e9c5 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -84,6 +84,7 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, > std::map<Stream *, StreamConfiguration> &config) > { > StreamConfiguration *cfg = &config[&stream_]; > + int ret; > > LOG(UVC, Debug) << "Configure the camera for resolution " > << cfg->width << "x" << cfg->height; > @@ -93,7 +94,16 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, > format.height = cfg->height; > format.fourcc = cfg->pixelFormat; > > - return video_->setFormat(&format); > + ret = video_->setFormat(&format); > + if (ret) > + return ret; > + > + if (format.width != cfg->width || > + format.height != cfg->height || > + format.fourcc != cfg->pixelFormat) > + return -EINVAL; This is fine for now, but I wonder if longer term we should enumerate the available formats and resolutions, and avoiding calling setFormat() is the requested format can't be achieved. The documentation in patch 1/4 explains that no configuration should be applied if an exact match is impossible, and calling setFormat() modifies the hardware configuration. On the other hand this change in hardware configuration isn't visible from the application, so it won't matter much. > + > + return 0; > } > > int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)
Hi Laurent, Thanks for your feedback. On 2019-02-22 12:48:53 +0200, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Thu, Feb 21, 2019 at 12:59:37AM +0100, Niklas Söderlund wrote: > > The format requested by configureStreams() should exactly match what > > s/what$/what is/ > > > programmed on the video device it self. If they do not match the call > > s/it self/itself/ or simply s/ it self// > > > should fail as the application in that case will not know what > > configuration actually was programmed. > > s/actually was/was actually/ I agree, will update this for 2/4 and 3/4 for next version. > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/pipeline/uvcvideo.cpp | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > > index 37a3477c2c56f3ea..b7b8ff109109e9c5 100644 > > --- a/src/libcamera/pipeline/uvcvideo.cpp > > +++ b/src/libcamera/pipeline/uvcvideo.cpp > > @@ -84,6 +84,7 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, > > std::map<Stream *, StreamConfiguration> &config) > > { > > StreamConfiguration *cfg = &config[&stream_]; > > + int ret; > > > > LOG(UVC, Debug) << "Configure the camera for resolution " > > << cfg->width << "x" << cfg->height; > > @@ -93,7 +94,16 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, > > format.height = cfg->height; > > format.fourcc = cfg->pixelFormat; > > > > - return video_->setFormat(&format); > > + ret = video_->setFormat(&format); > > + if (ret) > > + return ret; > > + > > + if (format.width != cfg->width || > > + format.height != cfg->height || > > + format.fourcc != cfg->pixelFormat) > > + return -EINVAL; > > This is fine for now, but I wonder if longer term we should enumerate > the available formats and resolutions, and avoiding calling setFormat() > is the requested format can't be achieved. The documentation in patch > 1/4 explains that no configuration should be applied if an exact match > is impossible, and calling setFormat() modifies the hardware > configuration. On the other hand this change in hardware configuration > isn't visible from the application, so it won't matter much. I concur that that this is a short- to mid- term solution that allows us to move forward. Once we have a more complete StreamConfiguration implementation and more pipeline handler implementations I think we might be able to draw some more insights. At that point I think we will be able to create one or more helper functions to assist pipeline handlers, likely using the *_TRY_* IOCTLs. That way we could try a complete set of configurations in a none invasive fashion and fail in a more safe way if needed. Bonus for *_TRY_* IOCTLs support will be that applications would be able to call that themself before calling configureStreams() to 'update' and inspect the format they request to hardware/driver limitations before asking the hardware to be reprogrammed. At least this is how I model the future of this part of the library in my head, if anyone have other ideas please let me know so I can try to have that in mind when working on the interactions between the Camera and applications. > > > + > > + return 0; > > } > > > > int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Sat, Feb 23, 2019 at 02:39:01AM +0100, Niklas Söderlund wrote: > On 2019-02-22 12:48:53 +0200, Laurent Pinchart wrote: > > On Thu, Feb 21, 2019 at 12:59:37AM +0100, Niklas Söderlund wrote: > >> The format requested by configureStreams() should exactly match what > > > > s/what$/what is/ > > > >> programmed on the video device it self. If they do not match the call > > > > s/it self/itself/ or simply s/ it self// > > > >> should fail as the application in that case will not know what > >> configuration actually was programmed. > > > > s/actually was/was actually/ > > I agree, will update this for 2/4 and 3/4 for next version. > > >> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >> --- > >> src/libcamera/pipeline/uvcvideo.cpp | 12 +++++++++++- > >> 1 file changed, 11 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > >> index 37a3477c2c56f3ea..b7b8ff109109e9c5 100644 > >> --- a/src/libcamera/pipeline/uvcvideo.cpp > >> +++ b/src/libcamera/pipeline/uvcvideo.cpp > >> @@ -84,6 +84,7 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, > >> std::map<Stream *, StreamConfiguration> &config) > >> { > >> StreamConfiguration *cfg = &config[&stream_]; > >> + int ret; > >> > >> LOG(UVC, Debug) << "Configure the camera for resolution " > >> << cfg->width << "x" << cfg->height; > >> @@ -93,7 +94,16 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, > >> format.height = cfg->height; > >> format.fourcc = cfg->pixelFormat; > >> > >> - return video_->setFormat(&format); > >> + ret = video_->setFormat(&format); > >> + if (ret) > >> + return ret; > >> + > >> + if (format.width != cfg->width || > >> + format.height != cfg->height || > >> + format.fourcc != cfg->pixelFormat) > >> + return -EINVAL; > > > > This is fine for now, but I wonder if longer term we should enumerate > > the available formats and resolutions, and avoiding calling setFormat() > > is the requested format can't be achieved. The documentation in patch > > 1/4 explains that no configuration should be applied if an exact match > > is impossible, and calling setFormat() modifies the hardware > > configuration. On the other hand this change in hardware configuration > > isn't visible from the application, so it won't matter much. > > I concur that that this is a short- to mid- term solution that allows us > to move forward. Once we have a more complete StreamConfiguration > implementation and more pipeline handler implementations I think we > might be able to draw some more insights. At that point I think we will > be able to create one or more helper functions to assist pipeline > handlers, likely using the *_TRY_* IOCTLs. That way we could try a > complete set of configurations in a none invasive fashion and fail in a > more safe way if needed. The TRY ioctls will usually not help much as they're limited to video nodes. Something more complex will be needed for MC-based pipelines, but we can do that. > Bonus for *_TRY_* IOCTLs support will be that applications would be able > to call that themself before calling configureStreams() to 'update' and > inspect the format they request to hardware/driver limitations before > asking the hardware to be reprogrammed. At least this is how I model the > future of this part of the library in my head, if anyone have other > ideas please let me know so I can try to have that in mind when working > on the interactions between the Camera and applications. I'm not sure if that will be feasible. While this is the model exposed by V4L2, I fell it may not be the most suitable for applications. We'll have to think about it. > >> + > >> + return 0; > >> } > >> > >> int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)
diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp index 37a3477c2c56f3ea..b7b8ff109109e9c5 100644 --- a/src/libcamera/pipeline/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo.cpp @@ -84,6 +84,7 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, std::map<Stream *, StreamConfiguration> &config) { StreamConfiguration *cfg = &config[&stream_]; + int ret; LOG(UVC, Debug) << "Configure the camera for resolution " << cfg->width << "x" << cfg->height; @@ -93,7 +94,16 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, format.height = cfg->height; format.fourcc = cfg->pixelFormat; - return video_->setFormat(&format); + ret = video_->setFormat(&format); + if (ret) + return ret; + + if (format.width != cfg->width || + format.height != cfg->height || + format.fourcc != cfg->pixelFormat) + return -EINVAL; + + return 0; } int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)
The format requested by configureStreams() should exactly match what programmed on the video device it self. If they do not match the call should fail as the application in that case will not know what configuration actually was programmed. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/pipeline/uvcvideo.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)