Hi Laurent,
Thanks for your review.
On 2016-09-05 16:05:10 +0300, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Friday 02 Sep 2016 15:47:13 Niklas Söderlund wrote:
> > The format is used on the R-Car VSP1 video queues that carry
> > 2-D histogram statistics data.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > ---
> > Documentation/media/uapi/v4l/meta-formats.rst | 1 +
> > .../media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst | 150 ++++++++++++++++++
> > drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> > include/uapi/linux/videodev2.h | 3 +-
> > 4 files changed, 154 insertions(+), 1 deletion(-)
> > create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst
>
> [snip]
>
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst
> > b/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst new file mode
> > 100644
> > index 0000000..a093f0a
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst
> > @@ -0,0 +1,150 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +
> > +.. _v4l2-meta-fmt-vsp1-hgt:
> > +
> > +*******************************
> > +V4L2_META_FMT_VSP1_HGT ('VSPT')
> > +*******************************
> > +
> > +*man V4L2_META_FMT_VSP1_HGT(2)*
> > +
> > +Renesas R-Car VSP1 2-D Histogram Data
> > +
> > +
> > +Description
> > +===========
> > +
> > +This format describes histogram data generated by the Renesas R-Car VSP1
> > +2-D Histogram (HGT) engine.
> > +
> > +The VSP1 HGT is a histogram computation engine that operates on HSV
> > +data. It operates on a possibly cropped and subsampled input image and
> > +computes the sum, maximum and minimum of the S component as well as a
> > +weighted frequency histogram based on the H and S components.
> > +
> > +The histogram is a matrix of 6 Hue and 32 Saturation buckets, 192 in
> > +total. Each HSV value is added to one or more buckets with a wight
>
> s/wight/weight/
Will fix.
>
> > +between 1 and 16 depending on how the Hue areas are setup.
>
> I would say 'depending on the Hue areas configuration' to insist on the fact
> that the configuration can be changed by the application.
Will fix.
>
> > Finding the
> > +correct buckets is done by inspecting the H and S value independently.
>
> Maybe s/correct/corresponding/ ?
Will fix.
>
> > +The Saturation position **n** (0 - 31) in the matrix are found by the
>
> Maybe s/in the matrix/of the bucket/, or s/in the matrix/of the bucket in the
> matrix/ ?
> s/are found/is found/ or maybe 'is computed' ?
Will fix.
>
> > +expression:
> > +
> > + 8 * n <= S < 8 * (n + 1)
>
> How about simply 'n = S / 8' ?
Will fix.
>
> > +The Hue positions **m** (0 - 5) in the matrix depends on how the HGT Hue
>
> s/positions/position/
Will fix.
>
> > +areas are configured. There are 6 user configurable Hue Areas which can
> > +be configured to cover overlapping Hue values:
> > +
> > +::
> > +
> > + Area 0 Area 1 Area 2 Area 3 Area 4
> > Area 5 + ________ ________ ________ ________
> > ________ ________ + \ /| |\ /| |\ /| |\ /|
> > |\ /| |\ /| |\ / + \ / | | \ / | | \ / |
> > | \ / | | \ / | | \ / | | \ / + X | | X | |
> > X | | X | | X | | X | | X + / \ | | /
> > \ | | / \ | | / \ | | / \ | | / \ | | / \ + /
> > \| |/ \| |/ \| |/ \| |/ \| |/ \| |/
> > \ + 5U 0L 0U 1L 1U 2L 2U 3L 3U 4L 4U
> > 5L 5U 0L + <0..............................Hue
> > Value............................255>
> > +
> > +As shown in the diagram a single Hue vale can be attributed to more then
>
> s/vale/value/
> s/then/than/
Will fix.
>
> > +one Hue area. In such case the Hue value is attributed to both Hue
> > +Areas, but with a weight. The maximum weight is 16 and is associated
> > +with all Hue values that are inside the center of a Hue area (between nL
> > +-- nU). Values outside this area are weighted with a rounded down value
> > +along the diagonal line. If there is no overlapped areas specified the
> > +value is included in the lower area.
>
> This sounds a bit confusing to me. How about the following ?
>
> "The Hue position **m** (0 - 5) of the bucket [in the matrix]* depends on how
> the HGT Hue areas are configured. There are 6 user configurable Hue Areas
> which can be configured to cover overlapping Hue values:
>
> [diagram]
>
> When two consecutive areas don't overlap (n+1L is equal to nU) the boundary
> value is considered as part of the lower area.
>
> Pixels with a hue value included in the centre of an area (between nL and nU
> included) are are attributed to that single area and given a weight of 16.
> Pixels with a hue value included in the overlapping region between two areas
> (between n+1L and nU excluded) are attributed to both areas and given a weight
> for each of these areas proportional to their position along the diagonal
> lines (rounded down)."
>
> * Add "in the matrix" depending on the wording of the saturation description.
>
Will fix.
> > +The Hue area setup must match one of the following constrains:
> > +
> > +::
> > +
> > + 0L <= 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U
> > +
> > +::
> > +
> > + 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U <= 0L
> > +
> > +**Byte Order.**
> > +All data is stored in memory in little endian format. Each cell in the
> > tables +contains one byte.
> > +
> > +.. flat-table:: VSP1 HGT Data - (800 bytes)
> > + :header-rows: 2
> > + :stub-columns: 0
> > +
> > + * - Offset
> > + - :cspan:`4` Memory
> > + * -
> > + - [31:24]
> > + - [23:16]
> > + - [15:8]
> > + - [7:0]
> > + * - 0
> > + - -
> > + - S max [7:0]
> > + - -
> > + - S min [7:0]
> > + * - 4
> > + - :cspan:`4` S sum [31:0]
> > + * - 8
> > + - -
> > + - Hue Area 0 Lower Boundary (0L) [0:7]
> > + - -
> > + - Hue Area 0 Upper Boundary (0U) [0:7]
> > + * - 12
> > + - -
> > + - Hue Area 1 Lower Boundary (1L) [0:7]
> > + - -
> > + - Hue Area 1 Upper Boundary (1U) [0:7]
> > + * - 16
> > + - -
> > + - Hue Area 2 Lower Boundary (2L) [0:7]
> > + - -
> > + - Hue Area 2 Upper Boundary (2U) [0:7]
> > + * - 20
> > + - -
> > + - Hue Area 3 Lower Boundary (3L) [0:7]
> > + - -
> > + - Hue Area 3 Upper Boundary (3U) [0:7]
> > + * - 24
> > + - -
> > + - Hue Area 4 Lower Boundary (4L) [0:7]
> > + - -
> > + - Hue Area 4 Upper Boundary (4U) [0:7]
> > + * - 28
> > + - -
> > + - Hue Area 5 Lower Boundary (5L) [0:7]
> > + - -
> > + - Hue Area 5 Upper Boundary (5U) [0:7]
>
> What's the rationale for including the boundaries in the statistics buffer ?
> Boundaries are configured by userspace, they should be known to the
> application already.
At the time I thought it would easy userspaces consumption of the data,
for example if the histograms where recorded for later processing. Other
then that I have no good rationale for including them. I be happy to
drop them in v2 if you see no value in them.
>
> > + * - 32
> > + - :cspan:`4` Histogram bucket (m=0, n=0) [31:0]
> > + * - 36
> > + - :cspan:`4` Histogram bucket (m=0, n=1) [31:0]
> > + * -
> > + - :cspan:`4` ...
> > + * - 156
> > + - :cspan:`4` Histogram bucket (m=0, n=31) [31:0]
> > + * - 160
> > + - :cspan:`4` Histogram bucket (m=1, n=0) [31:0]
> > + * -
> > + - :cspan:`4` ...
> > + * - 288
> > + - :cspan:`4` Histogram bucket (m=2, n=0) [31:0]
> > + * -
> > + - :cspan:`4` ...
> > + * - 416
> > + - :cspan:`4` Histogram bucket (m=3, n=0) [31:0]
> > + * -
> > + - :cspan:`4` ...
> > + * - 544
> > + - :cspan:`4` Histogram bucket (m=4, n=0) [31:0]
> > + * -
> > + - :cspan:`4` ...
> > + * - 672
> > + - :cspan:`4` Histogram bucket (m=5, n=0) [31:0]
> > + * -
> > + - :cspan:`4` ...
> > + * - 796
> > + - :cspan:`4` Histogram bucket (m=5, n=31) [31:0]
>
> [snip]
>
> --
> Regards,
>
> Laurent Pinchart
>
--
Regards,
Niklas Söderlund
