| Message ID | 20220623144736.78537-2-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Tomi Valkeinen (2022-06-23 15:47:30) > -C flag is supposed to affect only the camera that was previously > defined in the arguments. That's not the case, and, e.g.: > > cam.py -c2 -C -c3 > > causes camera 3 to start capturing, but it stops after the initial > Requests have been completed. > > Fix the issue by filtering out camera contexts that do not have -C > defined. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > src/py/cam/cam.py | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py > index 2ae89fa8..733e9ae5 100755 > --- a/src/py/cam/cam.py > +++ b/src/py/cam/cam.py > @@ -434,6 +434,8 @@ def main(): > if args.info: > ctx.do_cmd_info() > As the tools are often used for reference or guidance to newcomers it might be helpful to clarify this with a comment. Maybe the filtering is more clear to someone who writes a lot more python ... but from a C perspective it takes me a minute to identify what this line really does. # Filter out contexts which don't need to run a capture > + contexts = [ctx for ctx in contexts if ctx.opt_capture > 0] > + > if args.capture: Should this be done on a per-context basis? - Can cam.py already capture from multiple cameras at the same time? Maybe the rest is in the code below that I haven't read yet. But the filtering itself sounds fine independently: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > state = CaptureState(cm, contexts) > > -- > 2.34.1 >
On Fri, Jun 24, 2022 at 09:13:53AM +0100, Kieran Bingham wrote: > Quoting Tomi Valkeinen (2022-06-23 15:47:30) > > -C flag is supposed to affect only the camera that was previously > > defined in the arguments. That's not the case, and, e.g.: > > > > cam.py -c2 -C -c3 > > > > causes camera 3 to start capturing, but it stops after the initial > > Requests have been completed. > > > > Fix the issue by filtering out camera contexts that do not have -C > > defined. > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > --- > > src/py/cam/cam.py | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py > > index 2ae89fa8..733e9ae5 100755 > > --- a/src/py/cam/cam.py > > +++ b/src/py/cam/cam.py > > @@ -434,6 +434,8 @@ def main(): > > if args.info: > > ctx.do_cmd_info() > > As the tools are often used for reference or guidance to newcomers it > might be helpful to clarify this with a comment. Maybe the filtering is > more clear to someone who writes a lot more python ... but from a C > perspective it takes me a minute to identify what this line really > does. > > # Filter out contexts which don't need to run a capture > > + contexts = [ctx for ctx in contexts if ctx.opt_capture > 0] > > + > > if args.capture: Maybe if contexts: would be clearer ? > Should this be done on a per-context basis? - Can cam.py already capture > from multiple cameras at the same time? > Maybe the rest is in the code below that I haven't read yet. > > But the filtering itself sounds fine independently: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > state = CaptureState(cm, contexts) > >
On 24/06/2022 11:13, Kieran Bingham wrote: > Quoting Tomi Valkeinen (2022-06-23 15:47:30) >> -C flag is supposed to affect only the camera that was previously >> defined in the arguments. That's not the case, and, e.g.: >> >> cam.py -c2 -C -c3 >> >> causes camera 3 to start capturing, but it stops after the initial >> Requests have been completed. >> >> Fix the issue by filtering out camera contexts that do not have -C >> defined. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> src/py/cam/cam.py | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py >> index 2ae89fa8..733e9ae5 100755 >> --- a/src/py/cam/cam.py >> +++ b/src/py/cam/cam.py >> @@ -434,6 +434,8 @@ def main(): >> if args.info: >> ctx.do_cmd_info() >> > > As the tools are often used for reference or guidance to newcomers it > might be helpful to clarify this with a comment. Maybe the filtering is > more clear to someone who writes a lot more python ... but from a C > perspective it takes me a minute to identify what this line really > does. Ok, I'll add a comment. > # Filter out contexts which don't need to run a capture >> + contexts = [ctx for ctx in contexts if ctx.opt_capture > 0] >> + >> if args.capture: > > Should this be done on a per-context basis? - Can cam.py already capture This "if" just checks if there's any capturing to be done. The above works as args.capture is a dict of the capture parameters, but as Laurent commented, it's perhaps clearer to do "if contexts". > from multiple cameras at the same time? Yes, you can capture from multiple cameras and streams. > Maybe the rest is in the code below that I haven't read yet. > > But the filtering itself sounds fine independently: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> state = CaptureState(cm, contexts) >> >> -- >> 2.34.1 >>
On 24/06/2022 11:28, Laurent Pinchart wrote: > On Fri, Jun 24, 2022 at 09:13:53AM +0100, Kieran Bingham wrote: >> Quoting Tomi Valkeinen (2022-06-23 15:47:30) >>> -C flag is supposed to affect only the camera that was previously >>> defined in the arguments. That's not the case, and, e.g.: >>> >>> cam.py -c2 -C -c3 >>> >>> causes camera 3 to start capturing, but it stops after the initial >>> Requests have been completed. >>> >>> Fix the issue by filtering out camera contexts that do not have -C >>> defined. >>> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>> --- >>> src/py/cam/cam.py | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py >>> index 2ae89fa8..733e9ae5 100755 >>> --- a/src/py/cam/cam.py >>> +++ b/src/py/cam/cam.py >>> @@ -434,6 +434,8 @@ def main(): >>> if args.info: >>> ctx.do_cmd_info() >> >> As the tools are often used for reference or guidance to newcomers it >> might be helpful to clarify this with a comment. Maybe the filtering is >> more clear to someone who writes a lot more python ... but from a C >> perspective it takes me a minute to identify what this line really >> does. >> >> # Filter out contexts which don't need to run a capture >>> + contexts = [ctx for ctx in contexts if ctx.opt_capture > 0] >>> + >>> if args.capture: > > Maybe > > if contexts: > > would be clearer ? Yes, indeed. >> Should this be done on a per-context basis? - Can cam.py already capture >> from multiple cameras at the same time? >> Maybe the rest is in the code below that I haven't read yet. >> >> But the filtering itself sounds fine independently: >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks! Tomi
diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py index 2ae89fa8..733e9ae5 100755 --- a/src/py/cam/cam.py +++ b/src/py/cam/cam.py @@ -434,6 +434,8 @@ def main(): if args.info: ctx.do_cmd_info() + contexts = [ctx for ctx in contexts if ctx.opt_capture > 0] + if args.capture: state = CaptureState(cm, contexts)
-C flag is supposed to affect only the camera that was previously defined in the arguments. That's not the case, and, e.g.: cam.py -c2 -C -c3 causes camera 3 to start capturing, but it stops after the initial Requests have been completed. Fix the issue by filtering out camera contexts that do not have -C defined. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- src/py/cam/cam.py | 2 ++ 1 file changed, 2 insertions(+)