| Message ID | 20201209055410.3232987-1-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, On 09/12/2020 05:54, Hirokazu Honda wrote: > Camera3StreamConfig is a new class to store camera3_stream and > types with associated StreamConfiguration. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Perhaps this could be squashed with 2/3 which uses it - but w're at v5 here, so I don't think it's worth it now. It looks like this series is likely ready for merging, so no need to repost just to squash. > --- > src/android/camera_device.cpp | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 675af570..09269d95 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -128,6 +128,18 @@ const std::map<int, const Camera3Format> camera3FormatsMap = { > }, > }; > > +/* > + * \struct Camera3StreamConfig > + * \brief Data to store StreamConfiguration associated with camera3_stream(s). > + * \var streams List of streams requested by Android HAL client. > + * \var types List of CameraStream::Type associated with streams. > + * \var config StreamConfiguration for streams. > + */ > +struct Camera3StreamConfig { > + std::vector<camera3_stream_t*> streams; > + std::vector<CameraStream::Type> types; As far as i can tell, in 2/3 both streams and types are always used at the same index? If that's the case, I would have preferred to see them grouped directly so that it is enforced. That could either be by putting a std::pair in the vector (though I'm not sure if that gets a bit ugly) or grouping them together in a struct ... but then we have yet another type... So - I'm not going to say that's needed right now - but I'm just cautious of adding lots of multiple data types that require the same indexing, rather than putting the grouped data together, and stored inside a single data structure... In this case, I don't think it matters as it's just semantics for accessing the data. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + StreamConfiguration config; > +}; > } /* namespace */ > > LOG_DECLARE_CATEGORY(HAL) > -- > 2.29.2.576.ga3fc446d84-goog > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Kieran, On Wed, Dec 09, 2020 at 10:31:09AM +0000, Kieran Bingham wrote: > Hi Hiro, > > On 09/12/2020 05:54, Hirokazu Honda wrote: > > Camera3StreamConfig is a new class to store camera3_stream and > > types with associated StreamConfiguration. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Perhaps this could be squashed with 2/3 which uses it - but w're at v5 > here, so I don't think it's worth it now. > > It looks like this series is likely ready for merging, so no need to > repost just to squash. I asked patches to be separate. If there are no other review comments that require a new iteration I agree this can be squashed when applying if desired. > > > --- > > src/android/camera_device.cpp | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 675af570..09269d95 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -128,6 +128,18 @@ const std::map<int, const Camera3Format> camera3FormatsMap = { > > }, > > }; > > > > +/* > > + * \struct Camera3StreamConfig > > + * \brief Data to store StreamConfiguration associated with camera3_stream(s). > > + * \var streams List of streams requested by Android HAL client. > > + * \var types List of CameraStream::Type associated with streams. > > + * \var config StreamConfiguration for streams. > > + */ > > +struct Camera3StreamConfig { > > + std::vector<camera3_stream_t*> streams; > > + std::vector<CameraStream::Type> types; > > As far as i can tell, in 2/3 both streams and types are always used at > the same index? > > If that's the case, I would have preferred to see them grouped directly > so that it is enforced. > > That could either be by putting a std::pair in the vector (though I'm > not sure if that gets a bit ugly) or grouping them together in a struct > ... but then we have yet another type... > > So - I'm not going to say that's needed right now - but I'm just > cautious of adding lots of multiple data types that require the same > indexing, rather than putting the grouped data together, and stored > inside a single data structure... > > In this case, I don't think it matters as it's just semantics for > accessing the data. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > + StreamConfiguration config; > > +}; > > } /* namespace */ > > > > LOG_DECLARE_CATEGORY(HAL) > > -- > > 2.29.2.576.ga3fc446d84-goog > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Regards > -- > Kieran > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran On 12/9/20 4:01 PM, Kieran Bingham wrote: > Hi Hiro, > > On 09/12/2020 05:54, Hirokazu Honda wrote: >> Camera3StreamConfig is a new class to store camera3_stream and >> types with associated StreamConfiguration. >> >> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> >> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > Perhaps this could be squashed with 2/3 which uses it - but w're at v5 > here, so I don't think it's worth it now. > > It looks like this series is likely ready for merging, so no need to > repost just to squash. > >> --- >> src/android/camera_device.cpp | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index 675af570..09269d95 100644 >> --- a/src/android/camera_device.cpp >> +++ b/src/android/camera_device.cpp >> @@ -128,6 +128,18 @@ const std::map<int, const Camera3Format> camera3FormatsMap = { >> }, >> }; >> >> +/* >> + * \struct Camera3StreamConfig >> + * \brief Data to store StreamConfiguration associated with camera3_stream(s). >> + * \var streams List of streams requested by Android HAL client. >> + * \var types List of CameraStream::Type associated with streams. >> + * \var config StreamConfiguration for streams. >> + */ >> +struct Camera3StreamConfig { >> + std::vector<camera3_stream_t*> streams; >> + std::vector<CameraStream::Type> types; > As far as i can tell, in 2/3 both streams and types are always used at > the same index? > > If that's the case, I would have preferred to see them grouped directly > so that it is enforced. > > That could either be by putting a std::pair in the vector (though I'm > not sure if that gets a bit ugly) or grouping them together in a struct > ... but then we have yet another type... > > So - I'm not going to say that's needed right now - but I'm just > cautious of adding lots of multiple data types that require the same > indexing, rather than putting the grouped data together, and stored > inside a single data structure... This is my concern constantly while reviewing the series, but I have failed to come-up with a solution so I should not complain. :) I got the idea and it seems enough people have eyes on this, so probably this can go ahead. > In this case, I don't think it matters as it's just semantics for > accessing the data. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> + StreamConfiguration config; >> +}; >> } /* namespace */ >> >> LOG_DECLARE_CATEGORY(HAL) >> -- >> 2.29.2.576.ga3fc446d84-goog >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >>
Hi Hiro, On 12/9/20 11:24 AM, Hirokazu Honda wrote: > Camera3StreamConfig is a new class to store camera3_stream and > types with associated StreamConfiguration. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Umang Jain <email@uajain.com> > --- > src/android/camera_device.cpp | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 675af570..09269d95 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -128,6 +128,18 @@ const std::map<int, const Camera3Format> camera3FormatsMap = { > }, > }; > > +/* > + * \struct Camera3StreamConfig > + * \brief Data to store StreamConfiguration associated with camera3_stream(s). > + * \var streams List of streams requested by Android HAL client. > + * \var types List of CameraStream::Type associated with streams. > + * \var config StreamConfiguration for streams. > + */ > +struct Camera3StreamConfig { > + std::vector<camera3_stream_t*> streams; > + std::vector<CameraStream::Type> types; > + StreamConfiguration config; > +}; > } /* namespace */ > > LOG_DECLARE_CATEGORY(HAL) > -- > 2.29.2.576.ga3fc446d84-goog > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Hiro and Kieran, On Wed, Dec 09, 2020 at 10:31:09AM +0000, Kieran Bingham wrote: > On 09/12/2020 05:54, Hirokazu Honda wrote: > > Camera3StreamConfig is a new class to store camera3_stream and > > types with associated StreamConfiguration. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Perhaps this could be squashed with 2/3 which uses it - but w're at v5 > here, so I don't think it's worth it now. > > It looks like this series is likely ready for merging, so no need to > repost just to squash. > > > --- > > src/android/camera_device.cpp | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 675af570..09269d95 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -128,6 +128,18 @@ const std::map<int, const Camera3Format> camera3FormatsMap = { > > }, > > }; > > > > +/* > > + * \struct Camera3StreamConfig > > + * \brief Data to store StreamConfiguration associated with camera3_stream(s). > > + * \var streams List of streams requested by Android HAL client. > > + * \var types List of CameraStream::Type associated with streams. > > + * \var config StreamConfiguration for streams. > > + */ > > +struct Camera3StreamConfig { > > + std::vector<camera3_stream_t*> streams; > > + std::vector<CameraStream::Type> types; > > As far as i can tell, in 2/3 both streams and types are always used at > the same index? > > If that's the case, I would have preferred to see them grouped directly > so that it is enforced. > > That could either be by putting a std::pair in the vector (though I'm > not sure if that gets a bit ugly) or grouping them together in a struct > ... but then we have yet another type... > > So - I'm not going to say that's needed right now - but I'm just > cautious of adding lots of multiple data types that require the same > indexing, rather than putting the grouped data together, and stored > inside a single data structure... > > In this case, I don't think it matters as it's just semantics for > accessing the data. Certainly not a blocker, so, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I would however also prefer grouping the two in the same vector. Something like the following could do. struct Camera3StreamConfig { struct Camera3Stream { camera3_stream_t *stream; CameraStream::Type type; }; StreamConfiguration config; std::vector<Camera3Stream> streams; }; At some point I think we need to go through the HAL implementation to clean up the naming scheme, it's not very consistent. A common prefix for data types relating to the HAL, as well as a naming scheme for variables, would be nice. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > + StreamConfiguration config; > > +}; > > } /* namespace */ > > > > LOG_DECLARE_CATEGORY(HAL)
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 675af570..09269d95 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -128,6 +128,18 @@ const std::map<int, const Camera3Format> camera3FormatsMap = { }, }; +/* + * \struct Camera3StreamConfig + * \brief Data to store StreamConfiguration associated with camera3_stream(s). + * \var streams List of streams requested by Android HAL client. + * \var types List of CameraStream::Type associated with streams. + * \var config StreamConfiguration for streams. + */ +struct Camera3StreamConfig { + std::vector<camera3_stream_t*> streams; + std::vector<CameraStream::Type> types; + StreamConfiguration config; +}; } /* namespace */ LOG_DECLARE_CATEGORY(HAL)