| Message ID | 20210421160319.42251-8-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2021-04-21 18:03:10 +0200, Jacopo Mondi wrote: > When a Request is completed upon receiving the IPA produced metadata, > the metadata associated with the Request are over-written, deleting > the information set at output buffer completion, such as the > SensorTimestamp. > > This commit applies to the RkISP1 pipeline handler the same change > applied to IPU3 in commit 13a7ed7b1f1f ("libcamera: ipu3: Do not > over-write metadata") but compared to that commit it uses the newly > introduced ControlList::merge() function. > > Fixes: 0eb65e14e18d ("libcamera: pipeline: rkisp1: Attach to an IPA") > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 549f4a4e61a8..c3d390f775f2 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -362,7 +362,7 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta > if (!info) > return; > > - info->request->metadata() = metadata; > + info->request->metadata().merge(const_cast<ControlList &>(metadata)); I like the change but this cast makes me think twice of the merge() signature. Could we make the argument a const reference? > info->metadataProcessed = true; > > pipe->tryCompleteRequest(info->request); > -- > 2.31.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi again Jacopo, On 2021-04-21 18:03:10 +0200, Jacopo Mondi wrote: > When a Request is completed upon receiving the IPA produced metadata, > the metadata associated with the Request are over-written, deleting > the information set at output buffer completion, such as the > SensorTimestamp. > > This commit applies to the RkISP1 pipeline handler the same change > applied to IPU3 in commit 13a7ed7b1f1f ("libcamera: ipu3: Do not > over-write metadata") but compared to that commit it uses the newly > introduced ControlList::merge() function. > > Fixes: 0eb65e14e18d ("libcamera: pipeline: rkisp1: Attach to an IPA") This fixes tag is incorrect. Testing on master the only metadata recorded in the request is the AeLock. Applying this series the only metadata reported is the AeLock and SensorTimestamp, and the timestamp is added by the next patch in this series. There is no regression here to fix, only preparation for additional work, or am I missing something? > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 549f4a4e61a8..c3d390f775f2 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -362,7 +362,7 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta > if (!info) > return; > > - info->request->metadata() = metadata; > + info->request->metadata().merge(const_cast<ControlList &>(metadata)); > info->metadataProcessed = true; > > pipe->tryCompleteRequest(info->request); > -- > 2.31.1 > > _______________________________________________ > 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, Apr 22, 2021 at 7:03 AM Niklas Söderlund <niklas.soderlund@ragnatech.se> wrote: > > Hi again Jacopo, > > On 2021-04-21 18:03:10 +0200, Jacopo Mondi wrote: > > When a Request is completed upon receiving the IPA produced metadata, > > the metadata associated with the Request are over-written, deleting > > the information set at output buffer completion, such as the > > SensorTimestamp. > > > > This commit applies to the RkISP1 pipeline handler the same change > > applied to IPU3 in commit 13a7ed7b1f1f ("libcamera: ipu3: Do not > > over-write metadata") but compared to that commit it uses the newly > > introduced ControlList::merge() function. > > > > Fixes: 0eb65e14e18d ("libcamera: pipeline: rkisp1: Attach to an IPA") > > This fixes tag is incorrect. Testing on master the only metadata > recorded in the request is the AeLock. Applying this series the only > metadata reported is the AeLock and SensorTimestamp, and the timestamp > is added by the next patch in this series. There is no regression here > to fix, only preparation for additional work, or am I missing something? > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 549f4a4e61a8..c3d390f775f2 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -362,7 +362,7 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta > > if (!info) > > return; > > > > - info->request->metadata() = metadata; > > + info->request->metadata().merge(const_cast<ControlList &>(metadata)); > > info->metadataProcessed = true; > > > I like the change but this cast makes me think twice of the merge() > signature. Could we make the argument a const reference? I think there are two solutions, 1.) drop const of action in queueFrameAction. 2.) merge doesn't change a given ControlList. We currently use std::unordered_map::merge() for efficiency, which doesn't copy nodes among maps. I expect the copy of ControlValue is basically cheap, but this copy might be expensive depending on the number of nodes to be copied. I don't loot at the serialization code during IPA communication, but perhaps, the cost between cost reference and value are the same? Then, (1) is definitely better in terms of the efficiency. I would like to hear others' opinions. -Hiro > > pipe->tryCompleteRequest(info->request); > > -- > > 2.31.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Hiro, Niklas, On Thu, Apr 22, 2021 at 01:53:45PM +0900, Hirokazu Honda wrote: > Hi Jacopo, thank you for the patch, > > On Thu, Apr 22, 2021 at 7:03 AM Niklas Söderlund > <niklas.soderlund@ragnatech.se> wrote: > > > > Hi again Jacopo, > > > > On 2021-04-21 18:03:10 +0200, Jacopo Mondi wrote: > > > When a Request is completed upon receiving the IPA produced metadata, > > > the metadata associated with the Request are over-written, deleting > > > the information set at output buffer completion, such as the > > > SensorTimestamp. > > > > > > This commit applies to the RkISP1 pipeline handler the same change > > > applied to IPU3 in commit 13a7ed7b1f1f ("libcamera: ipu3: Do not > > > over-write metadata") but compared to that commit it uses the newly > > > introduced ControlList::merge() function. > > > > > > Fixes: 0eb65e14e18d ("libcamera: pipeline: rkisp1: Attach to an IPA") > > > > This fixes tag is incorrect. Testing on master the only metadata > > recorded in the request is the AeLock. Applying this series the only > > metadata reported is the AeLock and SensorTimestamp, and the timestamp > > is added by the next patch in this series. There is no regression here > > to fix, only preparation for additional work, or am I missing something? > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > index 549f4a4e61a8..c3d390f775f2 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -362,7 +362,7 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta > > > if (!info) > > > return; > > > > > > - info->request->metadata() = metadata; > > > + info->request->metadata().merge(const_cast<ControlList &>(metadata)); > > > info->metadataProcessed = true; > > > > > > > I like the change but this cast makes me think twice of the merge() > > signature. Could we make the argument a const reference? We currently can't as ControlList::merge() is implemented by wrapping std::unordered_map::merge() as Hiro explained > > I think there are two solutions, > 1.) drop const of action in queueFrameAction. > 2.) merge doesn't change a given ControlList. > > We currently use std::unordered_map::merge() for efficiency, which > doesn't copy nodes among maps. > I expect the copy of ControlValue is basically cheap, but this copy > might be expensive depending on the number of nodes to be copied. The number of nodes and their content, as we can transport Span<> of values > I don't loot at the serialization code during IPA communication, but > perhaps, the cost between cost reference and value are the same? > Then, (1) is definitely better in terms of the efficiency. > I would like to hear others' opinions. > I see your concerns about performaces, and in this specific case I think we can safely drop the const from the queueFrameAction ControlList parameter. Even better, if after merge there are elements in the list passed as argument we can catch a development issue earlier (an attempt to overwrite a metadata which has been set already). Anyway a merge() version which takes a const ControlList, and is implemented by copying values might be desirable. Thanks j > -Hiro > > > > pipe->tryCompleteRequest(info->request); > > > -- > > > 2.31.1 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards, > > Niklas Söderlund > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Thu, Apr 22, 2021 at 12:03:42AM +0200, Niklas Söderlund wrote: > Hi again Jacopo, > > On 2021-04-21 18:03:10 +0200, Jacopo Mondi wrote: > > When a Request is completed upon receiving the IPA produced metadata, > > the metadata associated with the Request are over-written, deleting > > the information set at output buffer completion, such as the > > SensorTimestamp. > > > > This commit applies to the RkISP1 pipeline handler the same change > > applied to IPU3 in commit 13a7ed7b1f1f ("libcamera: ipu3: Do not > > over-write metadata") but compared to that commit it uses the newly > > introduced ControlList::merge() function. > > > > Fixes: 0eb65e14e18d ("libcamera: pipeline: rkisp1: Attach to an IPA") > > This fixes tag is incorrect. Testing on master the only metadata > recorded in the request is the AeLock. Applying this series the only > metadata reported is the AeLock and SensorTimestamp, and the timestamp > is added by the next patch in this series. There is no regression here > to fix, only preparation for additional work, or am I missing something? > Dunno, I think blindly overwriting metadata is wrong, but if the tag makes you unconfortable I'll drop it. Tbh I don't even know why we use Fixes, since we have nowhere to backport too > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 549f4a4e61a8..c3d390f775f2 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -362,7 +362,7 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta > > if (!info) > > return; > > > > - info->request->metadata() = metadata; > > + info->request->metadata().merge(const_cast<ControlList &>(metadata)); > > info->metadataProcessed = true; > > > > pipe->tryCompleteRequest(info->request); > > -- > > 2.31.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
Hi Jacopo, On 2021-04-22 09:06:16 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Thu, Apr 22, 2021 at 12:03:42AM +0200, Niklas Söderlund wrote: > > Hi again Jacopo, > > > > On 2021-04-21 18:03:10 +0200, Jacopo Mondi wrote: > > > When a Request is completed upon receiving the IPA produced metadata, > > > the metadata associated with the Request are over-written, deleting > > > the information set at output buffer completion, such as the > > > SensorTimestamp. > > > > > > This commit applies to the RkISP1 pipeline handler the same change > > > applied to IPU3 in commit 13a7ed7b1f1f ("libcamera: ipu3: Do not > > > over-write metadata") but compared to that commit it uses the newly > > > introduced ControlList::merge() function. > > > > > > Fixes: 0eb65e14e18d ("libcamera: pipeline: rkisp1: Attach to an IPA") > > > > This fixes tag is incorrect. Testing on master the only metadata > > recorded in the request is the AeLock. Applying this series the only > > metadata reported is the AeLock and SensorTimestamp, and the timestamp > > is added by the next patch in this series. There is no regression here > > to fix, only preparation for additional work, or am I missing something? > > > > Dunno, I think blindly overwriting metadata is wrong, but if the tag > makes you unconfortable I'll drop it. I agree that overwriting stuff is bad, but the commit in question does not do that, before it there was no metadata at all :-) > > Tbh I don't even know why we use Fixes, since we have nowhere to > backport too I like the Fixes tags. When you bisect issues and hit a bad commit that have a Fixes tag you get way more context on what's going on. That's why I think in this particular case the Fixes tag is wrong as if I hit this commit when bisecting the Fixes tag would send me on a wild goose chase as this commit does not fix an issue that did not exist in 0eb65e14e18d~1 but does in 0eb65e14e18d. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > index 549f4a4e61a8..c3d390f775f2 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -362,7 +362,7 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta > > > if (!info) > > > return; > > > > > > - info->request->metadata() = metadata; > > > + info->request->metadata().merge(const_cast<ControlList &>(metadata)); > > > info->metadataProcessed = true; > > > > > > pipe->tryCompleteRequest(info->request); > > > -- > > > 2.31.1 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards, > > Niklas Söderlund
Hi Jacopo, On Thu, Apr 22, 2021 at 09:04:44AM +0200, Jacopo Mondi wrote: > On Thu, Apr 22, 2021 at 01:53:45PM +0900, Hirokazu Honda wrote: > > On Thu, Apr 22, 2021 at 7:03 AM Niklas Söderlund wrote: > > > On 2021-04-21 18:03:10 +0200, Jacopo Mondi wrote: > > > > When a Request is completed upon receiving the IPA produced metadata, > > > > the metadata associated with the Request are over-written, deleting > > > > the information set at output buffer completion, such as the > > > > SensorTimestamp. > > > > > > > > This commit applies to the RkISP1 pipeline handler the same change > > > > applied to IPU3 in commit 13a7ed7b1f1f ("libcamera: ipu3: Do not > > > > over-write metadata") but compared to that commit it uses the newly > > > > introduced ControlList::merge() function. > > > > > > > > Fixes: 0eb65e14e18d ("libcamera: pipeline: rkisp1: Attach to an IPA") > > > > > > This fixes tag is incorrect. Testing on master the only metadata > > > recorded in the request is the AeLock. Applying this series the only > > > metadata reported is the AeLock and SensorTimestamp, and the timestamp > > > is added by the next patch in this series. There is no regression here > > > to fix, only preparation for additional work, or am I missing something? > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > index 549f4a4e61a8..c3d390f775f2 100644 > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > @@ -362,7 +362,7 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta > > > > if (!info) > > > > return; > > > > > > > > - info->request->metadata() = metadata; > > > > + info->request->metadata().merge(const_cast<ControlList &>(metadata)); > > > > info->metadataProcessed = true; > > > > > > I like the change but this cast makes me think twice of the merge() > > > signature. Could we make the argument a const reference? Ouch. const_cast<> is very often a sign of a design issue. > We currently can't as ControlList::merge() is implemented by wrapping > std::unordered_map::merge() as Hiro explained > > > I think there are two solutions, > > 1.) drop const of action in queueFrameAction. > > 2.) merge doesn't change a given ControlList. > > > > We currently use std::unordered_map::merge() for efficiency, which > > doesn't copy nodes among maps. > > I expect the copy of ControlValue is basically cheap, but this copy > > might be expensive depending on the number of nodes to be copied. > > The number of nodes and their content, as we can transport Span<> of > values > > > I don't loot at the serialization code during IPA communication, but > > perhaps, the cost between cost reference and value are the same? > > Then, (1) is definitely better in terms of the efficiency. > > I would like to hear others' opinions. > > I see your concerns about performaces, and in this specific case I > think we can safely drop the const from the queueFrameAction > ControlList parameter. Even better, if after merge there are elements > in the list passed as argument we can catch a development issue > earlier (an attempt to overwrite a metadata which has been set already). I also think this is better, and I like the idea of being able to check if metadata is empty after the merge. You'll run into an issue though, as the IPA IPC code generator inserts the const automatically. To avoid delay in this patch series, I'd recommend modifying ControlList::merge() to take a const reference and copy elements, with a \todo to mention we should optimize it. > Anyway a merge() version which takes a const ControlList, and is > implemented by copying values might be desirable. > > > > > pipe->tryCompleteRequest(info->request);
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 549f4a4e61a8..c3d390f775f2 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -362,7 +362,7 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta if (!info) return; - info->request->metadata() = metadata; + info->request->metadata().merge(const_cast<ControlList &>(metadata)); info->metadataProcessed = true; pipe->tryCompleteRequest(info->request);
When a Request is completed upon receiving the IPA produced metadata, the metadata associated with the Request are over-written, deleting the information set at output buffer completion, such as the SensorTimestamp. This commit applies to the RkISP1 pipeline handler the same change applied to IPU3 in commit 13a7ed7b1f1f ("libcamera: ipu3: Do not over-write metadata") but compared to that commit it uses the newly introduced ControlList::merge() function. Fixes: 0eb65e14e18d ("libcamera: pipeline: rkisp1: Attach to an IPA") Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)