| Message ID | 20200709084128.5316-14-jacopo@jmondi.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2020-07-09 10:41:21 +0200, Jacopo Mondi wrote: > A reference to the CameraData sub-class is stored in the > IPU3CameraConfiguration as a const pointer, as the now removed comment > reports "to allow the compiler catch un-wanted modifications during > validate()". > > As the CameraData contains pointers to the available streams, which are > actually assigned during validate, the comment and the rationale behind > that choice seems now moot. > > Store CameraData as a mutable pointer and remove the comment and the > const_cast<> required to assign streams. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> I see now the comment gets removed in the end, which I think is nice :-) Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 16 ++++------------ > 1 file changed, 4 insertions(+), 12 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index feabffe641e1..9fed6c1e5aa7 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -75,7 +75,7 @@ private: > * reference to the camera data, store a new reference to the camera. > */ > std::shared_ptr<Camera> camera_; > - const IPU3CameraData *data_; > + IPU3CameraData *data_; > > StreamConfiguration cio2Configuration_; > std::vector<const Stream *> streams_; > @@ -207,7 +207,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > cfg.size = cio2Configuration_.size; > cfg.pixelFormat = cio2Configuration_.pixelFormat; > cfg.bufferCount = cio2Configuration_.bufferCount; > - cfg.setStream(const_cast<Stream *>(&data_->rawStream_)); > + cfg.setStream(&data_->rawStream_); > > LOG(IPU3, Debug) << "Assigned " << cfg.toString() > << " to the raw stream"; > @@ -256,27 +256,19 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > cfg.bufferCount = IPU3_BUFFER_COUNT; > cfg.pixelFormat = formats::NV12; > > - /* > - * Use a const_cast<> here instead of storing a mutable stream > - * pointer in the configuration to let the compiler catch > - * unwanted modifications of camera data in the configuration > - * validate() implementation. > - */ > - Stream *stream; > if (mainOutputAvailable && > (oldCfg.size == yuvSize || outCount == 1)) { > - stream = const_cast<Stream *>(&data_->outStream_); > + cfg.setStream(&data_->outStream_); > mainOutputAvailable = false; > > LOG(IPU3, Debug) << "Assigned " << cfg.toString() > << " to the main output"; > } else { > - stream = const_cast<Stream *>(&data_->vfStream_); > + cfg.setStream(&data_->vfStream_); > > LOG(IPU3, Debug) << "Assigned " << cfg.toString() > << " to the viewfinder output"; > } > - cfg.setStream(stream); > > if (cfg.pixelFormat != oldCfg.pixelFormat || > cfg.size != oldCfg.size) { > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thank you for the patch. On Thu, Jul 09, 2020 at 10:41:21AM +0200, Jacopo Mondi wrote: > A reference to the CameraData sub-class is stored in the > IPU3CameraConfiguration as a const pointer, as the now removed comment > reports "to allow the compiler catch un-wanted modifications during > validate()". > > As the CameraData contains pointers to the available streams, which are > actually assigned during validate, the comment and the rationale behind > that choice seems now moot. The idea is that IPU3CameraData models the active state of the camera, while IPU3CameraConfiguration models a configuration. The latter may need to access the former to query information, but should not modify it. That's why it's stored as a const pointer. I'd rather explore the option of storing a const Stream * in CameraConfiguration. > Store CameraData as a mutable pointer and remove the comment and the > const_cast<> required to assign streams. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 16 ++++------------ > 1 file changed, 4 insertions(+), 12 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index feabffe641e1..9fed6c1e5aa7 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -75,7 +75,7 @@ private: > * reference to the camera data, store a new reference to the camera. > */ > std::shared_ptr<Camera> camera_; > - const IPU3CameraData *data_; > + IPU3CameraData *data_; > > StreamConfiguration cio2Configuration_; > std::vector<const Stream *> streams_; > @@ -207,7 +207,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > cfg.size = cio2Configuration_.size; > cfg.pixelFormat = cio2Configuration_.pixelFormat; > cfg.bufferCount = cio2Configuration_.bufferCount; > - cfg.setStream(const_cast<Stream *>(&data_->rawStream_)); > + cfg.setStream(&data_->rawStream_); > > LOG(IPU3, Debug) << "Assigned " << cfg.toString() > << " to the raw stream"; > @@ -256,27 +256,19 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > cfg.bufferCount = IPU3_BUFFER_COUNT; > cfg.pixelFormat = formats::NV12; > > - /* > - * Use a const_cast<> here instead of storing a mutable stream > - * pointer in the configuration to let the compiler catch > - * unwanted modifications of camera data in the configuration > - * validate() implementation. > - */ > - Stream *stream; > if (mainOutputAvailable && > (oldCfg.size == yuvSize || outCount == 1)) { > - stream = const_cast<Stream *>(&data_->outStream_); > + cfg.setStream(&data_->outStream_); > mainOutputAvailable = false; > > LOG(IPU3, Debug) << "Assigned " << cfg.toString() > << " to the main output"; > } else { > - stream = const_cast<Stream *>(&data_->vfStream_); > + cfg.setStream(&data_->vfStream_); > > LOG(IPU3, Debug) << "Assigned " << cfg.toString() > << " to the viewfinder output"; > } > - cfg.setStream(stream); > > if (cfg.pixelFormat != oldCfg.pixelFormat || > cfg.size != oldCfg.size) {
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index feabffe641e1..9fed6c1e5aa7 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -75,7 +75,7 @@ private: * reference to the camera data, store a new reference to the camera. */ std::shared_ptr<Camera> camera_; - const IPU3CameraData *data_; + IPU3CameraData *data_; StreamConfiguration cio2Configuration_; std::vector<const Stream *> streams_; @@ -207,7 +207,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() cfg.size = cio2Configuration_.size; cfg.pixelFormat = cio2Configuration_.pixelFormat; cfg.bufferCount = cio2Configuration_.bufferCount; - cfg.setStream(const_cast<Stream *>(&data_->rawStream_)); + cfg.setStream(&data_->rawStream_); LOG(IPU3, Debug) << "Assigned " << cfg.toString() << " to the raw stream"; @@ -256,27 +256,19 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() cfg.bufferCount = IPU3_BUFFER_COUNT; cfg.pixelFormat = formats::NV12; - /* - * Use a const_cast<> here instead of storing a mutable stream - * pointer in the configuration to let the compiler catch - * unwanted modifications of camera data in the configuration - * validate() implementation. - */ - Stream *stream; if (mainOutputAvailable && (oldCfg.size == yuvSize || outCount == 1)) { - stream = const_cast<Stream *>(&data_->outStream_); + cfg.setStream(&data_->outStream_); mainOutputAvailable = false; LOG(IPU3, Debug) << "Assigned " << cfg.toString() << " to the main output"; } else { - stream = const_cast<Stream *>(&data_->vfStream_); + cfg.setStream(&data_->vfStream_); LOG(IPU3, Debug) << "Assigned " << cfg.toString() << " to the viewfinder output"; } - cfg.setStream(stream); if (cfg.pixelFormat != oldCfg.pixelFormat || cfg.size != oldCfg.size) {
A reference to the CameraData sub-class is stored in the IPU3CameraConfiguration as a const pointer, as the now removed comment reports "to allow the compiler catch un-wanted modifications during validate()". As the CameraData contains pointers to the available streams, which are actually assigned during validate, the comment and the rationale behind that choice seems now moot. Store CameraData as a mutable pointer and remove the comment and the const_cast<> required to assign streams. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)