[libcamera-devel] [RFC PATCH 02/10] libcamera: ipa_data_serializer: Modify (de)serialization for offset

[paul.elder at ideasonboard.com]

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 at 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 at 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;
> > > > >  }