| Message ID | 20190128004109.25860-1-niklas.soderlund@ragnatech.se |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Except for a "we're writing an option parser rather than pulling one in" concern, I don't see anything too wrong with this series and it's a stand-alone tool (which you currently own :D ) So I'd say, Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com> <edit: a couple of minor notes on 6/6> I think creating an opaque object to manage the camera from the application point of view will give us a lot of benefits to hiding the internal implementation and shared-ptrs. Then an application can get a single 'unique' ptr - and it's destruction can handle all clean up on the internal calls. Anyway - Keeping the tool in use and actively developed will help us develop how the applications will use the library - so I'm all for getting this series in and developing it as we go. On 28/01/2019 00:41, Niklas Söderlund wrote: > Hi, > > This series aims to add a --format option to the cam utility so that it > can set the format of a stream of a specified camera. To do this in an > efficient manner a new key=value parser is needed to understand the > arguments given to the new option, example > > cam --camera mycam --format width=800,height=600 > > This series adds such a parser and then moves on to adding the new > operation to the cam utility in 6/6. This series depends on [1] for 6/6 > which needs the stream API in the camera object to actually set the > requested format. > > 1. [PATCH v3 0/6] libcamera: add basic support for streams and format > configuration > > Niklas Söderlund (6): > cam: options: move enum OptionArgument > cam: options: create a template class for options > cam: options: return if addOption() succeeds or not > cam: options: remove OptionsParser::options_ > cam: options: add a key=value parser > cam: add --format option to configure a stream > > src/cam/main.cpp | 107 ++++++++++++++++++++++---- > src/cam/options.cpp | 178 +++++++++++++++++++++++++++++++++++++++----- > src/cam/options.h | 70 +++++++++++++---- > 3 files changed, 305 insertions(+), 50 deletions(-) >
Hi Niklas, When I rebase a series with these patches included - it fails to compile each patch cleanly using the -x command. git rebase -i $BASECOMMIT -x "cd build && ninja" Now I don't think that's a fault of this series, but a fault of the src/cam/meson.build file not specifying the options.h header as part of the sources. I think that is causing the main.cpp not to get rebuilt/re-linked when there are further changes here to the options file ... However - I'm surprised, as I would have expected meson to do some sort of include depend checking or generation. Have you experienced anything similar here? or am I going crazy. Now I think about it - the options.cpp changes too - so it should still rebuild. Ugh.. so I'm not sure what the underlying fault is yet. -- Kieran On 28/01/2019 00:41, Niklas Söderlund wrote: > Hi, > > This series aims to add a --format option to the cam utility so that it > can set the format of a stream of a specified camera. To do this in an > efficient manner a new key=value parser is needed to understand the > arguments given to the new option, example > > cam --camera mycam --format width=800,height=600 > > This series adds such a parser and then moves on to adding the new > operation to the cam utility in 6/6. This series depends on [1] for 6/6 > which needs the stream API in the camera object to actually set the > requested format. > > 1. [PATCH v3 0/6] libcamera: add basic support for streams and format > configuration > > Niklas Söderlund (6): > cam: options: move enum OptionArgument > cam: options: create a template class for options > cam: options: return if addOption() succeeds or not > cam: options: remove OptionsParser::options_ > cam: options: add a key=value parser > cam: add --format option to configure a stream > > src/cam/main.cpp | 107 ++++++++++++++++++++++---- > src/cam/options.cpp | 178 +++++++++++++++++++++++++++++++++++++++----- > src/cam/options.h | 70 +++++++++++++---- > 3 files changed, 305 insertions(+), 50 deletions(-) >
Hi Kieran, On 2019-01-30 13:46:17 +0000, Kieran Bingham wrote: > Hi Niklas, > > When I rebase a series with these patches included - it fails to compile > each patch cleanly using the -x command. > > git rebase -i $BASECOMMIT -x "cd build && ninja" > > > Now I don't think that's a fault of this series, but a fault of the > src/cam/meson.build file not specifying the options.h header as part of > the sources. I think that is causing the main.cpp not to get > rebuilt/re-linked when there are further changes here to the options > file ... > > However - I'm surprised, as I would have expected meson to do some sort > of include depend checking or generation. > > Have you experienced anything similar here? or am I going crazy. I have experienced it, and not only with the cam tool but the library as a whole. As I understand it we don't list any header files as sources. We just list them to feed them to Doxygen if I understand it correctly. I had it on my todo list to figure out why, but have not gotten around to do it as other things have been higher on my list and my workaround of using ccache and git rebase .. --exec "cd build && ninja clean && ninja" allowed me to move forward :-) > > Now I think about it - the options.cpp changes too - so it should still > rebuild. Ugh.. so I'm not sure what the underlying fault is yet. How do you think we should solve this? Looking around it seems other people are including there header files in the source file list, should we do the same? > > -- > Kieran > > > On 28/01/2019 00:41, Niklas Söderlund wrote: > > Hi, > > > > This series aims to add a --format option to the cam utility so that it > > can set the format of a stream of a specified camera. To do this in an > > efficient manner a new key=value parser is needed to understand the > > arguments given to the new option, example > > > > cam --camera mycam --format width=800,height=600 > > > > This series adds such a parser and then moves on to adding the new > > operation to the cam utility in 6/6. This series depends on [1] for 6/6 > > which needs the stream API in the camera object to actually set the > > requested format. > > > > 1. [PATCH v3 0/6] libcamera: add basic support for streams and format > > configuration > > > > Niklas Söderlund (6): > > cam: options: move enum OptionArgument > > cam: options: create a template class for options > > cam: options: return if addOption() succeeds or not > > cam: options: remove OptionsParser::options_ > > cam: options: add a key=value parser > > cam: add --format option to configure a stream > > > > src/cam/main.cpp | 107 ++++++++++++++++++++++---- > > src/cam/options.cpp | 178 +++++++++++++++++++++++++++++++++++++++----- > > src/cam/options.h | 70 +++++++++++++---- > > 3 files changed, 305 insertions(+), 50 deletions(-) > > > > -- > Regards > -- > Kieran
Hello, On Wed, Jan 30, 2019 at 06:05:34PM +0100, Niklas Söderlund wrote: > On 2019-01-30 13:46:17 +0000, Kieran Bingham wrote: > > Hi Niklas, > > > > When I rebase a series with these patches included - it fails to compile > > each patch cleanly using the -x command. > > > > git rebase -i $BASECOMMIT -x "cd build && ninja" > > > > > > Now I don't think that's a fault of this series, but a fault of the > > src/cam/meson.build file not specifying the options.h header as part of > > the sources. I think that is causing the main.cpp not to get > > rebuilt/re-linked when there are further changes here to the options > > file ... $ ninja -v ninja: no work to do. $ touch ../src/cam/options.h $ ninja -v [1/3] c++ -Isrc/cam/src@cam@@cam@exe -Isrc/cam -I../src/cam -Iinclude -I../include -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++11 -g -Wno-unused-parameter -MD -MQ 'src/cam/src@cam@@cam@exe/main.cpp.o' -MF 'src/cam/src@cam@@cam@exe/main.cpp.o.d' -o 'src/cam/src@cam@@cam@exe/main.cpp.o' -c ../src/cam/main.cpp [2/3] c++ -Isrc/cam/src@cam@@cam@exe -Isrc/cam -I../src/cam -Iinclude -I../include -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++11 -g -Wno-unused-parameter -MD -MQ 'src/cam/src@cam@@cam@exe/options.cpp.o' -MF 'src/cam/src@cam@@cam@exe/options.cpp.o.d' -o 'src/cam/src@cam@@cam@exe/options.cpp.o' -c ../src/cam/options.cpp [3/3] c++ -o src/cam/cam 'src/cam/src@cam@@cam@exe/event_loop.cpp.o' 'src/cam/src@cam@@cam@exe/main.cpp.o' 'src/cam/src@cam@@cam@exe/options.cpp.o' -Wl,--no-undefined -Wl,--as-needed -Wl,--start-group src/libcamera/libcamera.so -Wl,--end-group '-Wl,-rpath,$ORIGIN/../libcamera' -Wl,-rpath-link,src/libcamera Am I missing something ? > > However - I'm surprised, as I would have expected meson to do some sort > > of include depend checking or generation. > > > > Have you experienced anything similar here? or am I going crazy. > > I have experienced it, and not only with the cam tool but the library as > a whole. As I understand it we don't list any header files as sources. > We just list them to feed them to Doxygen if I understand it correctly. > > I had it on my todo list to figure out why, but have not gotten around > to do it as other things have been higher on my list and my workaround > of using ccache and > > git rebase .. --exec "cd build && ninja clean && ninja" > > allowed me to move forward :-) > > > > > Now I think about it - the options.cpp changes too - so it should still > > rebuild. Ugh.. so I'm not sure what the underlying fault is yet. > > How do you think we should solve this? Looking around it seems other > people are including there header files in the source file list, should > we do the same? > > > On 28/01/2019 00:41, Niklas Söderlund wrote: > >> Hi, > >> > >> This series aims to add a --format option to the cam utility so that it > >> can set the format of a stream of a specified camera. To do this in an > >> efficient manner a new key=value parser is needed to understand the > >> arguments given to the new option, example > >> > >> cam --camera mycam --format width=800,height=600 > >> > >> This series adds such a parser and then moves on to adding the new > >> operation to the cam utility in 6/6. This series depends on [1] for 6/6 > >> which needs the stream API in the camera object to actually set the > >> requested format. > >> > >> 1. [PATCH v3 0/6] libcamera: add basic support for streams and format > >> configuration > >> > >> Niklas Söderlund (6): > >> cam: options: move enum OptionArgument > >> cam: options: create a template class for options > >> cam: options: return if addOption() succeeds or not > >> cam: options: remove OptionsParser::options_ > >> cam: options: add a key=value parser > >> cam: add --format option to configure a stream > >> > >> src/cam/main.cpp | 107 ++++++++++++++++++++++---- > >> src/cam/options.cpp | 178 +++++++++++++++++++++++++++++++++++++++----- > >> src/cam/options.h | 70 +++++++++++++---- > >> 3 files changed, 305 insertions(+), 50 deletions(-)
Hi Kieran, On Tue, Jan 29, 2019 at 09:33:32AM +0000, Kieran Bingham wrote: > Hi Niklas, > > Except for a "we're writing an option parser rather than pulling one in" > concern, I don't see anything too wrong with this series and it's a > stand-alone tool (which you currently own :D ) > > So I'd say, > > Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > <edit: a couple of minor notes on 6/6> > > I think creating an opaque object to manage the camera from the > application point of view will give us a lot of benefits to hiding the > internal implementation and shared-ptrs. That's Tomasz's proposal, right ? > Then an application can get a single 'unique' ptr - and it's destruction > can handle all clean up on the internal calls. Should that be a std::unique_ptr<>, or just a normal pointer ? > Anyway - Keeping the tool in use and actively developed will help us > develop how the applications will use the library - so I'm all for > getting this series in and developing it as we go. > > On 28/01/2019 00:41, Niklas Söderlund wrote: > > Hi, > > > > This series aims to add a --format option to the cam utility so that it > > can set the format of a stream of a specified camera. To do this in an > > efficient manner a new key=value parser is needed to understand the > > arguments given to the new option, example > > > > cam --camera mycam --format width=800,height=600 > > > > This series adds such a parser and then moves on to adding the new > > operation to the cam utility in 6/6. This series depends on [1] for 6/6 > > which needs the stream API in the camera object to actually set the > > requested format. > > > > 1. [PATCH v3 0/6] libcamera: add basic support for streams and format > > configuration > > > > Niklas Söderlund (6): > > cam: options: move enum OptionArgument > > cam: options: create a template class for options > > cam: options: return if addOption() succeeds or not > > cam: options: remove OptionsParser::options_ > > cam: options: add a key=value parser > > cam: add --format option to configure a stream > > > > src/cam/main.cpp | 107 ++++++++++++++++++++++---- > > src/cam/options.cpp | 178 +++++++++++++++++++++++++++++++++++++++----- > > src/cam/options.h | 70 +++++++++++++---- > > 3 files changed, 305 insertions(+), 50 deletions(-)
Hi Laurent, On 2019-01-31 12:06:35 +0200, Laurent Pinchart wrote: > Hi Kieran, > > On Tue, Jan 29, 2019 at 09:33:32AM +0000, Kieran Bingham wrote: > > Hi Niklas, > > > > Except for a "we're writing an option parser rather than pulling one in" > > concern, I don't see anything too wrong with this series and it's a > > stand-alone tool (which you currently own :D ) > > > > So I'd say, > > > > Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > <edit: a couple of minor notes on 6/6> > > > > I think creating an opaque object to manage the camera from the > > application point of view will give us a lot of benefits to hiding the > > internal implementation and shared-ptrs. > > That's Tomasz's proposal, right ? To me this is the d-pointer discussion we had in the past and recoded as 'Research ABI stability with regard to private data' topic. And if I understand Tomasz proposal his view is somewhat aligned whit this, but I might have misunderstood. > > > Then an application can get a single 'unique' ptr - and it's destruction > > can handle all clean up on the internal calls. > > Should that be a std::unique_ptr<>, or just a normal pointer ? > > > Anyway - Keeping the tool in use and actively developed will help us > > develop how the applications will use the library - so I'm all for > > getting this series in and developing it as we go. > > > > On 28/01/2019 00:41, Niklas Söderlund wrote: > > > Hi, > > > > > > This series aims to add a --format option to the cam utility so that it > > > can set the format of a stream of a specified camera. To do this in an > > > efficient manner a new key=value parser is needed to understand the > > > arguments given to the new option, example > > > > > > cam --camera mycam --format width=800,height=600 > > > > > > This series adds such a parser and then moves on to adding the new > > > operation to the cam utility in 6/6. This series depends on [1] for 6/6 > > > which needs the stream API in the camera object to actually set the > > > requested format. > > > > > > 1. [PATCH v3 0/6] libcamera: add basic support for streams and format > > > configuration > > > > > > Niklas Söderlund (6): > > > cam: options: move enum OptionArgument > > > cam: options: create a template class for options > > > cam: options: return if addOption() succeeds or not > > > cam: options: remove OptionsParser::options_ > > > cam: options: add a key=value parser > > > cam: add --format option to configure a stream > > > > > > src/cam/main.cpp | 107 ++++++++++++++++++++++---- > > > src/cam/options.cpp | 178 +++++++++++++++++++++++++++++++++++++++----- > > > src/cam/options.h | 70 +++++++++++++---- > > > 3 files changed, 305 insertions(+), 50 deletions(-) > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Fri, Feb 01, 2019 at 04:22:00PM +0100, Niklas Söderlund wrote: > On 2019-01-31 12:06:35 +0200, Laurent Pinchart wrote: > > On Tue, Jan 29, 2019 at 09:33:32AM +0000, Kieran Bingham wrote: > >> Hi Niklas, > >> > >> Except for a "we're writing an option parser rather than pulling one in" > >> concern, I don't see anything too wrong with this series and it's a > >> stand-alone tool (which you currently own :D ) > >> > >> So I'd say, > >> > >> Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> > >> <edit: a couple of minor notes on 6/6> > >> > >> I think creating an opaque object to manage the camera from the > >> application point of view will give us a lot of benefits to hiding the > >> internal implementation and shared-ptrs. > > > > That's Tomasz's proposal, right ? > > To me this is the d-pointer discussion we had in the past and recoded as > 'Research ABI stability with regard to private data' topic. And if I > understand Tomasz proposal his view is somewhat aligned whit this, but I > might have misunderstood. It's similar in purpose to the d-pointer, yes. Usually a d-pointer will have the same lifetime as the object that holds it, so this case is slightly different in the sense that the d-pointer would be shared, but otherwise it does indeed serve as a way to hide the underlying implementation. > >> Then an application can get a single 'unique' ptr - and it's destruction > >> can handle all clean up on the internal calls. > > > > Should that be a std::unique_ptr<>, or just a normal pointer ? > > > >> Anyway - Keeping the tool in use and actively developed will help us > >> develop how the applications will use the library - so I'm all for > >> getting this series in and developing it as we go. > >> > >> On 28/01/2019 00:41, Niklas Söderlund wrote: > >>> Hi, > >>> > >>> This series aims to add a --format option to the cam utility so that it > >>> can set the format of a stream of a specified camera. To do this in an > >>> efficient manner a new key=value parser is needed to understand the > >>> arguments given to the new option, example > >>> > >>> cam --camera mycam --format width=800,height=600 > >>> > >>> This series adds such a parser and then moves on to adding the new > >>> operation to the cam utility in 6/6. This series depends on [1] for 6/6 > >>> which needs the stream API in the camera object to actually set the > >>> requested format. > >>> > >>> 1. [PATCH v3 0/6] libcamera: add basic support for streams and format > >>> configuration > >>> > >>> Niklas Söderlund (6): > >>> cam: options: move enum OptionArgument > >>> cam: options: create a template class for options > >>> cam: options: return if addOption() succeeds or not > >>> cam: options: remove OptionsParser::options_ > >>> cam: options: add a key=value parser > >>> cam: add --format option to configure a stream > >>> > >>> src/cam/main.cpp | 107 ++++++++++++++++++++++---- > >>> src/cam/options.cpp | 178 +++++++++++++++++++++++++++++++++++++++----- > >>> src/cam/options.h | 70 +++++++++++++---- > >>> 3 files changed, 305 insertions(+), 50 deletions(-)
Hi, This series aims to add a --format option to the cam utility so that it can set the format of a stream of a specified camera. To do this in an efficient manner a new key=value parser is needed to understand the arguments given to the new option, example cam --camera mycam --format width=800,height=600 This series adds such a parser and then moves on to adding the new operation to the cam utility in 6/6. This series depends on [1] for 6/6 which needs the stream API in the camera object to actually set the requested format. 1. [PATCH v3 0/6] libcamera: add basic support for streams and format configuration Niklas Söderlund (6): cam: options: move enum OptionArgument cam: options: create a template class for options cam: options: return if addOption() succeeds or not cam: options: remove OptionsParser::options_ cam: options: add a key=value parser cam: add --format option to configure a stream src/cam/main.cpp | 107 ++++++++++++++++++++++---- src/cam/options.cpp | 178 +++++++++++++++++++++++++++++++++++++++----- src/cam/options.h | 70 +++++++++++++---- 3 files changed, 305 insertions(+), 50 deletions(-)