[libcamera-devel] [PATCH v4 25/37] ipa: raspberrypi: Add mojom data definition file

[Laurent Pinchart]

Hi Paul,

On Sat, Dec 05, 2020 at 06:20:10PM +0900, paul.elder at ideasonboard.com wrote:
> On Thu, Nov 26, 2020 at 05:02:55PM +0200, Laurent Pinchart wrote:
> > On Fri, Nov 06, 2020 at 07:36:55PM +0900, Paul Elder wrote:
> > > Add a mojom data definition for raspberrypi pipeline handler's IPAs.
> > > This is a direct translation of what the raspberrypi pipeline handler
> > > and IPA was using before with IPAOperationData.
> > 
> > s/was/were/
> > 
> > > 
> > > Also move the enums from raspberrypi.h to raspberrypi.mojom
> > > 
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > 
> > > ---
> > > Changes in v4:
> > > - rename IPARPiCallbackInterface to IPARPiEventInterface
> > > 
> > > Changes in v3:
> > > - remove stray comment about how our compiler will generate code
> > > - add ipa.rpi namespace
> > >   - not ipa.RPi, because namespace conflict
> > > - remove RPi prefix from struct and enum names
> > > - add transform parameter to struct ConfigInput
> > > 
> > > Changes in v2:
> > > - rebased on "libcamera: pipeline: ipa: raspberrypi: Rework drop frame
> > >   signalling"
> > > - add license
> > > - move generic documentation to core.mojom
> > > - move documentation from IPA interface
> > > - customize the RPi IPA interface to make the pipeline and IPA code
> > >   cleaner
> > > ---
> > >  include/libcamera/ipa/meson.build       |   4 +-
> > >  include/libcamera/ipa/raspberrypi.h     |  18 ---
> > >  include/libcamera/ipa/raspberrypi.mojom | 158 ++++++++++++++++++++++++
> > >  3 files changed, 161 insertions(+), 19 deletions(-)
> > >  create mode 100644 include/libcamera/ipa/raspberrypi.mojom

[snip]

> > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > > new file mode 100644
> > > index 00000000..ec261569
> > > --- /dev/null
> > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > > @@ -0,0 +1,158 @@

[snip]

> > > +struct ConfigOutput {
> > > +	uint32 op;
> > 
> > Here too I'd like to drop the op field if possible.
> > RPI_IPA_CONFIG_DROP_FRAMES can be dropped as it's always set. For
> > RPI_IPA_CONFIG_SENSOR, we can check if controls is empty.
> > RPI_IPA_CONFIG_STAGGERED_WRITE is the annoying one, as ideally it should
> > be implemented through support for nullable fields. Mojo supports this
> > by turning fields of struct type to pointers, but we don't, as we embed
> > them instead. I think it would make sense doing the same, but not yet,
> > as that would require too many changes to this series. In the meantime,
> > one simple option would be to ignore staggeredWriteResult in the
> > pipelien handler when !gainDelay && !exposureDelay, as that's an invalid
> > case.
> 
> I'll change around the ops and the ops processing in v6.
> 
> > > +	StaggeredWritePayload staggeredWriteResult;
> > 
> > staggeredWriteResult sounds a bit cryptic. If we rename the structure to
> > SensorConfig, sensorConfig would be a better field name.
> 
> Yeah.
> 
> > > +	ControlList controls;
> > 
> > Should this be named sensorControls ?
> 
> I thought controls was just fine...

"controls" isn't bad :-) I thought it could be a bit more descriptive,
reading the code would likely leave one wonder what controls these are.
We of course need to find a proper balance to avoid descriptive names
that would be 100 characters long :-) I trust you to decide what's best
in this case.

> > > +	int32 dropFrameCount;
> > > +};
> > 
> > Even if the mojo IDL doesn't require this, should the enums and struct
> > be moved before the interfaces ? I think it would improve readability.
> 
> Yeah.

-- 
Regards,

Laurent Pinchart