Message ID | 20220630133902.321099-4-jacopo@jmondi.org |
---|---|
State | Not Applicable, archived |
Headers | show |
Series |
|
Related | show |
Quoting Jacopo Mondi via libcamera-devel (2022-06-30 14:38:42) > Introduce the enumeration of internal controls in > internal_control_ids.yaml. > > Plumb in the build system the command to generate the definition of > internal controls by re-using the same mechanism used for public > controls to make it easy to extend it to also handle internal properties > in future. I'm afraid these are bugging me ... Perhaps I missed this before when we discussed internal controls... > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org> > --- > include/libcamera/internal/meson.build | 15 ++++++++++++++ > src/libcamera/internal_control_ids.yaml | 27 +++++++++++++++++++++++++ > src/libcamera/meson.build | 17 ++++++++++++++++ > 3 files changed, 59 insertions(+) > create mode 100644 src/libcamera/internal_control_ids.yaml > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build > index 7a780d48ee57..1559c3c368c4 100644 > --- a/include/libcamera/internal/meson.build > +++ b/include/libcamera/internal/meson.build > @@ -9,6 +9,21 @@ libcamera_tracepoint_header = custom_target( > command: [gen_tracepoints_header, '@OUTPUT@', '@INPUT@'], > ) > > +# Generate the list of internal controls identifiers > +internal_control_source_files = ['control_ids'] > + > +libcamera_internal_control_headers = [] > + > +foreach header : internal_control_source_files > + input_files = files('../../../src/libcamera/internal_' + header +'.yaml',\ > + '../' + header + '.h.in') > + libcamera_internal_control_headers += custom_target( > + 'internal_' + header + '_h', > + input : input_files, > + output : header + '.h', > + command : [gen_controls, '--internal=True','-o', '@OUTPUT@', '@INPUT@']) > +endforeach > + > libcamera_internal_headers = files([ > 'bayer_format.h', > 'byte_stream_buffer.h', > diff --git a/src/libcamera/internal_control_ids.yaml b/src/libcamera/internal_control_ids.yaml > new file mode 100644 > index 000000000000..6d3775afcf67 > --- /dev/null > +++ b/src/libcamera/internal_control_ids.yaml > @@ -0,0 +1,27 @@ > +# SPDX-License-Identifier: LGPL-2.1-or-later > +# > +# Copyright (C) 2022, Google Inc. > +# > +%YAML 1.2 > +--- > +# Enumeration of internal libcamera controls > +# Not exposed to application, for library use only > + > +controls: > + > + - ExposureTime: We have: libcamera::controls::ExposureTime already https://libcamera.org/api-html/namespacelibcamera_1_1controls.html#a4e1ca45653b62cd969d4d67a741076eb > + type: int32_t > + description: | > + The sensor exposure time in milliseconds. > + > + - FrameDuration: And libcamera::controls::FrameDuration https://libcamera.org/api-html/namespacelibcamera_1_1controls.html#a37d99a76c7249c143beecd70917469be > + type: int64_t > + description: | > + The sensor frame duration time in milliseconds. > + > + - AnalogueGain: And ... libcamera::controls::AnalogueGain https://libcamera.org/api-html/namespacelibcamera_1_1controls.html#ab34ebeaa9cbfb3f3fc6996b089ca52b0 These are 'metadata' controls that are supposed to be returned by the Pipeline handler in the metadata. But they mean exactly the same thing here. So, is there a reason we aren't using them directly? -- Kieran > + type: float > + description: | > + The sensor analogue gain value. > + > +... > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > index b57bee7ef6ca..89fdf347c708 100644 > --- a/src/libcamera/meson.build > +++ b/src/libcamera/meson.build > @@ -52,6 +52,7 @@ libcamera_sources = files([ > libcamera_sources += libcamera_public_headers > libcamera_sources += libcamera_generated_ipa_headers > libcamera_sources += libcamera_tracepoint_header > +libcamera_sources += libcamera_internal_control_headers > > includes = [ > libcamera_includes, > @@ -99,6 +100,7 @@ if not libyaml.found() > libyaml = libyaml_wrap.dependency('yaml') > endif > > +# Generate control_ids.cpp and property_ids.cpp > control_sources = [] > > foreach source : control_source_files > @@ -111,6 +113,21 @@ endforeach > > libcamera_sources += control_sources > > +# Generate internal_control_ids.cpp > +internal_control_source_files = ['control_ids'] > +internal_control_sources = [] > + > +foreach source : internal_control_source_files > + input_files = files('internal_' + source +'.yaml', source + '.cpp.in') > + internal_control_sources += custom_target('internal_' + source + '_cpp', > + input : input_files, > + output : 'internal_' + source + '.cpp', > + command : [gen_controls, '--internal=True',\ > + '-o', '@OUTPUT@',\ > + '@INPUT@']) > +endforeach > +libcamera_sources += internal_control_sources > + > gen_version = meson.project_source_root() / 'utils' / 'gen-version.sh' > > # Use vcs_tag() and not configure_file() or run_command(), to ensure that the > -- > 2.36.1 >
Hi Kieran On Thu, Jun 30, 2022 at 10:06:27PM +0100, Kieran Bingham wrote: > Quoting Jacopo Mondi via libcamera-devel (2022-06-30 14:38:42) > > Introduce the enumeration of internal controls in > > internal_control_ids.yaml. > > > > Plumb in the build system the command to generate the definition of > > internal controls by re-using the same mechanism used for public > > controls to make it easy to extend it to also handle internal properties > > in future. > > I'm afraid these are bugging me ... Perhaps I missed this before when we > discussed internal controls... > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org> > > --- > > include/libcamera/internal/meson.build | 15 ++++++++++++++ > > src/libcamera/internal_control_ids.yaml | 27 +++++++++++++++++++++++++ > > src/libcamera/meson.build | 17 ++++++++++++++++ > > 3 files changed, 59 insertions(+) > > create mode 100644 src/libcamera/internal_control_ids.yaml > > > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build > > index 7a780d48ee57..1559c3c368c4 100644 > > --- a/include/libcamera/internal/meson.build > > +++ b/include/libcamera/internal/meson.build > > @@ -9,6 +9,21 @@ libcamera_tracepoint_header = custom_target( > > command: [gen_tracepoints_header, '@OUTPUT@', '@INPUT@'], > > ) > > > > +# Generate the list of internal controls identifiers > > +internal_control_source_files = ['control_ids'] > > + > > +libcamera_internal_control_headers = [] > > + > > +foreach header : internal_control_source_files > > + input_files = files('../../../src/libcamera/internal_' + header +'.yaml',\ > > + '../' + header + '.h.in') > > + libcamera_internal_control_headers += custom_target( > > + 'internal_' + header + '_h', > > + input : input_files, > > + output : header + '.h', > > + command : [gen_controls, '--internal=True','-o', '@OUTPUT@', '@INPUT@']) > > +endforeach > > + > > libcamera_internal_headers = files([ > > 'bayer_format.h', > > 'byte_stream_buffer.h', > > diff --git a/src/libcamera/internal_control_ids.yaml b/src/libcamera/internal_control_ids.yaml > > new file mode 100644 > > index 000000000000..6d3775afcf67 > > --- /dev/null > > +++ b/src/libcamera/internal_control_ids.yaml > > @@ -0,0 +1,27 @@ > > +# SPDX-License-Identifier: LGPL-2.1-or-later > > +# > > +# Copyright (C) 2022, Google Inc. > > +# > > +%YAML 1.2 > > +--- > > +# Enumeration of internal libcamera controls > > +# Not exposed to application, for library use only > > + > > +controls: > > + > > + - ExposureTime: > > We have: libcamera::controls::ExposureTime already > https://libcamera.org/api-html/namespacelibcamera_1_1controls.html#a4e1ca45653b62cd969d4d67a741076eb > > > + type: int32_t > > + description: | > > + The sensor exposure time in milliseconds. > > + > > + - FrameDuration: > > > And libcamera::controls::FrameDuration > > https://libcamera.org/api-html/namespacelibcamera_1_1controls.html#a37d99a76c7249c143beecd70917469be > > > > > + type: int64_t > > + description: | > > + The sensor frame duration time in milliseconds. > > + > > + - AnalogueGain: > > And ... libcamera::controls::AnalogueGain > > https://libcamera.org/api-html/namespacelibcamera_1_1controls.html#ab34ebeaa9cbfb3f3fc6996b089ca52b0 > > These are 'metadata' controls that are supposed to be returned by the > Pipeline handler in the metadata. But they mean exactly the same thing > here. > > So, is there a reason we aren't using them directly? Yes, the names are the same. Although the application visibile controls have additional semantic that the internal/camera_sensor controls do not have. controls::ExposureTime in example is not metadata only, but (will, once we finalize the AE controls rework) actually drive how the IPA handles auto/manual exposure, something which does not apply to the internal version. So we should append to each of the application visible controls a paragraph about their usage to drive the camera sensor, and we already have no clear division between controls/metadata in their definition which is already confusing sometimes. So, to me it's seems more appropriate to separate the controls space, also to let the two interface evolve separately, but maybe I'm just over-concerned and we could just re-use the public controls without even mentioning they're used internally, as applications do not care after all. > > -- > Kieran > > > + type: float > > + description: | > > + The sensor analogue gain value. > > + > > +... > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > index b57bee7ef6ca..89fdf347c708 100644 > > --- a/src/libcamera/meson.build > > +++ b/src/libcamera/meson.build > > @@ -52,6 +52,7 @@ libcamera_sources = files([ > > libcamera_sources += libcamera_public_headers > > libcamera_sources += libcamera_generated_ipa_headers > > libcamera_sources += libcamera_tracepoint_header > > +libcamera_sources += libcamera_internal_control_headers > > > > includes = [ > > libcamera_includes, > > @@ -99,6 +100,7 @@ if not libyaml.found() > > libyaml = libyaml_wrap.dependency('yaml') > > endif > > > > +# Generate control_ids.cpp and property_ids.cpp > > control_sources = [] > > > > foreach source : control_source_files > > @@ -111,6 +113,21 @@ endforeach > > > > libcamera_sources += control_sources > > > > +# Generate internal_control_ids.cpp > > +internal_control_source_files = ['control_ids'] > > +internal_control_sources = [] > > + > > +foreach source : internal_control_source_files > > + input_files = files('internal_' + source +'.yaml', source + '.cpp.in') > > + internal_control_sources += custom_target('internal_' + source + '_cpp', > > + input : input_files, > > + output : 'internal_' + source + '.cpp', > > + command : [gen_controls, '--internal=True',\ > > + '-o', '@OUTPUT@',\ > > + '@INPUT@']) > > +endforeach > > +libcamera_sources += internal_control_sources > > + > > gen_version = meson.project_source_root() / 'utils' / 'gen-version.sh' > > > > # Use vcs_tag() and not configure_file() or run_command(), to ensure that the > > -- > > 2.36.1 > >
Quoting Jacopo Mondi (2022-07-01 09:25:13) > Hi Kieran > > On Thu, Jun 30, 2022 at 10:06:27PM +0100, Kieran Bingham wrote: > > Quoting Jacopo Mondi via libcamera-devel (2022-06-30 14:38:42) > > > Introduce the enumeration of internal controls in > > > internal_control_ids.yaml. > > > > > > Plumb in the build system the command to generate the definition of > > > internal controls by re-using the same mechanism used for public > > > controls to make it easy to extend it to also handle internal properties > > > in future. > > > > I'm afraid these are bugging me ... Perhaps I missed this before when we > > discussed internal controls... > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org> > > > --- > > > include/libcamera/internal/meson.build | 15 ++++++++++++++ > > > src/libcamera/internal_control_ids.yaml | 27 +++++++++++++++++++++++++ > > > src/libcamera/meson.build | 17 ++++++++++++++++ > > > 3 files changed, 59 insertions(+) > > > create mode 100644 src/libcamera/internal_control_ids.yaml > > > > > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build > > > index 7a780d48ee57..1559c3c368c4 100644 > > > --- a/include/libcamera/internal/meson.build > > > +++ b/include/libcamera/internal/meson.build > > > @@ -9,6 +9,21 @@ libcamera_tracepoint_header = custom_target( > > > command: [gen_tracepoints_header, '@OUTPUT@', '@INPUT@'], > > > ) > > > > > > +# Generate the list of internal controls identifiers > > > +internal_control_source_files = ['control_ids'] > > > + > > > +libcamera_internal_control_headers = [] > > > + > > > +foreach header : internal_control_source_files > > > + input_files = files('../../../src/libcamera/internal_' + header +'.yaml',\ > > > + '../' + header + '.h.in') > > > + libcamera_internal_control_headers += custom_target( > > > + 'internal_' + header + '_h', > > > + input : input_files, > > > + output : header + '.h', > > > + command : [gen_controls, '--internal=True','-o', '@OUTPUT@', '@INPUT@']) > > > +endforeach > > > + > > > libcamera_internal_headers = files([ > > > 'bayer_format.h', > > > 'byte_stream_buffer.h', > > > diff --git a/src/libcamera/internal_control_ids.yaml b/src/libcamera/internal_control_ids.yaml > > > new file mode 100644 > > > index 000000000000..6d3775afcf67 > > > --- /dev/null > > > +++ b/src/libcamera/internal_control_ids.yaml > > > @@ -0,0 +1,27 @@ > > > +# SPDX-License-Identifier: LGPL-2.1-or-later > > > +# > > > +# Copyright (C) 2022, Google Inc. > > > +# > > > +%YAML 1.2 > > > +--- > > > +# Enumeration of internal libcamera controls > > > +# Not exposed to application, for library use only > > > + > > > +controls: > > > + > > > + - ExposureTime: > > > > We have: libcamera::controls::ExposureTime already > > https://libcamera.org/api-html/namespacelibcamera_1_1controls.html#a4e1ca45653b62cd969d4d67a741076eb > > > > > + type: int32_t > > > + description: | > > > + The sensor exposure time in milliseconds. > > > + > > > + - FrameDuration: > > > > > > And libcamera::controls::FrameDuration > > > > https://libcamera.org/api-html/namespacelibcamera_1_1controls.html#a37d99a76c7249c143beecd70917469be > > > > > > > > > + type: int64_t > > > + description: | > > > + The sensor frame duration time in milliseconds. > > > + > > > + - AnalogueGain: > > > > And ... libcamera::controls::AnalogueGain > > > > https://libcamera.org/api-html/namespacelibcamera_1_1controls.html#ab34ebeaa9cbfb3f3fc6996b089ca52b0 > > > > These are 'metadata' controls that are supposed to be returned by the > > Pipeline handler in the metadata. But they mean exactly the same thing > > here. > > > > So, is there a reason we aren't using them directly? > > Yes, the names are the same. > > Although the application visibile controls have additional semantic > that the internal/camera_sensor controls do not have. > > controls::ExposureTime in example is not metadata only, but (will, > once we finalize the AE controls rework) actually drive how the > IPA handles auto/manual exposure, something which does not apply to > the internal version. > > So we should append to each of the application visible controls a > paragraph about their usage to drive the camera sensor, and we already > have no clear division between controls/metadata in their definition > which is already confusing sometimes. > > So, to me it's seems more appropriate to separate the controls space, > also to let the two interface evolve separately, but maybe I'm just > over-concerned and we could just re-use the public controls without > even mentioning they're used internally, as applications do not care > after all. Well - to me the distinction would be ... could the metadata and the internal control ever have a different value to mean the same thing. That would warrant a different control namespace/scope - but as I understand it here, these internal controls should be the specific values that are exposed via metadata for each frame. -- Kieran > > > > > -- > > Kieran > > > > > + type: float > > > + description: | > > > + The sensor analogue gain value. > > > + > > > +... > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > > index b57bee7ef6ca..89fdf347c708 100644 > > > --- a/src/libcamera/meson.build > > > +++ b/src/libcamera/meson.build > > > @@ -52,6 +52,7 @@ libcamera_sources = files([ > > > libcamera_sources += libcamera_public_headers > > > libcamera_sources += libcamera_generated_ipa_headers > > > libcamera_sources += libcamera_tracepoint_header > > > +libcamera_sources += libcamera_internal_control_headers > > > > > > includes = [ > > > libcamera_includes, > > > @@ -99,6 +100,7 @@ if not libyaml.found() > > > libyaml = libyaml_wrap.dependency('yaml') > > > endif > > > > > > +# Generate control_ids.cpp and property_ids.cpp > > > control_sources = [] > > > > > > foreach source : control_source_files > > > @@ -111,6 +113,21 @@ endforeach > > > > > > libcamera_sources += control_sources > > > > > > +# Generate internal_control_ids.cpp > > > +internal_control_source_files = ['control_ids'] > > > +internal_control_sources = [] > > > + > > > +foreach source : internal_control_source_files > > > + input_files = files('internal_' + source +'.yaml', source + '.cpp.in') > > > + internal_control_sources += custom_target('internal_' + source + '_cpp', > > > + input : input_files, > > > + output : 'internal_' + source + '.cpp', > > > + command : [gen_controls, '--internal=True',\ > > > + '-o', '@OUTPUT@',\ > > > + '@INPUT@']) > > > +endforeach > > > +libcamera_sources += internal_control_sources > > > + > > > gen_version = meson.project_source_root() / 'utils' / 'gen-version.sh' > > > > > > # Use vcs_tag() and not configure_file() or run_command(), to ensure that the > > > -- > > > 2.36.1 > > >
diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build index 7a780d48ee57..1559c3c368c4 100644 --- a/include/libcamera/internal/meson.build +++ b/include/libcamera/internal/meson.build @@ -9,6 +9,21 @@ libcamera_tracepoint_header = custom_target( command: [gen_tracepoints_header, '@OUTPUT@', '@INPUT@'], ) +# Generate the list of internal controls identifiers +internal_control_source_files = ['control_ids'] + +libcamera_internal_control_headers = [] + +foreach header : internal_control_source_files + input_files = files('../../../src/libcamera/internal_' + header +'.yaml',\ + '../' + header + '.h.in') + libcamera_internal_control_headers += custom_target( + 'internal_' + header + '_h', + input : input_files, + output : header + '.h', + command : [gen_controls, '--internal=True','-o', '@OUTPUT@', '@INPUT@']) +endforeach + libcamera_internal_headers = files([ 'bayer_format.h', 'byte_stream_buffer.h', diff --git a/src/libcamera/internal_control_ids.yaml b/src/libcamera/internal_control_ids.yaml new file mode 100644 index 000000000000..6d3775afcf67 --- /dev/null +++ b/src/libcamera/internal_control_ids.yaml @@ -0,0 +1,27 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +# +# Copyright (C) 2022, Google Inc. +# +%YAML 1.2 +--- +# Enumeration of internal libcamera controls +# Not exposed to application, for library use only + +controls: + + - ExposureTime: + type: int32_t + description: | + The sensor exposure time in milliseconds. + + - FrameDuration: + type: int64_t + description: | + The sensor frame duration time in milliseconds. + + - AnalogueGain: + type: float + description: | + The sensor analogue gain value. + +... diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build index b57bee7ef6ca..89fdf347c708 100644 --- a/src/libcamera/meson.build +++ b/src/libcamera/meson.build @@ -52,6 +52,7 @@ libcamera_sources = files([ libcamera_sources += libcamera_public_headers libcamera_sources += libcamera_generated_ipa_headers libcamera_sources += libcamera_tracepoint_header +libcamera_sources += libcamera_internal_control_headers includes = [ libcamera_includes, @@ -99,6 +100,7 @@ if not libyaml.found() libyaml = libyaml_wrap.dependency('yaml') endif +# Generate control_ids.cpp and property_ids.cpp control_sources = [] foreach source : control_source_files @@ -111,6 +113,21 @@ endforeach libcamera_sources += control_sources +# Generate internal_control_ids.cpp +internal_control_source_files = ['control_ids'] +internal_control_sources = [] + +foreach source : internal_control_source_files + input_files = files('internal_' + source +'.yaml', source + '.cpp.in') + internal_control_sources += custom_target('internal_' + source + '_cpp', + input : input_files, + output : 'internal_' + source + '.cpp', + command : [gen_controls, '--internal=True',\ + '-o', '@OUTPUT@',\ + '@INPUT@']) +endforeach +libcamera_sources += internal_control_sources + gen_version = meson.project_source_root() / 'utils' / 'gen-version.sh' # Use vcs_tag() and not configure_file() or run_command(), to ensure that the