| Message ID | 20190228021626.15062-2-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. 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 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> (with the added todo in 3/4) > Signed-off-by: Niklas Söderlund <niklas.soderlund@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();
Hi Laurent, Thanks for your feedback. On 2019-02-28 19:14:37 +0200, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > 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. 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@ideasonboard.com> > > (with the added todo in 3/4) > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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
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@ideasonboard.com> > > > > (with the added todo in 3/4) > > > >> Signed-off-by: Niklas Söderlund <niklas.soderlund@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();
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();
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. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/cam/main.cpp | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)