[RFC] Cropping and scaling with subdev pad-level operations

[RFC] Cropping and scaling with subdev pad-level operations

[Laurent Pinchart]

Hi everybody,

I ran into an issue when implementing cropping and scaling on the OMAP3 ISP 
resizer sub-device using the pad-level operations. As nobody seems to be happy 
with the V4L2 crop ioctls, I thought I would ask for comments about the subdev 
pad-level API to avoid repeating the same mistakes.

A little background information first. The OMAP3 ISP resizer has an input and 
an output pad. It receives images on its input pad, performs cropping to a 
user-configurable rectange and then scales that rectangle up or down to a 
user-configurable output size. The resulting image is output on the output 
pad.

The hardware sets various restrictions on the crop rectangle and on the output 
size, or more precisely on the relationship between them. Horizontal and 
vertical scaling ratios are independent (at least independent enough for the 
purpose of this discussion), so I'll will not comment on one of them in 
particular.

The minimum and maximum scaling ratios are roughly 1/4 and x4. A complex 
equation describes the relationship between the ratio, the cropped size and 
the output size. It involves integer arithmetics and must be fullfilled 
exactly, so not all combination of crop rectangle and output size can be 
achieved.

The driver expects the user to set the input size first. It propagates the 
input format to the output pad, resetting the crop rectangle. That behaviour 
is common to all the OMAP3 ISP modules, and I don't think much discussion is 
needed there.

The user can then configure the crop rectangle and the output size 
independently. As not all combinations are possible, configuring one of them 
can modify the other one as a side effect. This is where problems come from.

Let's assume that the input size, the crop rectangle and the output size are 
all set to 4000x4000. The user then wants to crop a 500x500 rectangle and 
scale it up to 750x750.

If the user first sets the crop rectangle to 500x500,  the 4000x4000 output 
size would result in a x8 scaling factor, not supported by the resizer. The 
driver must then either modify the requested crop rectangle or the output size 
to fullfill the hardware requirements.

If the user first sets the output size to 750x750 we end up with a similar 
problem, and the driver needs to modify one of crop rectangle or output size 
as well.

When the stream is on, the output size can't be modified as it would change 
the captured frame size. The crop rectangle and scaling ratios, on the other 
hand, can be modified to implement digital zoom. For that reason, the resizer 
driver doesn't modify the output size when the crop rectangle is set while a 
stream is running, but restricts the crop rectangle size. With the above 
example as a starting point, requesting a 500x500 crop rectangle, which would 
result in an unsupported x8 zoom, will return a 1000x1000 crop rectangle.

When the stream is off, we have two options:

- Handle crop rectangle modifications the same way as when the stream is on. 
This is cleaner, but bring one drawback. The user can't set the crop rectangle 
to 500x500 and output size to 750x750 directly. No matter whether the crop 
rectangle or output size is set first, the intermediate 500x5000/4000x4000 or 
4000x4000/750x750 combination are invalid. An extra step will be needed: the 
crop rectangle will first be set to 1000x1000, the output size will then be 
set to 750x750, and the crop rectangle will finally be set to 500x500. That 
won't make life easy for userspace applications.

- Modify the output size when the crop rectangle is set. With this option, the 
output size is automatically set to the crop rectangle size when the crop 
rectangle is changed. With the above example, setting the crop rectangle to 
500x500 will automatically set the output size to 500x500, and the user will 
then just have to set the output size to 750x750.

The second option has a major drawback as well, as there's no way for 
applications to query the minimum/maximum zoom factor. With the first option 
an application can set the desired output size, and then set a very small crop 
rectangle to retrieve the minimum allowed crop rectangle (and thus the maximum 
zoom factor). With the second option the output size will be changed when the 
crop rectangle is set, so this isn't possible anymore.

Retrieving the maximum zoom factor in the stream off state is an application 
requirement to be able to display the zoom level on a GUI (with a slider for 
instance).

The OMAP3 ISP resizer currently implements the second option, and I'll modify 
it to implement the first option. The drawback is that some crop/output 
combinations will require an extra step to be achieved. I'd like your opinion 
on this issue. Is the behaviour described in option one acceptable ? Should 
the API be extended/modified to make it simpler for applications to configure 
the various sizes in the image pipeline ? Are we all doomed and will we have 
to use a crop/scale API that nobody will ever understand ? :-)

-- 
Regards,

Laurent Pinchart

Re: [RFC] Cropping and scaling with subdev pad-level operations

[Andy Walls]

On Thu, 2011-01-06 at 16:33 +0100, Laurent Pinchart wrote:
> Hi everybody,
> 
> I ran into an issue when implementing cropping and scaling on the OMAP3 ISP 
> resizer sub-device using the pad-level operations. As nobody seems to be happy 
> with the V4L2 crop ioctls, I thought I would ask for comments about the subdev 
> pad-level API to avoid repeating the same mistakes.
> 
> A little background information first. The OMAP3 ISP resizer has an input and 
> an output pad. It receives images on its input pad, performs cropping to a 
> user-configurable rectange and then scales that rectangle up or down to a 
> user-configurable output size. The resulting image is output on the output 
> pad.
> 
> The hardware sets various restrictions on the crop rectangle and on the output 
> size, or more precisely on the relationship between them. Horizontal and 
> vertical scaling ratios are independent (at least independent enough for the 
> purpose of this discussion), so I'll will not comment on one of them in 
> particular.
> 
> The minimum and maximum scaling ratios are roughly 1/4 and x4. A complex 
> equation describes the relationship between the ratio, the cropped size and 
> the output size. It involves integer arithmetics and must be fullfilled 
> exactly, so not all combination of crop rectangle and output size can be 
> achieved.
> 
> The driver expects the user to set the input size first. It propagates the 
> input format to the output pad, resetting the crop rectangle. That behaviour 
> is common to all the OMAP3 ISP modules, and I don't think much discussion is 
> needed there.

I'll note here that the driver is allowing the application to make *two*
assumptions here:

	output size == input size
and
	output pixel resolution == input pixel resolution

If I'm taking a picture of a building, at a distance from the building
such that the input image has a resolution of 1 pixel per 5 cm in the
plane of the building, then the output image also has a pixel resolution
of 1 pixel per 5 cm in the plane of the building. 


> The user can then configure the crop rectangle and the output size 
> independently. As not all combinations are possible, configuring one of them 
> can modify the other one as a side effect.

What enforces the modification, the hardware or the driver?

IMO, a crop should be a crop with no scaling or interpolation side
effects.  If I have 1 pixel per 5 cm on the input, I should get 1 pixel
per 5 cm on the output.

Changing the output size when setting a new crop window is IMO the
correct thing to do since:

a. it maintains the pixel resolution (e.g. 1 pixel per 5 cm).

b. it is a predictable result that people and applications can rely upon

c. It fail due to any implicit zoom constraint violation

d. The application also knows what the new output size will be, since it
just set the crop window to that same size


>  This is where problems come from.

> Let's assume that the input size, the crop rectangle and the output size are 
> all set to 4000x4000. The user then wants to crop a 500x500 rectangle and 
> scale it up to 750x750.
> 
> If the user first sets the crop rectangle to 500x500,  the 4000x4000 output 
> size would result in a x8 scaling factor, not supported by the resizer. The 
> driver must then either modify the requested crop rectangle or the output size 
> to fullfill the hardware requirements.

My suggestion is to have the driver modify the output size as a side
effect.  Changing the output size already happens as a side effect when
the application sets the input size.


> If the user first sets the output size to 750x750 we end up with a similar 
> problem, and the driver needs to modify one of crop rectangle or output size 
> as well.
> 
> When the stream is on, the output size can't be modified as it would change 
> the captured frame size.

Hmm.  

>  The crop rectangle and scaling ratios, on the other 
> hand, can be modified to implement digital zoom. For that reason, the resizer 
> driver doesn't modify the output size when the crop rectangle is set while a 
> stream is running, but restricts the crop rectangle size. With the above 
> example as a starting point, requesting a 500x500 crop rectangle, which would 
> result in an unsupported x8 zoom, will return a 1000x1000 crop rectangle.

But in this situation - fixed input size, fixed output size - you know
exactly what scaling levels are supported, 1/4x to 4x, right?

It sounds like this can be hidden from userpace with different behavior
inside the driver for the crop and scale API calls when streaming.

1.  When streaming, a single crop or scale API request from userspace
would cause the driver to command all the required underlying crop and
scale operations for the resizer.

2. When streaming stops, either 
a. require the applications to query the state of the crop window and
output size, or
b. require drivers to revert to the original crop and scale settings
that were in place before streaming started

Bad idea?


> When the stream is off, we have two options:
> 
> - Handle crop rectangle modifications the same way as when the stream is on. 
> This is cleaner, but bring one drawback. The user can't set the crop rectangle 
> to 500x500 and output size to 750x750 directly. No matter whether the crop 
> rectangle or output size is set first, the intermediate 500x5000/4000x4000 or 
> 4000x4000/750x750 combination are invalid. An extra step will be needed: the 
> crop rectangle will first be set to 1000x1000, the output size will then be 
> set to 750x750, and the crop rectangle will finally be set to 500x500. That 
> won't make life easy for userspace applications.
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
There's a reason why you shouldn't choose this option.

Are the constraints between crop and output size going to be the same
for every resizer hardware variant or every driver?



> - Modify the output size when the crop rectangle is set. With this option, the 
> output size is automatically set to the crop rectangle size when the crop 
> rectangle is changed. With the above example, setting the crop rectangle to 
> 500x500 will automatically set the output size to 500x500, and the user will 
> then just have to set the output size to 750x750.

I like this one.  The result of the side effect is easily understood and
should be easy to implement for different hardware using the same API.


> The second option has a major drawback as well, as there's no way for 
> applications to query the minimum/maximum zoom factor. With the first option 
> an application can set the desired output size, and then set a very small crop 
> rectangle to retrieve the minimum allowed crop rectangle (and thus the maximum 
> zoom factor). With the second option the output size will be changed when the 
> crop rectangle is set, so this isn't possible anymore.
> 
> Retrieving the maximum zoom factor in the stream off state is an application 
> requirement to be able to display the zoom level on a GUI (with a slider for 
> instance).

The application trying to make indirect behavior measurements to
determine maximum/minimum zoom seems odd.  Why can't the driver just
report those directly via some control or query?  The driver should know
the answer.


> The OMAP3 ISP resizer currently implements the second option, and I'll modify 
> it to implement the first option. The drawback is that some crop/output 
> combinations will require an extra step to be achieved. I'd like your opinion 
> on this issue. 

I'll opine for option 2.


> Is the behaviour described in option one acceptable ?

Is it possible to implement consistent behavior across all resizer
drivers, or will applications need apriori knowledge of the constraints
of underlying hardware device just to crop and scale an image?

>  Should 
> the API be extended/modified to make it simpler for applications to configure 
> the various sizes in the image pipeline ?


>  Are we all doomed and will we have 
> to use a crop/scale API that nobody will ever understand ? :-)

I don't know.  It would be an interesting exercise to

1. collect the assumptions people and applications currently make for a
"crop" function and a "scale" function

2. look at what behavior and assumptions our current API and drivers
make for "crop" and "scale" functions 

3. determine what assumptions or behaviors can be supported across
different hardware/software resizer units, not just the OMAP.

Regards,
Andy

Re: [RFC] Cropping and scaling with subdev pad-level operations

[Laurent Pinchart]

Hi Andy,

On Thursday 06 January 2011 19:28:56 Andy Walls wrote:
> On Thu, 2011-01-06 at 16:33 +0100, Laurent Pinchart wrote:
> > Hi everybody,
> > 
> > I ran into an issue when implementing cropping and scaling on the OMAP3
> > ISP resizer sub-device using the pad-level operations. As nobody seems
> > to be happy with the V4L2 crop ioctls, I thought I would ask for
> > comments about the subdev pad-level API to avoid repeating the same
> > mistakes.
> > 
> > A little background information first. The OMAP3 ISP resizer has an input
> > and an output pad. It receives images on its input pad, performs
> > cropping to a user-configurable rectange and then scales that rectangle
> > up or down to a user-configurable output size. The resulting image is
> > output on the output pad.
> > 
> > The hardware sets various restrictions on the crop rectangle and on the
> > output size, or more precisely on the relationship between them.
> > Horizontal and vertical scaling ratios are independent (at least
> > independent enough for the purpose of this discussion), so I'll will not
> > comment on one of them in particular.
> > 
> > The minimum and maximum scaling ratios are roughly 1/4 and x4. A complex
> > equation describes the relationship between the ratio, the cropped size
> > and the output size. It involves integer arithmetics and must be
> > fullfilled exactly, so not all combination of crop rectangle and output
> > size can be achieved.
> > 
> > The driver expects the user to set the input size first. It propagates
> > the input format to the output pad, resetting the crop rectangle. That
> > behaviour is common to all the OMAP3 ISP modules, and I don't think much
> > discussion is needed there.
> 
> I'll note here that the driver is allowing the application to make *two*
> assumptions here:
> 
> 	output size == input size
> and
> 	output pixel resolution == input pixel resolution
> 
> If I'm taking a picture of a building, at a distance from the building
> such that the input image has a resolution of 1 pixel per 5 cm in the
> plane of the building, then the output image also has a pixel resolution
> of 1 pixel per 5 cm in the plane of the building.

I'm not sure to follow you here. If the resizer upscales the image by 2, you 
will have 2 pixels per 5 cm at the output.

> > The user can then configure the crop rectangle and the output size
> > independently. As not all combinations are possible, configuring one of
> > them can modify the other one as a side effect.
> 
> What enforces the modification, the hardware or the driver?

The hardware requires several conditions to be fulfilled, and the driver 
enforces that.

> IMO, a crop should be a crop with no scaling or interpolation side
> effects.  If I have 1 pixel per 5 cm on the input, I should get 1 pixel
> per 5 cm on the output.

Correct, but I'm not talking about crop only. The OMAP3 ISP resizer first 
crops the input image, and then scales the cropped image to output a cropped 
and resized image.

> Changing the output size when setting a new crop window is IMO the
> correct thing to do since:
> 
> a. it maintains the pixel resolution (e.g. 1 pixel per 5 cm).
> 
> b. it is a predictable result that people and applications can rely upon
> 
> c. It fail due to any implicit zoom constraint violation
> 
> d. The application also knows what the new output size will be, since it
> just set the crop window to that same size
> 
> >  This is where problems come from.
> > 
> > Let's assume that the input size, the crop rectangle and the output size
> > are all set to 4000x4000. The user then wants to crop a 500x500
> > rectangle and scale it up to 750x750.
> > 
> > If the user first sets the crop rectangle to 500x500,  the 4000x4000
> > output size would result in a x8 scaling factor, not supported by the
> > resizer. The driver must then either modify the requested crop rectangle
> > or the output size to fullfill the hardware requirements.
> 
> My suggestion is to have the driver modify the output size as a side
> effect.  Changing the output size already happens as a side effect when
> the application sets the input size.

That's what we're currently doing, but it introduces an issue, see below.

> > If the user first sets the output size to 750x750 we end up with a
> > similar problem, and the driver needs to modify one of crop rectangle or
> > output size as well.
> > 
> > When the stream is on, the output size can't be modified as it would
> > change the captured frame size.
> 
> Hmm.
> 
> >  The crop rectangle and scaling ratios, on the other
> > 
> > hand, can be modified to implement digital zoom. For that reason, the
> > resizer driver doesn't modify the output size when the crop rectangle is
> > set while a stream is running, but restricts the crop rectangle size.
> > With the above example as a starting point, requesting a 500x500 crop
> > rectangle, which would result in an unsupported x8 zoom, will return a
> > 1000x1000 crop rectangle.
> 
> But in this situation - fixed input size, fixed output size - you know
> exactly what scaling levels are supported, 1/4x to 4x, right?

The driver know what scaling factors are supported, but applications don't.

> It sounds like this can be hidden from userpace with different behavior
> inside the driver for the crop and scale API calls when streaming.
> 
> 1.  When streaming, a single crop or scale API request from userspace
> would cause the driver to command all the required underlying crop and
> scale operations for the resizer.
> 
> 2. When streaming stops, either
> a. require the applications to query the state of the crop window and
> output size, or
> b. require drivers to revert to the original crop and scale settings
> that were in place before streaming started
> 
> Bad idea?

I don't like 2.b., there's no real reason to revert the settings. As for 1., 
what do you mean about a "single crop or scale API request" ? Scaling is 
currently requested by modifying the input and/or output sizes (the input size 
being the cropped rectangle in this case). While streaming, digital zoom is 
thus performed by modifying the crop rectangle, without touching the output 
size.

> > When the stream is off, we have two options:
> > 
> > - Handle crop rectangle modifications the same way as when the stream is
> > on. This is cleaner, but bring one drawback. The user can't set the crop
> > rectangle to 500x500 and output size to 750x750 directly. No matter
> > whether the crop rectangle or output size is set first, the intermediate
> > 500x5000/4000x4000 or 4000x4000/750x750 combination are invalid. An
> > extra step will be needed: the crop rectangle will first be set to
> > 1000x1000, the output size will then be set to 750x750, and the crop
> > rectangle will finally be set to 500x500. That won't make life easy for
> > userspace applications.
> 
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> There's a reason why you shouldn't choose this option.
> 
> Are the constraints between crop and output size going to be the same
> for every resizer hardware variant or every driver?

I don't know, I don't have enough experience with similar hardware.

> > - Modify the output size when the crop rectangle is set. With this
> > option, the output size is automatically set to the crop rectangle size
> > when the crop rectangle is changed. With the above example, setting the
> > crop rectangle to 500x500 will automatically set the output size to
> > 500x500, and the user will then just have to set the output size to
> > 750x750.
> 
> I like this one.  The result of the side effect is easily understood and
> should be easy to implement for different hardware using the same API.

I agree, and that's what the resizer driver currently implements. The issue 
with this is described in my previous e-mail: applications can't query the 
maximum zoom factor (or rather the minimum input crop rectangle size).

> > The second option has a major drawback as well, as there's no way for
> > applications to query the minimum/maximum zoom factor. With the first
> > option an application can set the desired output size, and then set a
> > very small crop rectangle to retrieve the minimum allowed crop rectangle
> > (and thus the maximum zoom factor). With the second option the output
> > size will be changed when the crop rectangle is set, so this isn't
> > possible anymore.
> > 
> > Retrieving the maximum zoom factor in the stream off state is an
> > application requirement to be able to display the zoom level on a GUI
> > (with a slider for instance).
> 
> The application trying to make indirect behavior measurements to
> determine maximum/minimum zoom seems odd.  Why can't the driver just
> report those directly via some control or query?  The driver should know
> the answer.

Setting the crop rectangle is such a query :-) What else would you use ?

> > The OMAP3 ISP resizer currently implements the second option, and I'll
> > modify it to implement the first option. The drawback is that some
> > crop/output combinations will require an extra step to be achieved. I'd
> > like your opinion on this issue.
> 
> I'll opine for option 2.
> 
> > Is the behaviour described in option one acceptable ?
> 
> Is it possible to implement consistent behavior across all resizer
> drivers, or will applications need apriori knowledge of the constraints
> of underlying hardware device just to crop and scale an image?

I don't know what constraints similar resizer hardware have, so I can't answer 
this.

> > Should the API be extended/modified to make it simpler for applications to
> > configure the various sizes in the image pipeline ?
> > 
> >  Are we all doomed and will we have to use a crop/scale API that nobody
> >  will ever understand ? :-)
> 
> I don't know.  It would be an interesting exercise to
> 
> 1. collect the assumptions people and applications currently make for a
> "crop" function and a "scale" function

Hence this RFC :-)

> 2. look at what behavior and assumptions our current API and drivers
> make for "crop" and "scale" functions
> 
> 3. determine what assumptions or behaviors can be supported across
> different hardware/software resizer units, not just the OMAP.

Thank you for your answer.

-- 
Regards,

Laurent Pinchart

Re: [RFC] Cropping and scaling with subdev pad-level operations

[Eino-Ville Talvala]

On 1/6/2011 7:33 AM, Laurent Pinchart wrote:
> Hi everybody,
...
> The OMAP3 ISP resizer currently implements the second option, and I'll modify
> it to implement the first option. The drawback is that some crop/output
> combinations will require an extra step to be achieved. I'd like your opinion
> on this issue. Is the behaviour described in option one acceptable ? Should
> the API be extended/modified to make it simpler for applications to configure
> the various sizes in the image pipeline ? Are we all doomed and will we have
> to use a crop/scale API that nobody will ever understand ? :-)
>
I'm personally a big fan of having some way to atomically set multiple 
settings at once, exactly to avoid these sorts of problems. The 
fundamental problem here is that the interface implicitly assumes that 
every intermediate state has to be a valid one, when during device 
configuration most states are transitory because the application hasn't 
finished configuring the pipeline/sensor/etc, and the state shouldn't 
get vetted and adjusted until the configuration is complete.  The 
VIDOC_S_EXT_CTRLS seems like a reasonable solution - can't something 
like that apply to the subdev interfaces? (Or am I missing something 
beyond that?)

Similar issues have cropped up for us with other interdependent settings 
like exposure time/frame duration, or the cropping/scaling options found 
directly on the mt9p031 sensor, which are quite analogous to your issue 
- there's 1-4x scaling and a selectable ROI, which interact, especially 
during streaming when a constant output size has to be maintained.  What 
I ended up doing was effectively hacking in an atomic control update 
procedure for the old v4l2_int_device stuff (very hacky), but then I 
didn't have to worry about it any more.

In general, I'd be worried if executing the same stream of control 
updates in a different order gave a different final result.  With atomic 
updates, you'd still have to decide how to round to the closest valid 
state, but at least it'd be consistent.

Regards,

Eino-Ville Talvala
Stanford University

Re: [RFC] Cropping and scaling with subdev pad-level operations

[Laurent Pinchart]

Hi,

On Tuesday 11 January 2011 20:06:52 Eino-Ville Talvala wrote:
> On 1/6/2011 7:33 AM, Laurent Pinchart wrote:
> > Hi everybody,
> 
> ...
> 
> > The OMAP3 ISP resizer currently implements the second option, and I'll
> > modify it to implement the first option. The drawback is that some
> > crop/output combinations will require an extra step to be achieved. I'd
> > like your opinion on this issue. Is the behaviour described in option
> > one acceptable ? Should the API be extended/modified to make it simpler
> > for applications to configure the various sizes in the image pipeline ?
> > Are we all doomed and will we have to use a crop/scale API that nobody
> > will ever understand ? :-)
> 
> I'm personally a big fan of having some way to atomically set multiple
> settings at once, exactly to avoid these sorts of problems. The
> fundamental problem here is that the interface implicitly assumes that
> every intermediate state has to be a valid one, when during device
> configuration most states are transitory because the application hasn't
> finished configuring the pipeline/sensor/etc, and the state shouldn't
> get vetted and adjusted until the configuration is complete.

Agreed, that's the exact cause of the problem.

> The VIDOC_S_EXT_CTRLS seems like a reasonable solution - can't something
> like that apply to the subdev interfaces? (Or am I missing something
> beyond that?)

I haven't thought about setting multiple formats in one go, but the idea is 
definitely worth a thought. It shouldn't be too difficult to implement, but we 
need to define proper semantics. I'd like to hear about other's opinions.

> Similar issues have cropped up for us with other interdependent settings
> like exposure time/frame duration, or the cropping/scaling options found
> directly on the mt9p031 sensor, which are quite analogous to your issue
> - there's 1-4x scaling and a selectable ROI, which interact, especially
> during streaming when a constant output size has to be maintained.  What
> I ended up doing was effectively hacking in an atomic control update
> procedure for the old v4l2_int_device stuff (very hacky), but then I
> didn't have to worry about it any more.
> 
> In general, I'd be worried if executing the same stream of control
> updates in a different order gave a different final result.  With atomic
> updates, you'd still have to decide how to round to the closest valid
> state, but at least it'd be consistent.

Thanks a lot for sharing your thoughts.

-- 
Regards,

Laurent Pinchart

Re: [RFC] Cropping and scaling with subdev pad-level operations

[Sylwester Nawrocki]

Hi Laurent,

On 01/06/2011 04:33 PM, Laurent Pinchart wrote:
> Hi everybody,
> 
> I ran into an issue when implementing cropping and scaling on the OMAP3 ISP
> resizer sub-device using the pad-level operations. As nobody seems to be happy
> with the V4L2 crop ioctls, I thought I would ask for comments about the subdev
> pad-level API to avoid repeating the same mistakes.
> 
> A little background information first. The OMAP3 ISP resizer has an input and
> an output pad. It receives images on its input pad, performs cropping to a
> user-configurable rectange and then scales that rectangle up or down to a
> user-configurable output size. The resulting image is output on the output
> pad.
> 
> The hardware sets various restrictions on the crop rectangle and on the output
> size, or more precisely on the relationship between them. Horizontal and
> vertical scaling ratios are independent (at least independent enough for the
> purpose of this discussion), so I'll will not comment on one of them in
> particular.
> 
> The minimum and maximum scaling ratios are roughly 1/4 and x4. A complex
> equation describes the relationship between the ratio, the cropped size and
> the output size. It involves integer arithmetics and must be fullfilled
> exactly, so not all combination of crop rectangle and output size can be
> achieved.
> 
> The driver expects the user to set the input size first. It propagates the
> input format to the output pad, resetting the crop rectangle. That behaviour
> is common to all the OMAP3 ISP modules, and I don't think much discussion is
> needed there.
> 
> The user can then configure the crop rectangle and the output size
> independently. As not all combinations are possible, configuring one of them
> can modify the other one as a side effect. This is where problems come from.
> 
> Let's assume that the input size, the crop rectangle and the output size are
> all set to 4000x4000. The user then wants to crop a 500x500 rectangle and
> scale it up to 750x750.
> 
> If the user first sets the crop rectangle to 500x500,  the 4000x4000 output
> size would result in a x8 scaling factor, not supported by the resizer. The
> driver must then either modify the requested crop rectangle or the output size
> to fullfill the hardware requirements.
> 
> If the user first sets the output size to 750x750 we end up with a similar
> problem, and the driver needs to modify one of crop rectangle or output size
> as well.
> 
> When the stream is on, the output size can't be modified as it would change
> the captured frame size. The crop rectangle and scaling ratios, on the other
> hand, can be modified to implement digital zoom. For that reason, the resizer
> driver doesn't modify the output size when the crop rectangle is set while a
> stream is running, but restricts the crop rectangle size. With the above
> example as a starting point, requesting a 500x500 crop rectangle, which would
> result in an unsupported x8 zoom, will return a 1000x1000 crop rectangle.
> 
> When the stream is off, we have two options:
> 
> - Handle crop rectangle modifications the same way as when the stream is on.
> This is cleaner, but bring one drawback. The user can't set the crop rectangle
> to 500x500 and output size to 750x750 directly. No matter whether the crop
> rectangle or output size is set first, the intermediate 500x5000/4000x4000 or
> 4000x4000/750x750 combination are invalid. An extra step will be needed: the
> crop rectangle will first be set to 1000x1000, the output size will then be
> set to 750x750, and the crop rectangle will finally be set to 500x500. That
> won't make life easy for userspace applications.
> 
> - Modify the output size when the crop rectangle is set. With this option, the
> output size is automatically set to the crop rectangle size when the crop
> rectangle is changed. With the above example, setting the crop rectangle to
> 500x500 will automatically set the output size to 500x500, and the user will
> then just have to set the output size to 750x750.

IMO, with the second option at some point it might get difficult to determine
in the application which parameters in the driver may change when the application
tries to change some parameter. I would expect the side effects to be as local
as possible so the application could possibly get notified about them without
additional steps.

> 
> The second option has a major drawback as well, as there's no way for
> applications to query the minimum/maximum zoom factor. With the first option
> an application can set the desired output size, and then set a very small crop
> rectangle to retrieve the minimum allowed crop rectangle (and thus the maximum
> zoom factor). With the second option the output size will be changed when the
> crop rectangle is set, so this isn't possible anymore.
> 
> Retrieving the maximum zoom factor in the stream off state is an application
> requirement to be able to display the zoom level on a GUI (with a slider for
> instance).

In the Samsung S5P FIMC driver minimum and maximum scaling ratios are 1/64 
and 64. So the scaling limits bite a bit less than in your case in typical
applications, the problem remains still same though.
The driver uses the v4l2 mem-to-mem framework so it may be considered much
as your resizer example with an input and output pad. The FIMC H/W supports 
cropping at the scaler input and also an effective output rectangle can be
positioned within the output buffer. The latter allows e.g. placing the video
window at the arbitrary position on a framebuffer.

Currently, with the mem-to-mem driver the application is required to set
the format at the device input and output first (V4L2_BUF_TYPE_OUTPUT and 
*_CAPTURE stream respectively). The relation between both image formats, 
i.e. scaling ratio was not being checked in s_fmt because it also depended 
on whether the rotator was enabled or not. So the check was postponed to 
actual transaction setup/start. This seems wrong to me and I want to change
it so the scaler limits are checked in try/set_fmt, try/set_crop
and s_control(ROTATION).

Then when the crop rectangle is set it is being checked for the scaling
ratio limit against current crop/full window size at the opposite side 
of the scaler.  When a scaling ratio is not within the supported range an error
is returned. The crop  window is adjusted in s_crop only when the device's
alignment requirements would not have been fulfilled. But I am going to change
that so the crop rectangle is adjusted according to the resizer limits as well,
without changing the effective image size at the opposite side of the scaler. 

> 
> The OMAP3 ISP resizer currently implements the second option, and I'll modify
> it to implement the first option. The drawback is that some crop/output
> combinations will require an extra step to be achieved. I'd like your opinion
> on this issue. Is the behaviour described in option one acceptable ? Should
> the API be extended/modified to make it simpler for applications to configure
> the various sizes in the image pipeline ? Are we all doomed and will we have

Not sure if it is a good idea, but with the introduction of the pad operations
maybe it is worth to introduce some flags to vidioc_try/s_crop selecting the 
exact behavior? However current struct v4l2_crop is rather resistant to any 
backward compatible extensions.

Just my $0.2.

Regards,
Sylwester

Re: [RFC] Cropping and scaling with subdev pad-level operations

[Laurent Pinchart]

Hi Sylwester,

On Wednesday 12 January 2011 00:31:26 Sylwester Nawrocki wrote:
> On 01/06/2011 04:33 PM, Laurent Pinchart wrote:

[snip]

> > When the stream is off, we have two options:
> > 
> > - Handle crop rectangle modifications the same way as when the stream is
> > on. This is cleaner, but bring one drawback. The user can't set the crop
> > rectangle to 500x500 and output size to 750x750 directly. No matter
> > whether the crop rectangle or output size is set first, the intermediate
> > 500x5000/4000x4000 or 4000x4000/750x750 combination are invalid. An
> > extra step will be needed: the crop rectangle will first be set to
> > 1000x1000, the output size will then be set to 750x750, and the crop
> > rectangle will finally be set to 500x500. That won't make life easy for
> > userspace applications.
> > 
> > - Modify the output size when the crop rectangle is set. With this
> > option, the output size is automatically set to the crop rectangle size
> > when the crop rectangle is changed. With the above example, setting the
> > crop rectangle to 500x500 will automatically set the output size to
> > 500x500, and the user will then just have to set the output size to
> > 750x750.
> 
> IMO, with the second option at some point it might get difficult to
> determine in the application which parameters in the driver may change
> when the application tries to change some parameter. I would expect the
> side effects to be as local as possible so the application could possibly
> get notified about them without additional steps.

I quite agree with that. Otherwise applications will need to guess what the 
side effects are, and they will end up hardcoding behaviours depending on the 
device model. That's bad.

> > The second option has a major drawback as well, as there's no way for
> > applications to query the minimum/maximum zoom factor. With the first
> > option an application can set the desired output size, and then set a
> > very small crop rectangle to retrieve the minimum allowed crop rectangle
> > (and thus the maximum zoom factor). With the second option the output
> > size will be changed when the crop rectangle is set, so this isn't
> > possible anymore.
> > 
> > Retrieving the maximum zoom factor in the stream off state is an
> > application requirement to be able to display the zoom level on a GUI
> > (with a slider for instance).
> 
> In the Samsung S5P FIMC driver minimum and maximum scaling ratios are 1/64
> and 64. So the scaling limits bite a bit less than in your case in typical
> applications, the problem remains still same though.
> The driver uses the v4l2 mem-to-mem framework so it may be considered much
> as your resizer example with an input and output pad. The FIMC H/W supports
> cropping at the scaler input and also an effective output rectangle can be
> positioned within the output buffer. The latter allows e.g. placing the
> video window at the arbitrary position on a framebuffer.
> 
> Currently, with the mem-to-mem driver the application is required to set
> the format at the device input and output first (V4L2_BUF_TYPE_OUTPUT and
> *_CAPTURE stream respectively). The relation between both image formats,
> i.e. scaling ratio was not being checked in s_fmt because it also depended
> on whether the rotator was enabled or not. So the check was postponed to
> actual transaction setup/start. This seems wrong to me and I want to change
> it so the scaler limits are checked in try/set_fmt, try/set_crop
> and s_control(ROTATION).
> 
> Then when the crop rectangle is set it is being checked for the scaling
> ratio limit against current crop/full window size at the opposite side
> of the scaler.  When a scaling ratio is not within the supported range an
> error is returned. The crop  window is adjusted in s_crop only when the
> device's alignment requirements would not have been fulfilled. But I am
> going to change that so the crop rectangle is adjusted according to the
> resizer limits as well, without changing the effective image size at the
> opposite side of the scaler.

So that's option number 1. I think it's the best one (unless we can find a 
better option 3, such as setting formats and crop rectangles on multiple pads 
atomically).

> > The OMAP3 ISP resizer currently implements the second option, and I'll
> > modify it to implement the first option. The drawback is that some
> > crop/output combinations will require an extra step to be achieved. I'd
> > like your opinion on this issue. Is the behaviour described in option
> > one acceptable ? Should the API be extended/modified to make it simpler
> > for applications to configure the various sizes in the image pipeline ?
> > Are we all doomed and will we have
> 
> Not sure if it is a good idea, but with the introduction of the pad
> operations maybe it is worth to introduce some flags to vidioc_try/s_crop
> selecting the exact behavior? However current struct v4l2_crop is rather
> resistant to any backward compatible extensions.

I'm using

struct v4l2_subdev_crop {
        __u32 which;
        __u32 pad;
        struct v4l2_rect rect;
        __u32 reserved[10];
};

to set crop rectangles on subdevs, so it shouldn't be an issue. I'm not sure 
if it's a good idea though, as it would make both drivers and applications 
more complex. I think I like the idea of setting multiple formats and crop 
rectangles atomically, but I have to think more about it.

-- 
Regards,

Laurent Pinchart

Re: [RFC] Cropping and scaling with subdev pad-level operations

[Hans Verkuil]

Hi Laurent,

My apologies that this reply is so late, but I knew I had to sit down and think
carefully about this and I didn't have time for that until today.

I've decided not to quote your post but instead restate the problem in my own
words.

Seen abstractly you have an entity with inputs, outputs and a processing pipeline
that transforms the input(s) to the output(s) somehow. And each stage of that
pipeline has its own configuration.

You want to provide the developer that sets up such an entity with a systematic
and well-defined procedure while at the same time you want to keep the entity's
code as simple as possible.

Part of this procedure is already decided: you always start with defining the
inputs. The confusion begins with how to setup the configuration and the outputs.

I think the only reasonably way is to configure the pipeline stages in strict order
from the input to the output.

So you start with defining the inputs. For a scaler the next thing to set is the
crop rectangle. And then finally the output.

At each stage the entity will only check if the configuration parameters can work
with the input. It may make the rest of the pipeline invalid, but that's OK for
now.

The final step comes when the output is defined. If that returns OK, then you
know the whole pipeline is valid.

So what to do when you have an input and an output configured and someone suddenly
changes the crop configuration to a value that doesn't work with that input/output
combinations?

The two options I've seen are:

1) modify the crop configuration to fit the output
2) modify the output configuration to fit the crop

But there is a third option which I think works much better:

3) accept the crop configuration if it fits the input, but report back to the caller
   that it invalidates the output.

What the entity does with this crop configuration is purely up to the entity: it can
just store it in a shadow configuration and wait with applying it to the hardware
until it receives a consistent output format, or it can modify the output format to
fit the crop configuration. In any case, if changes are made, then they should be
made 'upstream'. This makes the behavior consistent. As long as you work your way
from input to output, you know that any configuration you've set for earlier stages
stay put and won't change unexpectedly.

And if you decide to change a 'mid-pipeline' configuration, then you will get a
status back that tells you whether or not you broke the upstream configuration.

The advantage is that at any stage the driver code just has to check whether the
new configuration is valid given the input (in which case the call will succeed)
and whether the new configuration is valid for the current output and report that
in a status field.

Note that 'input' and 'output' do not necessarily refer to pads, it may also refer
to stages of a pipeline. And the same principle should hold for both the subdev
API and for the 'main' V4L API (more about that later).

A suggested alternative was to do the configuration atomically. I do not think
that is a good idea because it is so very hard to do things atomically. It is
hard to use and it is hard to implement. The extended controls API for example has
been defined years ago and there are very few drivers that implement it (let alone
implement it correctly). It took the creation of the control framework (almost 2000
lines of code) to make it possible to implement it easily in drivers and I've just
started out converting all drivers to this framework. And this is just for simple
controls...

So this sounds simple, but in practice I think it will be a disaster. I like to
cut things up in small pieces, and working your way through a pipeline as proposed
makes each part much easier.

Since the subdev API is not yet set in stone it shouldn't be hard to provide such
feedback. For the existing V4L2 API it can be a bit harder:

VIDIOC_S_FMT: struct v4l2_pix_format has a priv field which I believe is never used.
So we might hijack that as a flags field instead.

VIDIOC_S_CROP: Can't do anything with that. However, we could define a TRY_CROP that
does give back the information (i.e. 'if you try this crop rectangle, then the
upstream pipeline will become invalid'). Not ideal, but it should work.

ROTATE control: two options: either use one of the top bits in the control ID field
(the top four bits are reserved as flags, one of them is the NEXT_CTRL flag). Or
change the reserved2 field to a flags field.

VIDIOC_S_STD: Can't be done.
VIDIOC_S_INPUT: Can't be done.
VIDIOC_S_OUTPUT: Can't be done.

For these three you might be able to store some sort of information in their ENUM
counterparts.

VIDIOC_S_DV_PRESET: easy to add.
VIDIOC_S_DV_TIMINGS: easy to add.

VIDIOC_S_AUDIO/VIDIOC_S_AUDOUT: easy to add, but not sure if it is needed.

I'm sure there are a lot of details to sort out if we apply this principle as
well to the V4L2 API, but I think that for subdevs this should be a decent way
of doing things. Unless of course I've forgotten about all sorts of subtleties
that I didn't take into account.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC] Cropping and scaling with subdev pad-level operations

[Sakari Ailus]

Hi Hans, others,

Hans Verkuil wrote:
> Hi Laurent,
> 
> My apologies that this reply is so late, but I knew I had to sit down and think
> carefully about this and I didn't have time for that until today.
> 
> I've decided not to quote your post but instead restate the problem in my own
> words.
> 
> Seen abstractly you have an entity with inputs, outputs and a processing pipeline
> that transforms the input(s) to the output(s) somehow. And each stage of that
> pipeline has its own configuration.
> 
> You want to provide the developer that sets up such an entity with a systematic
> and well-defined procedure while at the same time you want to keep the entity's
> code as simple as possible.
> 
> Part of this procedure is already decided: you always start with defining the
> inputs. The confusion begins with how to setup the configuration and the outputs.
> 
> I think the only reasonably way is to configure the pipeline stages in strict order
> from the input to the output.
> 
> So you start with defining the inputs. For a scaler the next thing to set is the
> crop rectangle. And then finally the output.
> 
> At each stage the entity will only check if the configuration parameters can work
> with the input. It may make the rest of the pipeline invalid, but that's OK for
> now.

This would mean that there would have to be a strict order in which
parameters would be set since as a result of changes a set of other
settings would be rendered invalid.

Virtually every block in the OMAP 3 ISP, for example, is able to do
cropping. Scaling is only possible with the resizer, though. I would
expect that this holds true for almost all similar pieces of hardware as
the cropping can be implemented using different first line start
addresses, offsets and lengts.

> The final step comes when the output is defined. If that returns OK, then you
> know the whole pipeline is valid.
> 
> So what to do when you have an input and an output configured and someone suddenly
> changes the crop configuration to a value that doesn't work with that input/output
> combinations?
> 
> The two options I've seen are:
> 
> 1) modify the crop configuration to fit the output
> 2) modify the output configuration to fit the crop
> 
> But there is a third option which I think works much better:
> 
> 3) accept the crop configuration if it fits the input, but report back to the caller
>    that it invalidates the output.
>
> What the entity does with this crop configuration is purely up to the
entity: it can
> just store it in a shadow configuration and wait with applying it to the hardware
> until it receives a consistent output format, or it can modify the output format to
> fit the crop configuration. In any case, if changes are made, then they should be
> made 'upstream'. This makes the behavior consistent. As long as you work your way
> from input to output, you know that any configuration you've set for earlier stages
> stay put and won't change unexpectedly.

It should be also possible to change both crop and source format at the
same time since the former may necessitate a change to the latter as
well, but it is a must to apply both for the same frame. Even if the
in-between configuration is valid, it is unwanted.

So I think there would have to be a flag to tell only apply the crop
change when the source format is changed. Also, the formats should be
changeable on different pads in a single operation.

I have to say that I like this in the sense that it does not explicitly
restrict the ioctls that can be used to change settings related to crop
/ scaling, but OTOH I don't see a need for further ioctls. Also single
ioctls have well defined and quite generic functionality.

On the other hand, this would necessitate the driver to cache the user
originating parameters without applying them. If the user uses g_fmt()
before the parameters have been applied, which format should be
returned, the deferred one or the current one?

What do you think about a more generic, explicit defer/commit flag in
the format and crop ioctls? The crop and format settings could be
deferred until another one is given w/o defer flag set. The
functionality would stay essentially the same.

The user space must behave itself but that's expected anyway...

> And if you decide to change a 'mid-pipeline' configuration, then you will get a
> status back that tells you whether or not you broke the upstream configuration.
> 
> The advantage is that at any stage the driver code just has to check whether the
> new configuration is valid given the input (in which case the call will succeed)
> and whether the new configuration is valid for the current output and report that
> in a status field.
> 
> Note that 'input' and 'output' do not necessarily refer to pads, it may also refer
> to stages of a pipeline. And the same principle should hold for both the subdev
> API and for the 'main' V4L API (more about that later).
> 
> A suggested alternative was to do the configuration atomically. I do not think
> that is a good idea because it is so very hard to do things atomically. It is
> hard to use and it is hard to implement. The extended controls API for example has
> been defined years ago and there are very few drivers that implement it (let alone
> implement it correctly). It took the creation of the control framework (almost 2000
> lines of code) to make it possible to implement it easily in drivers and I've just
> started out converting all drivers to this framework. And this is just for simple
> controls...

I beg to differ a little regarding the extended controls. There are use
cases where you want to apply a set of settings on a sensor (for
example!) atomically. Such settings include digital and analog gain and
exposure.

Failure to apply the settings atomically will lead to bad images, either
too dark or too bright. The sensors typically contain commit
functionality so that the settings may be applied simultaneously at
device level after the sensor driver has processed all.

This can be implemented either using extended controls where the driver
will handle the whole set, or a separate commit ioctl would be needed.

This is somewhat analogous to atomic crop / format setting which Laurent
proposed, except that these settings have harder dependencies. Bad
combinations are impossible instead of just unwanted.

> So this sounds simple, but in practice I think it will be a disaster. I like to
> cut things up in small pieces, and working your way through a pipeline as proposed
> makes each part much easier.

I don't think this would be a problem inside a single entity. All the
parameters and their dependencies are well known for the driver.

The ioctl could be an array of configurations for format and crop. They
could be applied in the order they are placed in the array.

Simplified example:

#define V4L2_SUBDEV_FMTCROP_FMT		1
#define V4L2_SUBDEV_FMTCROP_CROP	2

struct v4l2_subdev_fmtcrop {
	__u32 type;
	union {
		struct v4l2_subdev_format fmt;
		struct v4l2_subdev_crop crop;
		__u8 __headroom[124];
	} u;
};

struct v4l2_subdev_fmtcrops {
	__u32 count;
	struct v4l2_subdev_fmtcrop *fmtcrop;
};

#define VIDIOC_SUBDEV_S_FMTCROP _IOWR('V', x, struct v4l2_subdev_fmtcrops)

I don't think this is really different than your proposal except that
the information is passed to the driver in a single ioctl instead of many.

I think this would also be less confusing and easier to use from the
user space. The user space always has the freedom to set only either
crop or format if it sees that best.

Also, the driver wouldn't need to cache the crop (or format) settings.

Cheers,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com