| Message ID | 20200120002437.6633-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Mon, Jan 20, 2020 at 02:24:19AM +0200, Laurent Pinchart wrote: > The BufferSource::allocate() return value isn't propagated correctly, > resulting in a test failure when the test should be skipped due to a > missing vivid device. Fix it. > > While at it, return valid status codes from BufferSource::allocate() in > all error cases, with proper diagnostic messages. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > test/camera/buffer_import.cpp | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp > index e7048335e031..16bfca9beeea 100644 > --- a/test/camera/buffer_import.cpp > +++ b/test/camera/buffer_import.cpp > @@ -89,15 +89,23 @@ public: > ret = video_->getFormat(&format); > if (ret) { > std::cout << "Failed to get format on output device" << std::endl; > - return ret; > + return TestFail; Nitpicking, but we now have 2 occurrences of ret = video->doSomething() if (ret) { } Where the value of ret is actually later ignored. > } > > format.size = config.size; > format.fourcc = V4L2VideoDevice::toV4L2Fourcc(config.pixelFormat, false); > - if (video_->setFormat(&format)) > + if (video_->setFormat(&format)) { I would unify all three calls here to this format if (video_->doSomething()) { } and remove ret With or without this considered Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > + std::cout << "Failed to set format on output device" << std::endl; > return TestFail; > + } > > - return video_->exportBuffers(config.bufferCount, &buffers_); > + ret = video_->exportBuffers(config.bufferCount, &buffers_); > + if (ret < 0) { > + std::cout << "Failed to export buffers" << std::endl; > + return TestFail; > + } > + > + return TestPass; > } > > const std::vector<std::unique_ptr<FrameBuffer>> &buffers() > @@ -178,8 +186,8 @@ protected: > > BufferSource source; > int ret = source.allocate(cfg); > - if (ret < 0) > - return TestFail; > + if (ret != TestPass) > + return ret; > > std::vector<Request *> requests; > for (const std::unique_ptr<FrameBuffer> &buffer : source.buffers()) { > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Mon, Jan 20, 2020 at 12:09:17PM +0100, Jacopo Mondi wrote: > On Mon, Jan 20, 2020 at 02:24:19AM +0200, Laurent Pinchart wrote: > > The BufferSource::allocate() return value isn't propagated correctly, > > resulting in a test failure when the test should be skipped due to a > > missing vivid device. Fix it. > > > > While at it, return valid status codes from BufferSource::allocate() in > > all error cases, with proper diagnostic messages. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > test/camera/buffer_import.cpp | 18 +++++++++++++----- > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp > > index e7048335e031..16bfca9beeea 100644 > > --- a/test/camera/buffer_import.cpp > > +++ b/test/camera/buffer_import.cpp > > @@ -89,15 +89,23 @@ public: > > ret = video_->getFormat(&format); > > if (ret) { > > std::cout << "Failed to get format on output device" << std::endl; > > - return ret; > > + return TestFail; > > Nitpicking, but we now have 2 occurrences of > > ret = video->doSomething() > if (ret) { > > } > > Where the value of ret is actually later ignored. > > > } > > > > format.size = config.size; > > format.fourcc = V4L2VideoDevice::toV4L2Fourcc(config.pixelFormat, false); > > - if (video_->setFormat(&format)) > > + if (video_->setFormat(&format)) { > > I would unify all three calls here to this format > > if (video_->doSomething()) { > > } > and remove ret OK I'll change that. > With or without this considered > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > + std::cout << "Failed to set format on output device" << std::endl; > > return TestFail; > > + } > > > > - return video_->exportBuffers(config.bufferCount, &buffers_); > > + ret = video_->exportBuffers(config.bufferCount, &buffers_); > > + if (ret < 0) { > > + std::cout << "Failed to export buffers" << std::endl; > > + return TestFail; > > + } > > + > > + return TestPass; > > } > > > > const std::vector<std::unique_ptr<FrameBuffer>> &buffers() > > @@ -178,8 +186,8 @@ protected: > > > > BufferSource source; > > int ret = source.allocate(cfg); > > - if (ret < 0) > > - return TestFail; > > + if (ret != TestPass) > > + return ret; > > > > std::vector<Request *> requests; > > for (const std::unique_ptr<FrameBuffer> &buffer : source.buffers()) {
diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp index e7048335e031..16bfca9beeea 100644 --- a/test/camera/buffer_import.cpp +++ b/test/camera/buffer_import.cpp @@ -89,15 +89,23 @@ public: ret = video_->getFormat(&format); if (ret) { std::cout << "Failed to get format on output device" << std::endl; - return ret; + return TestFail; } format.size = config.size; format.fourcc = V4L2VideoDevice::toV4L2Fourcc(config.pixelFormat, false); - if (video_->setFormat(&format)) + if (video_->setFormat(&format)) { + std::cout << "Failed to set format on output device" << std::endl; return TestFail; + } - return video_->exportBuffers(config.bufferCount, &buffers_); + ret = video_->exportBuffers(config.bufferCount, &buffers_); + if (ret < 0) { + std::cout << "Failed to export buffers" << std::endl; + return TestFail; + } + + return TestPass; } const std::vector<std::unique_ptr<FrameBuffer>> &buffers() @@ -178,8 +186,8 @@ protected: BufferSource source; int ret = source.allocate(cfg); - if (ret < 0) - return TestFail; + if (ret != TestPass) + return ret; std::vector<Request *> requests; for (const std::unique_ptr<FrameBuffer> &buffer : source.buffers()) {