[libcamera-devel] [PATCH v2 1/4] cam: fix order camera is operated on

[Laurent Pinchart]

Hi Niklas,

On Thu, Feb 28, 2019 at 06:38:39PM +0100, Niklas Söderlund wrote:
> On 2019-02-28 19:14:37 +0200, Laurent Pinchart wrote:
> > On Thu, Feb 28, 2019 at 03:16:23AM +0100, Niklas Söderlund wrote:
> >> Upcoming enforcing of order the camera shall be operate on is not
> >> compatible with the cam utility. Requests shall be queued after the
> >> camera is started, not before.
> > 
> > My comment on v1 hasn't been addressed. Could you at least add a \todo
> > in the state machine patch to remind us to document camera start timings
> > ? I'm still not sure that preventing requests from being queued before
> > calling start() will not come back to bite us later, but for now I can't
> > think of a good alternative, so
> 
> I will add a \todo to 3/4, sorry for missing this. How about?
> 
>  * \todo Add documentation for camera star timings. What exactly do the
>  * camera expect the pipeline handler to do when start() is called.

s/star/start/
s/do the/does the/
s/called./called?/

> I agree we might need to relax the criteria for when requests can be 
> queued in the future. For now I think this is the right thing to do and 
> we can change it later if we discover a need for it.
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > (with the added todo in 3/4)
> > 
> >> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >> ---
> >>  src/cam/main.cpp | 18 +++++++++++-------
> >>  1 file changed, 11 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> >> index 4c2df583fe8e99b7..8df8844c33a2d052 100644
> >> --- a/src/cam/main.cpp
> >> +++ b/src/cam/main.cpp
> >> @@ -133,6 +133,7 @@ static int capture()
> >>  	int ret;
> >>  
> >>  	std::vector<Stream *> streams = camera->streams();
> >> +	std::vector<Request *> requests;
> >>  
> >>  	ret = configureStreams(camera.get(), streams);
> >>  	if (ret < 0) {
> >> @@ -169,21 +170,24 @@ static int capture()
> >>  			goto out;
> >>  		}
> >>  
> >> -		ret = camera->queueRequest(request);
> >> -		if (ret < 0) {
> >> -			std::cerr << "Can't queue request" << std::endl;
> >> -			goto out;
> >> -		}
> >> +		requests.push_back(request);
> >>  	}
> >>  
> >> -	std::cout << "Capture until user interrupts by SIGINT" << std::endl;
> >> -
> >>  	ret = camera->start();
> >>  	if (ret) {
> >>  		std::cout << "Failed to start capture" << std::endl;
> >>  		goto out;
> >>  	}
> >>  
> >> +	for (Request *request : requests) {
> >> +		ret = camera->queueRequest(request);
> >> +		if (ret < 0) {
> >> +			std::cerr << "Can't queue request" << std::endl;
> >> +			goto out;
> >> +		}
> >> +	}
> >> +
> >> +	std::cout << "Capture until user interrupts by SIGINT" << std::endl;
> >>  	ret = loop->exec();
> >>  
> >>  	ret = camera->stop();

-- 
Regards,

Laurent Pinchart