On Fri, 2019-05-10 at 10:28 +0200, Hans Verkuil wrote:
[...]
> > +static int allegro_g_selection(struct file *file, void *priv,
> > + struct v4l2_selection *s)
> > +{
> > + struct v4l2_fh *fh = file->private_data;
> > + struct allegro_channel *channel = fh_to_channel(fh);
> > +
> > + if (!V4L2_TYPE_IS_OUTPUT(s->type))
> > + return -EINVAL;
> > +
> > + switch (s->target) {
> > + case V4L2_SEL_TGT_CROP:
> > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > + s->r.left = 0;
> > + s->r.top = 0;
> > + s->r.width = channel->width;
> > + s->r.height = channel->height;
>
> I don't think this is quite right. The CROP target should return the visible
> width/height (e.g. 1920x1080) whereas the other two targets should return the
> coded width/height (e.g. 1920x1088 when rounded to the macroblock alignment).
The stateful encoder spec says about CROP_DEFAULT and CROP_BOUNDS:
V4L2_SEL_TGT_CROP_BOUNDS
equal to the full source frame, matching the active OUTPUT
format
V4L2_SEL_TGT_CROP_DEFAULT
equal to V4L2_SEL_TGT_CROP_BOUNDS
There is no mention that the "full source frame" must be equal to the
coded size. Enforcing this would unnecesarily limit the ability to
import DMA-Buffers from other sources.
Imagine for example 1080p capture and encoding - if the source provides
unpadded 1920x1080 buffers (with the required stride), importing them
into an encoder that enforces height to be a multiple ot 16 wouldn't
work, even though the hardware may be capable of only reading 1080 lines
from the source frame.
> Note: if the hardware doesn't require that the raw frame is macroblock aligned,
> then I need to think a bit more about how the selection handling should be
> done.
regards
Philipp
