Re: [PATCH v3 09/33] v4l: Add subdev selections documentation

Laurent Pinchart
2012-02-28T11:42:21Z

Hi Sakari,

On Sunday 26 February 2012 23:42:19 Sakari Ailus wrote:
> Laurent Pinchart wrote:
> > On Monday 20 February 2012 03:56:48 Sakari Ailus wrote:
> >> Add documentation for V4L2 subdev selection API. This changes also
> >> experimental V4L2 subdev API so that scaling now works through selection
> >> API only.
> >>
> >> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> >>
> >>
> >> diff --git a/Documentation/DocBook/media/v4l/dev-subdev.xml
> >> b/Documentation/DocBook/media/v4l/dev-subdev.xml index 0916a73..9d5e7da
> >> 100644
> >> --- a/Documentation/DocBook/media/v4l/dev-subdev.xml
> >> +++ b/Documentation/DocBook/media/v4l/dev-subdev.xml
> >
> > [snip]
> >
> >> + Scaling operation changes the size of the image by scaling
> >> + it to new dimensions. Some sub-devices support it. The scaled
> >> + size (width and height) is represented by &v4l2-rect;. In the
> >> + case of scaling, top and left will always be zero. Scaling is
> >> + configured using &sub-subdev-g-selection; and
> >> + <constant>V4L2_SUBDEV_SEL_COMPOSE_ACTIVE</constant> selection
> >> + target on the sink pad of the subdev. The scaling is performed
> >> + related to the width and height of the crop rectangle on the
> >> + subdev's sink pad.
> >
> > I'm not sure if that would be very clear for readers who are not yet
> > familiar with the API. What about the following text instead ?
> >
> > "The scaling operation changes the size of the image by scaling it to new
> > dimensions. The scaling ratio isn't specified explicitly, but is implied
> > from the original and scaled image sizes. Both sizes are represented by
> > &v4l2- rect;.
> >
> > Scaling support is optional. When supported by a subdev, the crop
> > rectangle on the subdev's sink pad is scaled to the size configured using
> > &VIDIOC-SUBDEV-G- SELECTION; and
> > <constant>V4L2_SUBDEV_SEL_COMPOSE_ACTIVE</constant> selection target on
> > the same pad. If the subdev supports scaling but no composing, the top
> > and left values are not used and must always be set to zero."
> >
> > (note that &sub-subdev-g-selection; has been replaced with
> > &VIDIOC-SUBDEV-G- SELECTION;)
> >
> > I would also move this text after the sink pad crop description to follow
> > the order in which operations are applied by subdevs.
>
> I'm fine with that change, so I did it. However, I won't replace
> &sub-subdev-g-selection; with &VIDIOC-SUBDEV-G-SELECTION; simply because
> it won't work:
>
> dev-subdev.xml:310: parser error : Entity 'VIDIOC-SUBDEV-G-SELECTION'
> not defined
> size configured using &VIDIOC-SUBDEV-G-SELECTION; and
>
> It's beyond me why not; similar references are being used elsewhere with
> otherwise equivalent definitions. Perhaps the name is just too long?
> That's the only difference I could think of: xmlto typically segfaults
> on errors so I wouldn't be surprised of something so simple.

Don't give up so fast :-)

diff --git a/Documentation/DocBook/media/Makefile b/Documentation/DocBook/media/Makefile
index 729f840..77ead85 100644
--- a/Documentation/DocBook/media/Makefile
+++ b/Documentation/DocBook/media/Makefile
@@ -65,6 +65,8 @@ IOCTLS = \
$(shell perl -ne 'print "$$1 " if /\#define\s+([^\s]+)\s+_IO/' $(srctree)/include/linux/dvb/video.h) \
$(shell perl -ne 'print "$$1 " if /\#define\s+([^\s]+)\s+_IO/' $(srctree)/include/linux/media.h) \
$(shell perl -ne 'print "$$1 " if /\#define\s+([^\s]+)\s+_IO/' $(srctree)/include/linux/v4l2-subdev.h) \
+ VIDIOC_SUBDEV_G_SELECTION \
+ VIDIOC_SUBDEV_S_SELECTION \
VIDIOC_SUBDEV_G_FRAME_INTERVAL \
VIDIOC_SUBDEV_S_FRAME_INTERVAL \
VIDIOC_SUBDEV_ENUM_MBUS_CODE \

and rm Documentation/DocBook/media-entities.tmpl before you compile the
documentation.

> >> + As for pad formats, drivers store try and active
> >> + rectangles for the selection targets of ACTIVE type <xref
> >> + linkend="v4l2-subdev-selection-targets">.
> >> +
> >> + On sink pads, cropping is applied relatively to the
> >> + current pad format. The pad format represents the image size as
> >> + received by the sub-device from the previous block in the
> >> + pipeline, and the crop rectangle represents the sub-image that
> >> + will be transmitted further inside the sub-device for
> >> + processing.
> >> +
> >> + On source pads, cropping is similar to sink pads, with the
> >> + exception that the source size from which the cropping is
> >> + performed, is the COMPOSE rectangle on the sink pad. In both
> >> + sink and source pads, the crop rectangle must be entirely
> >> + containted inside the source image size for the crop
> >> + operation.
> >> +
> >> + The drivers should always use the closest possible
> >> + rectangle the user requests on all selection targets, unless
> >> + specificly told otherwise<xref
> >> + linkend="v4l2-subdev-selection-flags">.
> >> +
> >> +
> >> +
> >> + Types of selection targets
> >> +
> >> +
> >> + ACTIVE targets
> >> +
> >> + ACTIVE targets reflect the actual hardware configuration
> >> + at any point of time.
> >> +
> >> +
> >> +
> >> + BOUNDS targets
> >> +
> >> + BOUNDS targets is the smallest rectangle within which
> >> + contains all valid ACTIVE rectangles.
> >
> > s/within which/that/ ?
>
> Ack.
>
> >> It may not be possible
> >> + to set the ACTIVE rectangle as large as the BOUNDS rectangle,
> >> + however.
> >
> > What about
> >
> > "The BOUNDS rectangle might not itself be a valid ACTIVE rectangle when
> > all possible ACTIVE pixels do not form a rectangular shape (e.g.
> > cross-shaped or round sensors)."
>
> There are cases where the active size is limited, even if it's
> rectangular. I can add the above case there, sure, if you think such
> devices exist --- I've never heard of nor seen them. Some sensors are
> documented to be cross-shaped but the only thing separating these from
> the rest is that the manufacturer doesn't guarantee the quality of the
> pixels in the corners. At least on those I've seen. You can still
> capture the full pixel matrix.

Those are just examples. I think it's useful to add them, otherwise the reader
won't know why and under what kind of circumstances the ACTIVE rectangle can't
be as large at the BOUNDS rectangle.

> >> +
> >>
> >> - Cropping behaviour on output pads is not defined.
> >> +
> >> +
> >> +
> >> + Order of configuration and format propagation
> >> +
> >> + Inside subdevs, the order of image processing steps will
> >> + always be from the sink pad towards the source pad. This is also
> >> + reflected in the order in which the configuration must be
> >> + performed by the user: the changes made will be propagated to
> >> + any subsequent stages. If this behaviour is not desired, the
> >> + user must set
> >> + <constant>V4L2_SUBDEV_SEL_FLAG_KEEP_CONFIG</constant> flag.
> >
> > Could you explain what happens when V4L2_SUBDEV_SEL_FLAG_KEEP_CONFIG is
> > set ? Just stating that it doesn't follow the propagation behaviour
> > previously described could be understood in many different ways.
>
> Good point. How about this:
>
> "This flag causes that no propagation of the changes are allowed in any
> circumstances. This may also lead the accessed rectangle not being
> changed at all,

"The accessed rectangle will likely be adjusted by the driver,"

> depending on the properties of the underlying hardware.
> Some drivers may not support this flag."

What should happen then ? Should the flag be ignored, or should the driver
return an error ?

> >> The
> >> + coordinates to a step always refer to the active size of the
> >> + previous step. The exception to this rule is the source compose
> >> + rectangle, which refers to the sink compose bounds rectangle ---
> >> + if it is supported by the hardware.
> >> +
> >> +
> >> + Sink pad format. The user configures the sink pad
> >> + format. This format defines the parameters of the image the
> >> + entity receives through the pad for further processing.
> >> +
> >> + Sink pad active crop selection. The sink pad crop
> >> + defines the performed to the sink pad format.
> >
> > s/defines the/defines the cropping/ ?
>
> Fixed.
>
> >> +
> >> + Sink pad active compose selection. The size of the
> >> + sink pad compose rectangle defines the scaling ratio compared
> >> + to the size of the sink pad crop rectangle. The location of
> >> + the compose rectangle specifies the location of the active
> >> + sink compose rectangle in the sink compose bounds
> >> + rectangle.
> >> +
> >> + Source pad active crop selection. Crop on the source
> >> + pad defines crop performed to the image in the sink compose
> >> + bounds rectangle.
> >> +
> >> + Source pad format. The source pad format defines the
> >> + output pixel format of the subdev, as well as the other
> >> + parameters with the exception of the image width and height.
> >> + Width and height are defined by the size of the source pad
> >> + active crop selection.
> >> +
> >> +
> >> + Accessing any of the above rectangles not supported by the
> >> + subdev will return <constant>EINVAL</constant>.
> >
> > Do drivers have to support BOUNDS rectangles for every ACTIVE rectangle
> > they support (and the other way around) ? If so, I think it should be
> > specified.
> I think the answer to that should be "yes", albeit this hasn't been
> discussed previously. I see no reason not to support equivalent BOUNDS
> rectangles.
>
> I'll change the documentation to state that.
>
> > Is EINVAL returned for any other error case in the selection API ? If so,
> > it might make enumeration of the supported rectangles difficult.
>
> Good point. There are two other reasons to return EINVAL and those are
> invalid parameters in either pad or which fields. I think that should be
> fine. I'd keep it EINVAL.
>
> Alternatively we could use e.g. ENOENT.
>
> What do you think?

I'm OK with EINVAL then, as applications shouldn't pass an invalid pad or
which value in the first place.

> >> Any rectangle
> >> + referring to a previous unsupported rectangle coordinates will
> >> + instead refer to the previous supported rectangle. For example,
> >> + if sink crop is not supported, the compose selection will refer
> >> + to the sink pad format dimensions instead.
> >
> > Should we add a list of the rectangles a subdev must/should/can support
> > for the different possible use cases ?
>
> I think this should be rather obvious with the examples and the
> statement on BOUNDS rectangles. Such a list, if we make it very
> detailed, will be unnecessarily redundant and will soon become
> out-of-date. In principle I don't like redundancy whether it's code or
> documentation. It easily leads to contradictions.
>
> If it turns out it'll be needed we can easily add it later on.

OK.

[snip]

> >> diff --git
> >> a/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml
> >> b/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml new file
> >> mode 100644
> >> index 0000000..033077a
> >> --- /dev/null
> >> +++ b/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml

[snip]

> >> +
> >> + V4L2 subdev selection flags
> >> + <tgroup cols="3">
> >> + &cs-def;
> >> +
> >> + <row>
> >> + <entry><constant>V4L2_SUBDEV_SEL_FLAG_SIZE_GE</constant></entry>
> >> + <entry>(1 &lt;&lt; 0)</entry>
> >> + <entry>Suggest the driver it should choose greater or
> >> + equal rectangle (in size) than was requested.</entry>
> >> + </row>
> >> + <row>
> >> + <entry><constant>V4L2_SUBDEV_SEL_FLAG_SIZE_LE</constant></entry>
> >> + <entry>(1 &lt;&lt; 1)</entry>
> >> + <entry>Suggest the driver it should choose lesser or
> >> + equal rectangle (in size) than was requested.</entry>
> >> + </row>
> >
> > Those two flags are only briefly described here, could you add a more
> > detailed description either here or in
> > Documentation/DocBook/media/v4l/dev-subdev.xml ?
> Added:
>
> " Albeit the driver may choose a lesser size,
> it will only do so due to hardware limitations. Without
> this flag (and
> <constant>V4L2_SUBDEV_SEL_FLAG_SIZE_LE</constant>) the
> behaviour is to choose the closest possible
> rectangle."
>
> Does that cover your understanding of a more detailed description? :-)

I was thinking about something in media/v4l/dev-subdev.xml that would explain
what the flags are used for. media/v4l/vidioc-subdev-g-selection.xml is more
like reference documentation.

--
Regards,

Laurent Pinchart