| Message ID | 20210816043138.957984-3-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. On Mon, Aug 16, 2021 at 01:31:30PM +0900, Hirokazu Honda wrote: > The offset variable is added to FrameBuffer::Plane. This modifies > the serialization and deserialization code for the offset variable. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/ipa_data_serializer.cpp | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp > index fb941e6b..144c3248 100644 > --- a/src/libcamera/ipa_data_serializer.cpp > +++ b/src/libcamera/ipa_data_serializer.cpp > @@ -562,6 +562,7 @@ FileDescriptor IPADataSerializer<FileDescriptor>::deserialize(const std::vector< > * FrameBuffer::Plane is serialized as: > * > * 1 byte - FileDescriptor > + * 4 bytes - uint32_t Offset > * 4 bytes - uint32_t Length > */ > template<> > @@ -580,6 +581,7 @@ IPADataSerializer<FrameBuffer::Plane>::serialize(const FrameBuffer::Plane &data, > fdsVec.insert(fdsVec.end(), fdFds.begin(), fdFds.end()); > > appendPOD<uint32_t>(dataVec, data.length); > + appendPOD<uint32_t>(dataVec, data.offset); > > return { dataVec, fdsVec }; > } > @@ -596,7 +598,8 @@ IPADataSerializer<FrameBuffer::Plane>::deserialize(std::vector<uint8_t>::const_i > > ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 1, > fdsBegin, fdsBegin + 1); > - ret.length = readPOD<uint32_t>(dataBegin, 1, dataEnd); > + ret.offset = readPOD<uint32_t>(dataBegin, 1, dataBegin + 2); > + ret.length = readPOD<uint32_t>(dataBegin + 1, 1, dataEnd); > > return ret; > }
Hello, On Mon, Aug 16, 2021 at 01:31:30PM +0900, Hirokazu Honda wrote: > The offset variable is added to FrameBuffer::Plane. This modifies > the serialization and deserialization code for the offset variable. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/libcamera/ipa_data_serializer.cpp | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp > index fb941e6b..144c3248 100644 > --- a/src/libcamera/ipa_data_serializer.cpp > +++ b/src/libcamera/ipa_data_serializer.cpp > @@ -562,6 +562,7 @@ FileDescriptor IPADataSerializer<FileDescriptor>::deserialize(const std::vector< > * FrameBuffer::Plane is serialized as: > * > * 1 byte - FileDescriptor > + * 4 bytes - uint32_t Offset > * 4 bytes - uint32_t Length > */ > template<> > @@ -580,6 +581,7 @@ IPADataSerializer<FrameBuffer::Plane>::serialize(const FrameBuffer::Plane &data, > fdsVec.insert(fdsVec.end(), fdFds.begin(), fdFds.end()); > > appendPOD<uint32_t>(dataVec, data.length); > + appendPOD<uint32_t>(dataVec, data.offset); > > return { dataVec, fdsVec }; > } > @@ -596,7 +598,8 @@ IPADataSerializer<FrameBuffer::Plane>::deserialize(std::vector<uint8_t>::const_i > > ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 1, > fdsBegin, fdsBegin + 1); > - ret.length = readPOD<uint32_t>(dataBegin, 1, dataEnd); > + ret.offset = readPOD<uint32_t>(dataBegin, 1, dataBegin + 2); > + ret.length = readPOD<uint32_t>(dataBegin + 1, 1, dataEnd); Am I wrong ot offset is appeneded at the end of dataVec (after lenght) and then read from the beginning (before lenght) ? > > return ret; > } > -- > 2.33.0.rc1.237.g0d66db33f3-goog >
Hi, On Wed, Aug 18, 2021 at 11:58:29AM +0200, Jacopo Mondi wrote: > On Mon, Aug 16, 2021 at 01:31:30PM +0900, Hirokazu Honda wrote: > > The offset variable is added to FrameBuffer::Plane. This modifies > > the serialization and deserialization code for the offset variable. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > src/libcamera/ipa_data_serializer.cpp | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp > > index fb941e6b..144c3248 100644 > > --- a/src/libcamera/ipa_data_serializer.cpp > > +++ b/src/libcamera/ipa_data_serializer.cpp > > @@ -562,6 +562,7 @@ FileDescriptor IPADataSerializer<FileDescriptor>::deserialize(const std::vector< > > * FrameBuffer::Plane is serialized as: > > * > > * 1 byte - FileDescriptor > > + * 4 bytes - uint32_t Offset > > * 4 bytes - uint32_t Length > > */ > > template<> > > @@ -580,6 +581,7 @@ IPADataSerializer<FrameBuffer::Plane>::serialize(const FrameBuffer::Plane &data, > > fdsVec.insert(fdsVec.end(), fdFds.begin(), fdFds.end()); > > > > appendPOD<uint32_t>(dataVec, data.length); > > + appendPOD<uint32_t>(dataVec, data.offset); > > > > return { dataVec, fdsVec }; > > } > > @@ -596,7 +598,8 @@ IPADataSerializer<FrameBuffer::Plane>::deserialize(std::vector<uint8_t>::const_i > > > > ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 1, > > fdsBegin, fdsBegin + 1); > > - ret.length = readPOD<uint32_t>(dataBegin, 1, dataEnd); > > + ret.offset = readPOD<uint32_t>(dataBegin, 1, dataBegin + 2); > > + ret.length = readPOD<uint32_t>(dataBegin + 1, 1, dataEnd); > > Am I wrong ot offset is appeneded at the end of dataVec (after lenght) and > then read from the beginning (before lenght) ? How did I miss that ? :-S Let's serialize the fields in the order they're defined in the structure. Also, the dataBegin + 2 isn't correct, deserializing an uint32_t requires 4 bytes, and dataBegin + 1 is thus incorrect too. Paul, is there an easy way to get the serdes code for FrameBuffer::Plane generated ? > > > > return ret; > > }
Hi, On Wed, Aug 18, 2021 at 01:22:02PM +0300, Laurent Pinchart wrote: > Hi, > > On Wed, Aug 18, 2021 at 11:58:29AM +0200, Jacopo Mondi wrote: > > On Mon, Aug 16, 2021 at 01:31:30PM +0900, Hirokazu Honda wrote: > > > The offset variable is added to FrameBuffer::Plane. This modifies > > > the serialization and deserialization code for the offset variable. > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > --- > > > src/libcamera/ipa_data_serializer.cpp | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp > > > index fb941e6b..144c3248 100644 > > > --- a/src/libcamera/ipa_data_serializer.cpp > > > +++ b/src/libcamera/ipa_data_serializer.cpp > > > @@ -562,6 +562,7 @@ FileDescriptor IPADataSerializer<FileDescriptor>::deserialize(const std::vector< > > > * FrameBuffer::Plane is serialized as: > > > * > > > * 1 byte - FileDescriptor > > > + * 4 bytes - uint32_t Offset > > > * 4 bytes - uint32_t Length > > > */ > > > template<> > > > @@ -580,6 +581,7 @@ IPADataSerializer<FrameBuffer::Plane>::serialize(const FrameBuffer::Plane &data, > > > fdsVec.insert(fdsVec.end(), fdFds.begin(), fdFds.end()); > > > > > > appendPOD<uint32_t>(dataVec, data.length); > > > + appendPOD<uint32_t>(dataVec, data.offset); > > > > > > return { dataVec, fdsVec }; > > > } > > > @@ -596,7 +598,8 @@ IPADataSerializer<FrameBuffer::Plane>::deserialize(std::vector<uint8_t>::const_i > > > > > > ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 1, > > > fdsBegin, fdsBegin + 1); > > > - ret.length = readPOD<uint32_t>(dataBegin, 1, dataEnd); > > > + ret.offset = readPOD<uint32_t>(dataBegin, 1, dataBegin + 2); > > > + ret.length = readPOD<uint32_t>(dataBegin + 1, 1, dataEnd); > > > > Am I wrong ot offset is appeneded at the end of dataVec (after lenght) and > > then read from the beginning (before lenght) ? Yes, the serialize order needs to match the deserialize order. The definition is fd, offset, length, so I would serialize in that order too (the deserializer has that order already). > > How did I miss that ? :-S Let's serialize the fields in the order > they're defined in the structure. Also, the dataBegin + 2 isn't correct, Okay, that's the fault of readPOD() being poorly (read: not having been) documentated. readPOD() takes the begin iterator, the position, and the end iterator. So it should be: - ret.length = readPOD<uint32_t>(dataBegin, 1, dataEnd); + ret.offset = readPOD<uint32_t>(dataBegin, 1, dataEnd); + ret.length = readPOD<uint32_t>(dataBegin, 5, dataEnd); (I'm confused about from where dataBegin + 2 came from in the first place...) > deserializing an uint32_t requires 4 bytes, and dataBegin + 1 is thus > incorrect too. > > Paul, is there an easy way to get the serdes code for > FrameBuffer::Plane generated ? No, because mojo doesn't support defining FrameBuffer.Plane. Unless... we make a FrameBuffer namespace? Not sure. It's more work than it's worth for a simple struct like this though, imo. Speaking of which, this patch needs to be squashed into the previous patch, otherwise isolation will break in between that and this patch. Paul > > > > > > > return ret; > > > }
Hi Paul and Laurent, On Thu, Aug 19, 2021 at 3:28 PM <paul.elder@ideasonboard.com> wrote: > > Hi, > > On Wed, Aug 18, 2021 at 01:22:02PM +0300, Laurent Pinchart wrote: > > Hi, > > > > On Wed, Aug 18, 2021 at 11:58:29AM +0200, Jacopo Mondi wrote: > > > On Mon, Aug 16, 2021 at 01:31:30PM +0900, Hirokazu Honda wrote: > > > > The offset variable is added to FrameBuffer::Plane. This modifies > > > > the serialization and deserialization code for the offset variable. > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > --- > > > > src/libcamera/ipa_data_serializer.cpp | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp > > > > index fb941e6b..144c3248 100644 > > > > --- a/src/libcamera/ipa_data_serializer.cpp > > > > +++ b/src/libcamera/ipa_data_serializer.cpp > > > > @@ -562,6 +562,7 @@ FileDescriptor IPADataSerializer<FileDescriptor>::deserialize(const std::vector< > > > > * FrameBuffer::Plane is serialized as: > > > > * > > > > * 1 byte - FileDescriptor > > > > + * 4 bytes - uint32_t Offset > > > > * 4 bytes - uint32_t Length > > > > */ > > > > template<> > > > > @@ -580,6 +581,7 @@ IPADataSerializer<FrameBuffer::Plane>::serialize(const FrameBuffer::Plane &data, > > > > fdsVec.insert(fdsVec.end(), fdFds.begin(), fdFds.end()); > > > > > > > > appendPOD<uint32_t>(dataVec, data.length); > > > > + appendPOD<uint32_t>(dataVec, data.offset); > > > > > > > > return { dataVec, fdsVec }; > > > > } > > > > @@ -596,7 +598,8 @@ IPADataSerializer<FrameBuffer::Plane>::deserialize(std::vector<uint8_t>::const_i > > > > > > > > ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 1, > > > > fdsBegin, fdsBegin + 1); > > > > - ret.length = readPOD<uint32_t>(dataBegin, 1, dataEnd); > > > > + ret.offset = readPOD<uint32_t>(dataBegin, 1, dataBegin + 2); > > > > + ret.length = readPOD<uint32_t>(dataBegin + 1, 1, dataEnd); > > > > > > Am I wrong ot offset is appeneded at the end of dataVec (after lenght) and > > > then read from the beginning (before lenght) ? > > Yes, the serialize order needs to match the deserialize order. > > The definition is fd, offset, length, so I would serialize in that order > too (the deserializer has that order already). > > > > > How did I miss that ? :-S Let's serialize the fields in the order > > they're defined in the structure. Also, the dataBegin + 2 isn't correct, > > Okay, that's the fault of readPOD() being poorly (read: not having been) > documentated. readPOD() takes the begin iterator, the position, and the > end iterator. So it should be: > > - ret.length = readPOD<uint32_t>(dataBegin, 1, dataEnd); > + ret.offset = readPOD<uint32_t>(dataBegin, 1, dataEnd); > + ret.length = readPOD<uint32_t>(dataBegin, 5, dataEnd); > > (I'm confused about from where dataBegin + 2 came from in the first > place...) > > > deserializing an uint32_t requires 4 bytes, and dataBegin + 1 is thus > > incorrect too. > > > > Paul, is there an easy way to get the serdes code for > > FrameBuffer::Plane generated ? > > No, because mojo doesn't support defining FrameBuffer.Plane. Unless... > we make a FrameBuffer namespace? Not sure. It's more work than it's > worth for a simple struct like this though, imo. > > Speaking of which, this patch needs to be squashed into the previous > patch, otherwise isolation will break in between that and this patch. > Thanks for the correction. I misunderstood readPOD arguments. I couldn't find readPOD document. Where is it? Best Regards, -Hiro > > Paul > > > > > > > > > > > return ret; > > > > }
Hi Hiro, On Fri, Aug 20, 2021 at 03:54:15PM +0900, Hirokazu Honda wrote: > Hi Paul and Laurent, > > On Thu, Aug 19, 2021 at 3:28 PM <paul.elder@ideasonboard.com> wrote: > > > > Hi, > > > > On Wed, Aug 18, 2021 at 01:22:02PM +0300, Laurent Pinchart wrote: > > > Hi, > > > > > > On Wed, Aug 18, 2021 at 11:58:29AM +0200, Jacopo Mondi wrote: > > > > On Mon, Aug 16, 2021 at 01:31:30PM +0900, Hirokazu Honda wrote: > > > > > The offset variable is added to FrameBuffer::Plane. This modifies > > > > > the serialization and deserialization code for the offset variable. > > > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > > --- > > > > > src/libcamera/ipa_data_serializer.cpp | 5 ++++- > > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp > > > > > index fb941e6b..144c3248 100644 > > > > > --- a/src/libcamera/ipa_data_serializer.cpp > > > > > +++ b/src/libcamera/ipa_data_serializer.cpp > > > > > @@ -562,6 +562,7 @@ FileDescriptor IPADataSerializer<FileDescriptor>::deserialize(const std::vector< > > > > > * FrameBuffer::Plane is serialized as: > > > > > * > > > > > * 1 byte - FileDescriptor > > > > > + * 4 bytes - uint32_t Offset > > > > > * 4 bytes - uint32_t Length > > > > > */ > > > > > template<> > > > > > @@ -580,6 +581,7 @@ IPADataSerializer<FrameBuffer::Plane>::serialize(const FrameBuffer::Plane &data, > > > > > fdsVec.insert(fdsVec.end(), fdFds.begin(), fdFds.end()); > > > > > > > > > > appendPOD<uint32_t>(dataVec, data.length); > > > > > + appendPOD<uint32_t>(dataVec, data.offset); > > > > > > > > > > return { dataVec, fdsVec }; > > > > > } > > > > > @@ -596,7 +598,8 @@ IPADataSerializer<FrameBuffer::Plane>::deserialize(std::vector<uint8_t>::const_i > > > > > > > > > > ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 1, > > > > > fdsBegin, fdsBegin + 1); > > > > > - ret.length = readPOD<uint32_t>(dataBegin, 1, dataEnd); > > > > > + ret.offset = readPOD<uint32_t>(dataBegin, 1, dataBegin + 2); > > > > > + ret.length = readPOD<uint32_t>(dataBegin + 1, 1, dataEnd); > > > > > > > > Am I wrong ot offset is appeneded at the end of dataVec (after lenght) and > > > > then read from the beginning (before lenght) ? > > > > Yes, the serialize order needs to match the deserialize order. > > > > The definition is fd, offset, length, so I would serialize in that order > > too (the deserializer has that order already). > > > > > > > > How did I miss that ? :-S Let's serialize the fields in the order > > > they're defined in the structure. Also, the dataBegin + 2 isn't correct, > > > > Okay, that's the fault of readPOD() being poorly (read: not having been) > > documentated. readPOD() takes the begin iterator, the position, and the > > end iterator. So it should be: > > > > - ret.length = readPOD<uint32_t>(dataBegin, 1, dataEnd); > > + ret.offset = readPOD<uint32_t>(dataBegin, 1, dataEnd); > > + ret.length = readPOD<uint32_t>(dataBegin, 5, dataEnd); > > > > (I'm confused about from where dataBegin + 2 came from in the first > > place...) > > > > > deserializing an uint32_t requires 4 bytes, and dataBegin + 1 is thus > > > incorrect too. > > > > > > Paul, is there an easy way to get the serdes code for > > > FrameBuffer::Plane generated ? > > > > No, because mojo doesn't support defining FrameBuffer.Plane. Unless... > > we make a FrameBuffer namespace? Not sure. It's more work than it's > > worth for a simple struct like this though, imo. > > > > Speaking of which, this patch needs to be squashed into the previous > > patch, otherwise isolation will break in between that and this patch. > > > > Thanks for the correction. > I misunderstood readPOD arguments. > I couldn't find readPOD document. Where is it? That's what I said... there is no documentation for readPOD... I guess I'll have to add one... (I want to strengthen the ASSERT that's in there too) Paul > > > > > > > > > > > > > > > return ret; > > > > > }
diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp index fb941e6b..144c3248 100644 --- a/src/libcamera/ipa_data_serializer.cpp +++ b/src/libcamera/ipa_data_serializer.cpp @@ -562,6 +562,7 @@ FileDescriptor IPADataSerializer<FileDescriptor>::deserialize(const std::vector< * FrameBuffer::Plane is serialized as: * * 1 byte - FileDescriptor + * 4 bytes - uint32_t Offset * 4 bytes - uint32_t Length */ template<> @@ -580,6 +581,7 @@ IPADataSerializer<FrameBuffer::Plane>::serialize(const FrameBuffer::Plane &data, fdsVec.insert(fdsVec.end(), fdFds.begin(), fdFds.end()); appendPOD<uint32_t>(dataVec, data.length); + appendPOD<uint32_t>(dataVec, data.offset); return { dataVec, fdsVec }; } @@ -596,7 +598,8 @@ IPADataSerializer<FrameBuffer::Plane>::deserialize(std::vector<uint8_t>::const_i ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 1, fdsBegin, fdsBegin + 1); - ret.length = readPOD<uint32_t>(dataBegin, 1, dataEnd); + ret.offset = readPOD<uint32_t>(dataBegin, 1, dataBegin + 2); + ret.length = readPOD<uint32_t>(dataBegin + 1, 1, dataEnd); return ret; }
The offset variable is added to FrameBuffer::Plane. This modifies the serialization and deserialization code for the offset variable. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- src/libcamera/ipa_data_serializer.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)