Re: [RFC v3] [RFC] v4l2: Support for multiple selections
Hi Ricardo, Please refer to the comments below. On 12/10/2013 09:37 AM, Ricardo Ribalda Delgado wrote: Hello Tomasz Now is my time to say sorry for the delay, but i have been in holidays and then I had a pile of work waiting on my desk :). No problem. On Tue, Nov 12, 2013 at 3:54 PM, Tomasz Stanislawski t.stanisl...@samsung.com wrote: Hi Ricardo, Sorry for a late reply. I've been 'offline' for the last two weeks. Please refer to the comments below. [snip] As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on. So activation of rectangle B (setting height to non-zero value) will enable rectangle C with some default size. Moreover disabling rectangle B (setting height to 0) may disable rectangle C automatically. I do not follow what is the problem here? Lets say you want a configuration composed by 3 rectangles ABC and there are no pair of rectangles with a legal configuration. You cannot do step by step configuration. Also lets say that your sensor requires that the total size of the image is 700 lines on 3 rectanges and 500 on 4. You cannot do this configuration step by step. Please, give more details. I do not think that situation is that problematic. Is it even a practical limitation of hardware? Hmm. I think that the real problem is much different. Not how to set up rectangles atomically but rather how do anything non-trivial atomically in V4L2. It would be very nice to have crop/compose and format configured at the same time. However, current version of V4L2 API does not support that. Setting multiple crop rectangles may help a bit for only this little case. But how would like to set multicrop rectangles and multicompose rectangle atomically. How to define which crop rectangle refers to which to which compose rectangle. The number of retangles in crop are the same number of rectables in the compose. Crop[0] corresponds to compose[0], crop[1]to compose[1] and so on I mean something different. While using multirectangle selection, one still must use two VIDIOC_S_SELECTION calls. One to set crop rectangles, second one to set compose rectangles. So you cannot set crop and compose atomically, anyway. Hans proposed some extension to support atomic setup of both properties. However I think that it is a little overengineered. I still does not solve problems with flipping and rotations, which may have a huge impact on mulitrect cropping/composing limitations. What to do if one would like to change only 3rd crop rectangle? You send the whole configuration. The same as today when the user only wants to change the pixel format. He still have to send the size. Introduce rectangle id into v4l2_ext_rect? Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION? Is it really scalable? Why it is not scalable?It is much more scalable than 8 ioctls to set 8 rectangles. One has to copy_from_user/copy_to_user an arbitrary amount of data. One cannot say I would like to change rectangle number 5. Using single S_SELECTION for 1 rectangle is only one ioctl need to be called. When using multirect selections a kernel must copy all data to user (completely needlessly) during G_SELECTION. next, the kernel must load all rectangles back to kernel space (only one rectangle is actually needed to be copied). It is quite probable that copying 8 rectangle will NOT cause any performance issues. But copying 256 rectangles in both directions might cause an observable slowdown. Multirectangle targets may seam to be a solution but I think it is not. I think that atomic transactions are what is really needed in V4L2. Other ideas like try-context from subdev API may also help. It will be nice to have something like VIDIOC_BEGIN VIDIOC_S_SELECTION (target = CROP) VIDIOC_S_SELECTION (target = COMPOSE) VIDIOC_S_FMT VIDIOC_S_CTRL VIDIOC_COMMIT and call a bunch of VIDIOC_G_* to see what really was applied. This will trigger other isues. What we do if 2 programs starts two transactions at the same time. We will keep a transaction array with ALL the configurations? And what happens if there are 100? There are multiple ways to solve the problem. One options would be making VIDIOC_BEGIN a blocking operation so only one thread can access the critical section. Other way would be creation of temporary configuration for each file handle and applying it after VIDIOC_COMMIT. The VIDIOC_COMMIT would he handled in a driver under a mutex. There is a huge space for inventions here. I have an auxiliary question. Do you have to set all rectangles at once? can you set up them one by one? Also if you tell the driver what exact configuration you will need, it will provide you with the closest possible confuration, that cannot be s/cannot be done/may not be 'doable' cannot be done. The driver cannot guess which rectangles will the user set in the future.
Re: [RFC v3] [RFC] v4l2: Support for multiple selections
Hi Sylwester, On 12/19/2013 12:09 PM, Tomasz Stanislawski wrote: Hi Ricardo, Please refer to the comments below. On 12/10/2013 09:37 AM, Ricardo Ribalda Delgado wrote: Hello Tomasz Now is my time to say sorry for the delay, but i have been in holidays and then I had a pile of work waiting on my desk :). No problem. On Tue, Nov 12, 2013 at 3:54 PM, Tomasz Stanislawski t.stanisl...@samsung.com wrote: Hi Ricardo, Sorry for a late reply. I've been 'offline' for the last two weeks. Please refer to the comments below. [snip] As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on. So activation of rectangle B (setting height to non-zero value) will enable rectangle C with some default size. Moreover disabling rectangle B (setting height to 0) may disable rectangle C automatically. I do not follow what is the problem here? Lets say you want a configuration composed by 3 rectangles ABC and there are no pair of rectangles with a legal configuration. You cannot do step by step configuration. Also lets say that your sensor requires that the total size of the image is 700 lines on 3 rectanges and 500 on 4. You cannot do this configuration step by step. Please, give more details. I do not think that situation is that problematic. Is it even a practical limitation of hardware? Hmm. I think that the real problem is much different. Not how to set up rectangles atomically but rather how do anything non-trivial atomically in V4L2. It would be very nice to have crop/compose and format configured at the same time. However, current version of V4L2 API does not support that. Setting multiple crop rectangles may help a bit for only this little case. But how would like to set multicrop rectangles and multicompose rectangle atomically. How to define which crop rectangle refers to which to which compose rectangle. The number of retangles in crop are the same number of rectables in the compose. Crop[0] corresponds to compose[0], crop[1]to compose[1] and so on I mean something different. While using multirectangle selection, one still must use two VIDIOC_S_SELECTION calls. One to set crop rectangles, second one to set compose rectangles. So you cannot set crop and compose atomically, anyway. Hans proposed some extension to support atomic setup of both properties. However I think that it is a little overengineered. I disagree. I implemented it in vivi and it turned out to be quite easy. For the record: I'm talking about this RFC: http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/71822 I still does not solve problems with flipping and rotations, which may have a huge impact on mulitrect cropping/composing limitations. My proposal will make that much easier as well since flipping, rotating, cropping and composing are all controls/properties that can be set atomically (a control cluster). So drivers can create a single function that can handle all the relationships in one place, and applications can set all of these with one VIDIOC_S_EXT_CTRLS call. What to do if one would like to change only 3rd crop rectangle? You send the whole configuration. The same as today when the user only wants to change the pixel format. He still have to send the size. Introduce rectangle id into v4l2_ext_rect? Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION? Is it really scalable? Why it is not scalable?It is much more scalable than 8 ioctls to set 8 rectangles. One has to copy_from_user/copy_to_user an arbitrary amount of data. One cannot say I would like to change rectangle number 5. Using single S_SELECTION for 1 rectangle is only one ioctl need to be called. When using multirect selections a kernel must copy all data to user (completely needlessly) during G_SELECTION. next, the kernel must load all rectangles back to kernel space (only one rectangle is actually needed to be copied). It is quite probable that copying 8 rectangle will NOT cause any performance issues. But copying 256 rectangles in both directions might cause an observable slowdown. Adding matrix support is part of my proposal (i.e. allowing for 2D-arrays of properties/controls. An array is just a 1xN matrix). Originally I thought it would be a good idea to prefix the matrix with a struct v4l2_rect where the application could set which sub-matrix it wants to get/set. E.g. if the full 2D array is 10x20, then you could just obtain a 2x2 matrix at position 5x4. While that works, I've decided to drop that. It is quite cumbersome and I am not convinced of its usefulness. Should it be needed in the future, then it can be added (a flag would need to be added in struct v4l2_ext_control to tell the framework that the matrix is preceeded by a struct v4l2_rect). But before I would do that I would have to see a real use-case where this is a clear performance issue. Adding
Re: [RFC v3] [RFC] v4l2: Support for multiple selections
On 12/19/2013 12:45 PM, Hans Verkuil wrote: Hi Sylwester, That should be Tomasz, of course :-) Hans -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v3] [RFC] v4l2: Support for multiple selections
Hello Tomasz Thanks for your comments. On Thu, Dec 19, 2013 at 12:09 PM, Tomasz Stanislawski t.stanisl...@samsung.com wrote: Hi Ricardo, Please refer to the comments below. On 12/10/2013 09:37 AM, Ricardo Ribalda Delgado wrote: Hello Tomasz Now is my time to say sorry for the delay, but i have been in holidays and then I had a pile of work waiting on my desk :). No problem. On Tue, Nov 12, 2013 at 3:54 PM, Tomasz Stanislawski t.stanisl...@samsung.com wrote: Hi Ricardo, Sorry for a late reply. I've been 'offline' for the last two weeks. Please refer to the comments below. [snip] As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on. So activation of rectangle B (setting height to non-zero value) will enable rectangle C with some default size. Moreover disabling rectangle B (setting height to 0) may disable rectangle C automatically. I do not follow what is the problem here? Lets say you want a configuration composed by 3 rectangles ABC and there are no pair of rectangles with a legal configuration. You cannot do step by step configuration. Also lets say that your sensor requires that the total size of the image is 700 lines on 3 rectanges and 500 on 4. You cannot do this configuration step by step. Please, give more details. I do not think that situation is that problematic. Is it even a practical limitation of hardware? We have an fpga camera that uses X cycles to read a sensor. That X is FIXED. Lets say it is 1000 lines time The sensor is read linearly from a programmable address A. for a lines You can program also address B for b lines Jumping from the area A to the area B needs j lines time. a+b+j must be equal to 1000. On the multiselect, the user sets A and B, the constrains are verified and the system is set. On your proposal Area A is set, but because the size is 1000, the driver changes the size to 1000, probably also changing the initial line a. Area B is set, the driver needs to resize the area A too, but the initial line a (what the user wants) has been replaced by a driver chossen a This is a very simple example with 2 areas, extrapolate it to 8 areas. Hmm. I think that the real problem is much different. Not how to set up rectangles atomically but rather how do anything non-trivial atomically in V4L2. It would be very nice to have crop/compose and format configured at the same time. However, current version of V4L2 API does not support that. Setting multiple crop rectangles may help a bit for only this little case. But how would like to set multicrop rectangles and multicompose rectangle atomically. How to define which crop rectangle refers to which to which compose rectangle. The number of retangles in crop are the same number of rectables in the compose. Crop[0] corresponds to compose[0], crop[1]to compose[1] and so on I mean something different. While using multirectangle selection, one still must use two VIDIOC_S_SELECTION calls. One to set crop rectangles, second one to set compose rectangles. So you cannot set crop and compose atomically, anyway. No, but I can set all the crop rectangles at once. The driver then finds out a compatible configuration of composing. The user replaces that configuration (if needed) with a new compose. Exactly the same as today with one rectangle.. Hans proposed some extension to support atomic setup of both properties. However I think that it is a little overengineered. I still does not solve problems with flipping and rotations, which may have a huge impact on mulitrect cropping/composing limitations. Flip and rotation is affected to whole sensor not to the specific rectangles. What to do if one would like to change only 3rd crop rectangle? You send the whole configuration. The same as today when the user only wants to change the pixel format. He still have to send the size. Introduce rectangle id into v4l2_ext_rect? Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION? Is it really scalable? Why it is not scalable?It is much more scalable than 8 ioctls to set 8 rectangles. One has to copy_from_user/copy_to_user an arbitrary amount of data. One cannot say I would like to change rectangle number 5. Using single S_SELECTION for 1 rectangle is only one ioctl need to be called. If the user have saved the output of the previous s_selection he can change a specific rectangle with one s_selection call On the other hand he can even change all the rectangles with one s_selection, on your example the user has to make N s_selection calls. When using multirect selections a kernel must copy all data to user (completely needlessly) during G_SELECTION. next, the kernel must load all rectangles back to kernel space (only one rectangle is actually needed to be copied). It is quite probable that copying 8 rectangle will NOT cause any performance issues. But copying 256
Re: [RFC v3] [RFC] v4l2: Support for multiple selections
Hi Ricardo, On 12/10/2013 10:46 AM, Ricardo Ribalda Delgado wrote: Hello Tomasz and Hans On Thu, Nov 14, 2013 at 4:40 PM, Tomasz Stanislawski t.stanisl...@samsung.com wrote: Hi Hans, On 11/14/2013 11:18 AM, Hans Verkuil wrote: Hi Tomasz, On 11/12/13 15:54, Tomasz Stanislawski wrote: Hi Ricardo, Sorry for a late reply. I've been 'offline' for the last two weeks. Please refer to the comments below. [snip] As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on. So activation of rectangle B (setting height to non-zero value) will enable rectangle C with some default size. Moreover disabling rectangle B (setting height to 0) may disable rectangle C automatically. I do not follow what is the problem here? The problem would be in a situation like this: .. .AA.B. .. -- AAB .C.DD. CDD .. A-D are the rectangles you want to select. They are cropped as shown on the left and composed as shown on the right. From what Ricardo told me the resulting composed image typically must be a proper rectangle without padding anywhere. Trying to add rectangles one at a time breaks down when adding C because the composition result is no longer a 'proper' rectangle. I don't see how you can set something like that up one rectangle at a time. I see the issue but I think that it is not a big problem. Activating C forms a non-proper rectangle with A and B. Therefore, driver must force enabling D to form a proper rectangle again. I mean that instead of enlarging C to sum of width of A and B, the driver can implicitly activate D to ensure that A,B,C,D form a proper rectangle. I think this will lead to X different drivers with X different behaviours. I sadly have to agree. All drivers behave differently. But, is it really an issue? As I understand a developer is responsible to assure that future versions of driver works the same (in practical context) when used by old applications. The driver is allowed to interpret/adjust user calls. This includes even ignoring them. The special target called V4L2_SEL_TGT_COMPOSE_PADDED was introduced to inform application which part of a buffers if going to be modified with some undefined value. I see nothing against setting a padded rectangle for C to a rectangle that covers C and D or even the whole ABCD rectangle. I think it could be a great application for PADDED target. The application could easily detect which part of a buffer are affected. Even applications prepared to work with single crop devices would still work after enabling multi crop mode. The setup of rectangles my look like this. S_SELECTION(CROP0 = A) crop compose -- .. .AA... .. -- AA.. .. .. G_SELECTION(COMPOSE0) AA.. G_SELECTION(COMPOSE0_PADDED) AA.. S_SELECTION(CROP1 = B) .. .AA.B. .. -- AAB. .. .. G_SELECTION(COMPOSE0) AA.. G_SELECTION(COMPOSE0_PADDED) AAA. G_SELECTION(COMPOSE1) ..B. G_SELECTION(COMPOSE1_PADDED) BBB. S_SELECTION(CROP2 = C) - D is activated implicitly .. .AA.B. .C.DD. -- AAB. .. CDD. .. G_SELECTION(COMPOSE0_PADDED) AAA. AAA. G_SELECTION(COMPOSE2) C... G_SELECTION(COMPOSE2_PADDED) CCC. CCC. G_SELECTION(COMPOSE3) .DD. G_SELECTION(COMPOSE3_PADDED) DDD. DDD. One may argue that all this logic is unnecessary after adding support for multirect selections. So, I kindly ask what should happen if someone call S_SELECTION (in multirect mode) passing THREE rectangles A, B, and C (not D) ? The driver must adjust rectangles to some valid value. So it can increase width of C or implicitly activate D or disable C. I think that the best solution is activating D because it allows to set size of C to the value closest to requested one. Therefore logic for implicit activation of D should be implemented anyway. You are assuming that the user will send you the rectangles in an order that makes sense, but it is not always the case. On the other hand if you have all the rectangles, you can ALWAYS make the better decisition. Yes. I assume that a user will send rectangles in the right order. It stated that changing order of V4L2 operations may result is different configuration. So I do not see a problem that a specific configuration can be achieved on specific hardware only if operations are performed in specific order. On the other hand if you have all the rectangles, you can ALWAYS make the better decisition. I agree completely. To make a best
Re: [RFC v3] [RFC] v4l2: Support for multiple selections
Hello Tomasz On Thu, Dec 19, 2013 at 1:34 PM, Tomasz Stanislawski t.stanisl...@samsung.com wrote: Hi Ricardo, On 12/10/2013 10:46 AM, Ricardo Ribalda Delgado wrote: Hello Tomasz and Hans On Thu, Nov 14, 2013 at 4:40 PM, Tomasz Stanislawski t.stanisl...@samsung.com wrote: Hi Hans, On 11/14/2013 11:18 AM, Hans Verkuil wrote: Hi Tomasz, On 11/12/13 15:54, Tomasz Stanislawski wrote: Hi Ricardo, Sorry for a late reply. I've been 'offline' for the last two weeks. Please refer to the comments below. [snip] As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on. So activation of rectangle B (setting height to non-zero value) will enable rectangle C with some default size. Moreover disabling rectangle B (setting height to 0) may disable rectangle C automatically. I do not follow what is the problem here? The problem would be in a situation like this: .. .AA.B. .. -- AAB .C.DD. CDD .. A-D are the rectangles you want to select. They are cropped as shown on the left and composed as shown on the right. From what Ricardo told me the resulting composed image typically must be a proper rectangle without padding anywhere. Trying to add rectangles one at a time breaks down when adding C because the composition result is no longer a 'proper' rectangle. I don't see how you can set something like that up one rectangle at a time. I see the issue but I think that it is not a big problem. Activating C forms a non-proper rectangle with A and B. Therefore, driver must force enabling D to form a proper rectangle again. I mean that instead of enlarging C to sum of width of A and B, the driver can implicitly activate D to ensure that A,B,C,D form a proper rectangle. I think this will lead to X different drivers with X different behaviours. I sadly have to agree. All drivers behave differently. But, is it really an issue? Well, I would say that we have APIs to abstract HW from SW. I expect the same behaviour on every mouse I connect on my computer :) As I understand a developer is responsible to assure that future versions of driver works the same (in practical context) when used by old applications. The driver is allowed to interpret/adjust user calls. This includes even ignoring them. The special target called V4L2_SEL_TGT_COMPOSE_PADDED was introduced to inform application which part of a buffers if going to be modified with some undefined value. I see nothing against setting a padded rectangle for C to a rectangle that covers C and D or even the whole ABCD rectangle. I think it could be a great application for PADDED target. The application could easily detect which part of a buffer are affected. Even applications prepared to work with single crop devices would still work after enabling multi crop mode. The setup of rectangles my look like this. S_SELECTION(CROP0 = A) crop compose -- .. .AA... .. -- AA.. .. .. G_SELECTION(COMPOSE0) AA.. G_SELECTION(COMPOSE0_PADDED) AA.. S_SELECTION(CROP1 = B) .. .AA.B. .. -- AAB. .. .. G_SELECTION(COMPOSE0) AA.. G_SELECTION(COMPOSE0_PADDED) AAA. G_SELECTION(COMPOSE1) ..B. G_SELECTION(COMPOSE1_PADDED) BBB. S_SELECTION(CROP2 = C) - D is activated implicitly .. .AA.B. .C.DD. -- AAB. .. CDD. .. G_SELECTION(COMPOSE0_PADDED) AAA. AAA. G_SELECTION(COMPOSE2) C... G_SELECTION(COMPOSE2_PADDED) CCC. CCC. G_SELECTION(COMPOSE3) .DD. G_SELECTION(COMPOSE3_PADDED) DDD. DDD. One may argue that all this logic is unnecessary after adding support for multirect selections. So, I kindly ask what should happen if someone call S_SELECTION (in multirect mode) passing THREE rectangles A, B, and C (not D) ? The driver must adjust rectangles to some valid value. So it can increase width of C or implicitly activate D or disable C. I think that the best solution is activating D because it allows to set size of C to the value closest to requested one. Therefore logic for implicit activation of D should be implemented anyway. You are assuming that the user will send you the rectangles in an order that makes sense, but it is not always the case. On the other hand if you have all the rectangles, you can ALWAYS make the better decisition. Yes. I assume that a user will send rectangles in the right order. It stated that changing order of V4L2 operations may result is different configuration. So I do not see a problem that a
Re: [RFC v3] [RFC] v4l2: Support for multiple selections
Hi Hans,
We've misunderstood. When I was saying 'overengineered'
I did not mean your RFC.
I was taking about this:
#define V4L2_SEL_TGT_CROP_COMPOSE0x0200
struct v4l2_selection {
__u32 type;
__u32 target;
__u32 flags;
union {
struct v4l2_rectr;
struct v4l2_ext_rect*pr;
};
__u32 flags2;
union {
struct v4l2_rectr2;
struct v4l2_ext_rect*pr2;
};
__u32 rectangles;
__u32 reserved[3];
};
This structure looks scary to me :).
I disagree. I implemented it in vivi and it turned out to be quite easy.
For the record: I'm talking about this RFC:
http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/71822
I still does not solve problems with flipping and rotations, which may
have a huge impact on mulitrect cropping/composing limitations.
My proposal will make that much easier as well since flipping, rotating,
cropping and composing are all controls/properties that can be set
atomically (a control cluster). So drivers can create a single function
that can handle all the relationships in one place, and applications can
set all of these with one VIDIOC_S_EXT_CTRLS call.
I think that your idea is quite good. Solve atomic configuration
in a different part of API (control cluster), not by making
properties larger.
As I said, there are multiple way to handle atomic configuration.
Using control API is one of them. Quite nice BTW :)
Regards,
Tomasz Stanislawski
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v3] [RFC] v4l2: Support for multiple selections
On 12/19/2013 02:30 PM, Tomasz Stanislawski wrote:
Hi Hans,
We've misunderstood. When I was saying 'overengineered'
I did not mean your RFC.
I was taking about this:
#define V4L2_SEL_TGT_CROP_COMPOSE0x0200
struct v4l2_selection {
__u32 type;
__u32 target;
__u32 flags;
union {
struct v4l2_rectr;
struct v4l2_ext_rect*pr;
};
__u32 flags2;
union {
struct v4l2_rectr2;
struct v4l2_ext_rect*pr2;
};
__u32 rectangles;
__u32 reserved[3];
};
This structure looks scary to me :).
Ah, yes. Implementing this as properties works much better.
And multi-selection can be done simply by making an array of crop and
compose rectangles.
Regards,
Hans
I disagree. I implemented it in vivi and it turned out to be quite easy.
For the record: I'm talking about this RFC:
http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/71822
I still does not solve problems with flipping and rotations, which may
have a huge impact on mulitrect cropping/composing limitations.
My proposal will make that much easier as well since flipping, rotating,
cropping and composing are all controls/properties that can be set
atomically (a control cluster). So drivers can create a single function
that can handle all the relationships in one place, and applications can
set all of these with one VIDIOC_S_EXT_CTRLS call.
I think that your idea is quite good. Solve atomic configuration
in a different part of API (control cluster), not by making
properties larger.
As I said, there are multiple way to handle atomic configuration.
Using control API is one of them. Quite nice BTW :)
Regards,
Tomasz Stanislawski
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v3] [RFC] v4l2: Support for multiple selections
Hello Tomasz Now is my time to say sorry for the delay, but i have been in holidays and then I had a pile of work waiting on my desk :). On Tue, Nov 12, 2013 at 3:54 PM, Tomasz Stanislawski t.stanisl...@samsung.com wrote: Hi Ricardo, Sorry for a late reply. I've been 'offline' for the last two weeks. Please refer to the comments below. On 10/28/2013 11:46 PM, Ricardo Ribalda Delgado wrote: Hello Tomasz Sorry for the late reply, but I have been offline the last week due to the conference. On Thu, Oct 24, 2013 at 12:31 PM, Tomasz Stanislawski t.stanisl...@samsung.com wrote: Hi Ricardo, I am the designer of selection API. I hope I can help you a little. I think that there are two issues mixed in 'Mulitple selections' topic. Firstly, you described that you program a piece of hardware that is capable of selecting 8 areas for scanning. Now you are looking for userspace API to support such a feature. The feature of posting multiple rectangle was proposed in this RFC. Secondly, You introduced struct v4l2_ext_rect which is a future-proof version of v4l2_rect. I think that both issues should be solved in two separate patchsets. Ad 1. The selection of multiple scanning areas is a very driver-specific feature, isn't it? I think that you do not need to introduce any abstract interface. What would be other applications of the proposed interface? It is not driver specific. There are many sensors out there that supports multiple window of interest, but today we are ignoring them just because we dont have an api. The main application would be industrial imaging, where less data to read means more fps and therefore the system can run faster. From my field I can tell you that it is a hard requirement for computer vision. And it is a feature that we cannot model through v4l2 controls. OK. So there is no need to implement this feature as a driver-specific API. It can go automatically to generic API. Do you know other drivers that may need it? Sakari mentioned introduction of private targets for selections. I like this idea. Just define: #define V4L2_SEL_TGT_PRIVATE 0x8000 All targets that are = V4L2_SEL_TGT_PRIVATE are driver-specific. Generic applications must not use them. Non-generic application must check out the driver of video node before using selections from private set. If some target becomes more useful and accepted by more then one driver then it can be moved to generic API. The good thing about private target is that enums from different drivers can collide so the target space is not going to be trashed. If you read the previous RFCs you will see that the approach you are mentioning has been rejected. The main issue is that you cannot set atomically all the rectangles. Lets say that the configuration formed by rectangle A, B and C is legal, but the configuration A and B is not allowed by the sensor. How could you set the rectangles one by one? As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on. So activation of rectangle B (setting height to non-zero value) will enable rectangle C with some default size. Moreover disabling rectangle B (setting height to 0) may disable rectangle C automatically. I do not follow what is the problem here? Lets say you want a configuration composed by 3 rectangles ABC and there are no pair of rectangles with a legal configuration. You cannot do step by step configuration. Also lets say that your sensor requires that the total size of the image is 700 lines on 3 rectanges and 500 on 4. You cannot do this configuration step by step. Hmm. I think that the real problem is much different. Not how to set up rectangles atomically but rather how do anything non-trivial atomically in V4L2. It would be very nice to have crop/compose and format configured at the same time. However, current version of V4L2 API does not support that. Setting multiple crop rectangles may help a bit for only this little case. But how would like to set multicrop rectangles and multicompose rectangle atomically. How to define which crop rectangle refers to which to which compose rectangle. The number of retangles in crop are the same number of rectables in the compose. Crop[0] corresponds to compose[0], crop[1]to compose[1] and so on What to do if one would like to change only 3rd crop rectangle? You send the whole configuration. The same as today when the user only wants to change the pixel format. He still have to send the size. Introduce rectangle id into v4l2_ext_rect? Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION? Is it really scalable? Why it is not scalable?It is much more scalable than 8 ioctls to set 8 rectangles. Multirectangle targets may seam to be a solution but I think it is not. I think that atomic transactions are what is really needed in V4L2. Other ideas like try-context from subdev API may
Re: [RFC v3] [RFC] v4l2: Support for multiple selections
Hello Tomasz and Hans On Thu, Nov 14, 2013 at 4:40 PM, Tomasz Stanislawski t.stanisl...@samsung.com wrote: Hi Hans, On 11/14/2013 11:18 AM, Hans Verkuil wrote: Hi Tomasz, On 11/12/13 15:54, Tomasz Stanislawski wrote: Hi Ricardo, Sorry for a late reply. I've been 'offline' for the last two weeks. Please refer to the comments below. [snip] As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on. So activation of rectangle B (setting height to non-zero value) will enable rectangle C with some default size. Moreover disabling rectangle B (setting height to 0) may disable rectangle C automatically. I do not follow what is the problem here? The problem would be in a situation like this: .. .AA.B. .. -- AAB .C.DD. CDD .. A-D are the rectangles you want to select. They are cropped as shown on the left and composed as shown on the right. From what Ricardo told me the resulting composed image typically must be a proper rectangle without padding anywhere. Trying to add rectangles one at a time breaks down when adding C because the composition result is no longer a 'proper' rectangle. I don't see how you can set something like that up one rectangle at a time. I see the issue but I think that it is not a big problem. Activating C forms a non-proper rectangle with A and B. Therefore, driver must force enabling D to form a proper rectangle again. I mean that instead of enlarging C to sum of width of A and B, the driver can implicitly activate D to ensure that A,B,C,D form a proper rectangle. I think this will lead to X different drivers with X different behaviours. The special target called V4L2_SEL_TGT_COMPOSE_PADDED was introduced to inform application which part of a buffers if going to be modified with some undefined value. I see nothing against setting a padded rectangle for C to a rectangle that covers C and D or even the whole ABCD rectangle. I think it could be a great application for PADDED target. The application could easily detect which part of a buffer are affected. Even applications prepared to work with single crop devices would still work after enabling multi crop mode. The setup of rectangles my look like this. S_SELECTION(CROP0 = A) crop compose -- .. .AA... .. -- AA.. .. .. G_SELECTION(COMPOSE0) AA.. G_SELECTION(COMPOSE0_PADDED) AA.. S_SELECTION(CROP1 = B) .. .AA.B. .. -- AAB. .. .. G_SELECTION(COMPOSE0) AA.. G_SELECTION(COMPOSE0_PADDED) AAA. G_SELECTION(COMPOSE1) ..B. G_SELECTION(COMPOSE1_PADDED) BBB. S_SELECTION(CROP2 = C) - D is activated implicitly .. .AA.B. .C.DD. -- AAB. .. CDD. .. G_SELECTION(COMPOSE0_PADDED) AAA. AAA. G_SELECTION(COMPOSE2) C... G_SELECTION(COMPOSE2_PADDED) CCC. CCC. G_SELECTION(COMPOSE3) .DD. G_SELECTION(COMPOSE3_PADDED) DDD. DDD. One may argue that all this logic is unnecessary after adding support for multirect selections. So, I kindly ask what should happen if someone call S_SELECTION (in multirect mode) passing THREE rectangles A, B, and C (not D) ? The driver must adjust rectangles to some valid value. So it can increase width of C or implicitly activate D or disable C. I think that the best solution is activating D because it allows to set size of C to the value closest to requested one. Therefore logic for implicit activation of D should be implemented anyway. You are assuming that the user will send you the rectangles in an order that makes sense, but it is not always the case. On the other hand if you have all the rectangles, you can ALWAYS make the better decisition. It makes much more sense to set everything up at once. I agree that it is better to set everything at once. But I strongly believe that transactions are the proper way to achieve that. Not multirectangle selections. As I said before the transaction is something easier said than made. What happens if multiple users starts a transaction? What happen if in a transaction of 10 itmes, item 7 needs a readjusment by the driver? The selection API cannot cover a type of selection, therefore it should be fixed. Especially when it is something as easy as the propossed RFC. It obfuscates API. It only pretends to fix a problem with applying a part of configuration atomically. BTW, what probably wasn't clear from Ricardo's explanation is that for every crop rectangle you must have a corresponding compose rectangle so that you
Re: [RFC v3] [RFC] v4l2: Support for multiple selections
Hi Tomasz,
On 11/12/13 15:54, Tomasz Stanislawski wrote:
Hi Ricardo,
Sorry for a late reply. I've been 'offline' for the last two weeks.
Please refer to the comments below.
On 10/28/2013 11:46 PM, Ricardo Ribalda Delgado wrote:
Hello Tomasz
Sorry for the late reply, but I have been offline the last week due to
the conference.
On Thu, Oct 24, 2013 at 12:31 PM, Tomasz Stanislawski
t.stanisl...@samsung.com wrote:
Hi Ricardo,
I am the designer of selection API. I hope I can help you a little.
I think that there are two issues mixed in 'Mulitple selections' topic.
Firstly, you described that you program a piece of hardware that is
capable of selecting 8 areas for scanning. Now you
are looking for userspace API to support such a feature.
The feature of posting multiple rectangle was proposed in this RFC.
Secondly, You introduced struct v4l2_ext_rect which is a future-proof
version of v4l2_rect.
I think that both issues should be solved in two separate patchsets.
Ad 1.
The selection of multiple scanning areas is a very driver-specific
feature, isn't it? I think that you do not need to introduce any abstract
interface. What would be other applications of the proposed interface?
It is not driver specific. There are many sensors out there that
supports multiple window of interest, but today we are ignoring them
just because we dont have an api.
The main application would be industrial imaging, where less data to
read means more fps and therefore the system can run faster.
From my field I can tell you that it is a hard requirement for
computer vision. And it is a feature that we cannot model through v4l2
controls.
OK. So there is no need to implement this feature as a driver-specific API.
It can go automatically to generic API.
Do you know other drivers that may need it? Sakari mentioned introduction
of private targets for selections. I like this idea. Just define:
#define V4L2_SEL_TGT_PRIVATE 0x8000
All targets that are = V4L2_SEL_TGT_PRIVATE are driver-specific.
Generic applications must not use them. Non-generic application
must check out the driver of video node before using selections
from private set. If some target becomes more useful and accepted
by more then one driver then it can be moved to generic API.
The good thing about private target is that enums from different
drivers can collide so the target space is not going to be trashed.
If you read the previous RFCs you will see that the approach you are
mentioning has been rejected.
The main issue is that you cannot set atomically all the rectangles.
Lets say that the configuration formed by rectangle A, B and C is
legal, but the configuration A and B is not allowed by the sensor. How
could you set the rectangles one by one?
As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so
on.
So activation of rectangle B (setting height to non-zero value) will enable
rectangle C with some default size. Moreover disabling rectangle B (setting
height to 0)
may disable rectangle C automatically. I do not follow what is the problem
here?
The problem would be in a situation like this:
..
.AA.B.
.. -- AAB
.C.DD. CDD
..
A-D are the rectangles you want to select. They are cropped as shown on the
left and composed as shown on the right.
From what Ricardo told me the resulting composed image typically must be
a proper rectangle without padding anywhere.
Trying to add rectangles one at a time breaks down when adding C because
the composition result is no longer a 'proper' rectangle. I don't see how
you can set something like that up one rectangle at a time.
It makes much more sense to set everything up at once.
BTW, what probably wasn't clear from Ricardo's explanation is that for every
crop rectangle you must have a corresponding compose rectangle so that you
know where to DMA it to.
Your next question will be that it is a real problem that you can't set
crop and compose simultaneously, and you are right about that. Read on for
that... :-)
Hmm. I think that the real problem is much different.
Not how to set up rectangles atomically but rather
how do anything non-trivial atomically in V4L2.
It would be very nice to have crop/compose and format configured at the same
time.
However, current version of V4L2 API does not support that.
Setting multiple crop rectangles may help a bit for only this little case.
But how would like to set multicrop rectangles and multicompose rectangle
atomically.
Why can't we extend the selection API a bit? How about this:
#define V4L2_SEL_TGT_CROP_COMPOSE0x0200
struct v4l2_selection {
__u32 type;
__u32 target;
__u32 flags;
union {
struct v4l2_rectr;
struct v4l2_ext_rect*pr;
};
__u32 flags2;
union {
Re: [RFC v3] [RFC] v4l2: Support for multiple selections
Hi Hans,
On 11/14/2013 11:18 AM, Hans Verkuil wrote:
Hi Tomasz,
On 11/12/13 15:54, Tomasz Stanislawski wrote:
Hi Ricardo,
Sorry for a late reply. I've been 'offline' for the last two weeks.
Please refer to the comments below.
[snip]
As I said. Changes of rectangle n may trigger changes in rectangle n+1 and
so on.
So activation of rectangle B (setting height to non-zero value) will enable
rectangle C with some default size. Moreover disabling rectangle B (setting
height to 0)
may disable rectangle C automatically. I do not follow what is the problem
here?
The problem would be in a situation like this:
..
.AA.B.
.. -- AAB
.C.DD. CDD
..
A-D are the rectangles you want to select. They are cropped as shown on the
left and composed as shown on the right.
From what Ricardo told me the resulting composed image typically must be
a proper rectangle without padding anywhere.
Trying to add rectangles one at a time breaks down when adding C because
the composition result is no longer a 'proper' rectangle. I don't see how
you can set something like that up one rectangle at a time.
I see the issue but I think that it is not a big problem.
Activating C forms a non-proper rectangle with A and B.
Therefore, driver must force enabling D to form a proper rectangle again.
I mean that instead of enlarging C to sum of width of A and B,
the driver can implicitly activate D to ensure that A,B,C,D form a proper
rectangle.
The special target called V4L2_SEL_TGT_COMPOSE_PADDED was introduced
to inform application which part of a buffers if going to be modified
with some undefined value.
I see nothing against setting a padded rectangle for C to a rectangle that
covers C and D or even the whole ABCD rectangle.
I think it could be a great application for PADDED target.
The application could easily detect which part of a buffer are affected.
Even applications prepared to work with single crop devices
would still work after enabling multi crop mode.
The setup of rectangles my look like this.
S_SELECTION(CROP0 = A)
crop compose
--
..
.AA...
.. -- AA..
..
..
G_SELECTION(COMPOSE0)
AA..
G_SELECTION(COMPOSE0_PADDED)
AA..
S_SELECTION(CROP1 = B)
..
.AA.B.
.. -- AAB.
..
..
G_SELECTION(COMPOSE0)
AA..
G_SELECTION(COMPOSE0_PADDED)
AAA.
G_SELECTION(COMPOSE1)
..B.
G_SELECTION(COMPOSE1_PADDED)
BBB.
S_SELECTION(CROP2 = C) - D is activated implicitly
..
.AA.B.
.C.DD. -- AAB.
.. CDD.
..
G_SELECTION(COMPOSE0_PADDED)
AAA.
AAA.
G_SELECTION(COMPOSE2)
C...
G_SELECTION(COMPOSE2_PADDED)
CCC.
CCC.
G_SELECTION(COMPOSE3)
.DD.
G_SELECTION(COMPOSE3_PADDED)
DDD.
DDD.
One may argue that all this logic is unnecessary after adding support
for multirect selections.
So, I kindly ask what should happen if someone call S_SELECTION
(in multirect mode) passing THREE rectangles A, B, and C (not D) ?
The driver must adjust rectangles to some valid value. So it can
increase width of C or implicitly activate D or disable C.
I think that the best solution is activating D because
it allows to set size of C to the value closest to requested one.
Therefore logic for implicit activation of D should be implemented anyway.
It makes much more sense to set everything up at once.
I agree that it is better to set everything at once.
But I strongly believe that transactions are the proper way to achieve that.
Not multirectangle selections.
It obfuscates API. It only pretends to fix a problem with applying
a part of configuration atomically.
BTW, what probably wasn't clear from Ricardo's explanation is that for every
crop rectangle you must have a corresponding compose rectangle so that you
know where to DMA it to.
Your next question will be that it is a real problem that you can't set
crop and compose simultaneously, and you are right about that. Read on for
that... :-)
Hmm. I think that the real problem is much different.
Not how to set up rectangles atomically but rather
how do anything non-trivial atomically in V4L2.
It would be very nice to have crop/compose and format configured at the same
time.
However, current version of V4L2 API does not support that.
Setting multiple crop rectangles may help a bit for only this little case.
But how would like to set multicrop rectangles and multicompose rectangle
atomically.
Why can't we extend the selection API a bit? How about this:
#define V4L2_SEL_TGT_CROP_COMPOSE0x0200
struct v4l2_selection {
__u32 type;
Re: [RFC v3] [RFC] v4l2: Support for multiple selections
Hi Ricardo, Sorry for a late reply. I've been 'offline' for the last two weeks. Please refer to the comments below. On 10/28/2013 11:46 PM, Ricardo Ribalda Delgado wrote: Hello Tomasz Sorry for the late reply, but I have been offline the last week due to the conference. On Thu, Oct 24, 2013 at 12:31 PM, Tomasz Stanislawski t.stanisl...@samsung.com wrote: Hi Ricardo, I am the designer of selection API. I hope I can help you a little. I think that there are two issues mixed in 'Mulitple selections' topic. Firstly, you described that you program a piece of hardware that is capable of selecting 8 areas for scanning. Now you are looking for userspace API to support such a feature. The feature of posting multiple rectangle was proposed in this RFC. Secondly, You introduced struct v4l2_ext_rect which is a future-proof version of v4l2_rect. I think that both issues should be solved in two separate patchsets. Ad 1. The selection of multiple scanning areas is a very driver-specific feature, isn't it? I think that you do not need to introduce any abstract interface. What would be other applications of the proposed interface? It is not driver specific. There are many sensors out there that supports multiple window of interest, but today we are ignoring them just because we dont have an api. The main application would be industrial imaging, where less data to read means more fps and therefore the system can run faster. From my field I can tell you that it is a hard requirement for computer vision. And it is a feature that we cannot model through v4l2 controls. OK. So there is no need to implement this feature as a driver-specific API. It can go automatically to generic API. Do you know other drivers that may need it? Sakari mentioned introduction of private targets for selections. I like this idea. Just define: #define V4L2_SEL_TGT_PRIVATE 0x8000 All targets that are = V4L2_SEL_TGT_PRIVATE are driver-specific. Generic applications must not use them. Non-generic application must check out the driver of video node before using selections from private set. If some target becomes more useful and accepted by more then one driver then it can be moved to generic API. The good thing about private target is that enums from different drivers can collide so the target space is not going to be trashed. If you read the previous RFCs you will see that the approach you are mentioning has been rejected. The main issue is that you cannot set atomically all the rectangles. Lets say that the configuration formed by rectangle A, B and C is legal, but the configuration A and B is not allowed by the sensor. How could you set the rectangles one by one? As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on. So activation of rectangle B (setting height to non-zero value) will enable rectangle C with some default size. Moreover disabling rectangle B (setting height to 0) may disable rectangle C automatically. I do not follow what is the problem here? Hmm. I think that the real problem is much different. Not how to set up rectangles atomically but rather how do anything non-trivial atomically in V4L2. It would be very nice to have crop/compose and format configured at the same time. However, current version of V4L2 API does not support that. Setting multiple crop rectangles may help a bit for only this little case. But how would like to set multicrop rectangles and multicompose rectangle atomically. How to define which crop rectangle refers to which to which compose rectangle. What to do if one would like to change only 3rd crop rectangle? Introduce rectangle id into v4l2_ext_rect? Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION? Is it really scalable? Multirectangle targets may seam to be a solution but I think it is not. I think that atomic transactions are what is really needed in V4L2. Other ideas like try-context from subdev API may also help. It will be nice to have something like VIDIOC_BEGIN VIDIOC_S_SELECTION (target = CROP) VIDIOC_S_SELECTION (target = COMPOSE) VIDIOC_S_FMT VIDIOC_S_CTRL VIDIOC_COMMIT and call a bunch of VIDIOC_G_* to see what really was applied. I have an auxiliary question. Do you have to set all rectangles at once? can you set up them one by one? Also if you tell the driver what exact configuration you will need, it will provide you with the closest possible confuration, that cannot be s/cannot be done/may not be 'doable' done if you provide rectangle by rectangle. But how to deal with multiple rectangles? Multiple rectangles is a desired feature, please take a look to the presentation on the workshop. I agree that it may be useful. I just think that multirectangle selections are needed to add support for such a feature. Anyway, first try to define something like this: #define V4L2_SEL_TGT_XXX_SCANOUT0
Re: [RFC v3] [RFC] v4l2: Support for multiple selections
Hi Tomasz, (Please also see the notes from the Media summit Hans posted.) Tomasz Stanislawski wrote: Hi Ricardo, I am the designer of selection API. I hope I can help you a little. I think that there are two issues mixed in 'Mulitple selections' topic. Firstly, you described that you program a piece of hardware that is capable of selecting 8 areas for scanning. Now you are looking for userspace API to support such a feature. The feature of posting multiple rectangle was proposed in this RFC. Secondly, You introduced struct v4l2_ext_rect which is a future-proof version of v4l2_rect. I think that both issues should be solved in two separate patchsets. Ad 1. The selection of multiple scanning areas is a very driver-specific feature, isn't it? I think that you do not need to introduce any abstract interface. What would be other applications of the proposed interface? Do you know other drivers that may need it? Sakari mentioned introduction of private targets for selections. I like this idea. Just define: #define V4L2_SEL_TGT_PRIVATE 0x8000 I think a private target would definitely make sense if this was functionality that was present on a single sensor or two --- that's what I actually proposed for this originally. But as far as I understand, it is more common but just not present on sensors used in mobile devices. So standardising this makes sense. All targets that are = V4L2_SEL_TGT_PRIVATE are driver-specific. Generic applications must not use them. Non-generic application must check out the driver of video node before using selections from private set. If some target becomes more useful and accepted by more then one driver then it can be moved to generic API. The good thing about private target is that enums from different drivers can collide so the target space is not going to be trashed. But how to deal with multiple rectangles? I have an auxiliary question. Do you have to set all rectangles at once? can you set up them one by one? Anyway, first try to define something like this: #define V4L2_SEL_TGT_XXX_SCANOUT0 V4L2_SEL_TGT_PRIVATE #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT (V4L2_SEL_TGT_XXX_SCANOUT0 + 1) #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS (V4L2_SEL_TGT_XXX_SCANOUT0 + 2) #define V4L2_SEL_TGT_XXX_SCANOUT0 (V4L2_SEL_TGT_PRIVATE + 16) ... -- OR-- parametrized macros similar to one below: #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n)) The application could setup all scanout areas one-by-one. By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array. The height of all consecutive area would be 0. This means disabling them effectively. The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive rectangle by shifting them down or resetting them to height 0. Notice that as long as targets are driver specific you are free do define interaction between the targets. I hope that proposed solution is satisfactory. BTW. I think that the HW trick you described is not cropping. By cropping you select which part of sensor area is going to be processed into compose rectangle in a buffer. So technicaly you still insert the whole sensor area into the buffer. Only part of the buffer is actually updated. So there is no cropping (cropping area is equal to the whole array). Notice, that every 'cropping' area goes to different part of a buffer. So you would need also muliple targets for composing (!!!) and very long chapter in V4L2 doc describing interactions between muliple-rectangle crops and compositions. Good luck !!! :). Multiple crop rectangles shouldn't make that more difficult than it was since for the next step the image size is still a rectangle (it is just composed of several smaller ones). Drivers supporting this will suffer, though, but others shouldn't need to care. The documentation must thus also be changed to say that the crop rectangles must together form a rectangle if multiple rectangle support is added. I reckon Ricardo is looking forward to using this on V4L2 interface, but I think support should also be added to the V4L2 sub-device interface. Currently it is a hell to understand and define interaction between single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness. I strongly recommend to use private selections. It will be much simpler to define, implement, and modify if needed. BTW2. I think that the mulitple scanout areas can be modelled using media device API. Sakari may know how to do this. The media device plays no part in subsystem specific matters such as formats. This is relevant in the scope of V4L2 only, IMHO. -- Kind regards, Sakari Ailus sakari.ai...@iki.fi -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v3] [RFC] v4l2: Support for multiple selections
Hello Tomasz Sorry for the late reply, but I have been offline the last week due to the conference. On Thu, Oct 24, 2013 at 12:31 PM, Tomasz Stanislawski t.stanisl...@samsung.com wrote: Hi Ricardo, I am the designer of selection API. I hope I can help you a little. I think that there are two issues mixed in 'Mulitple selections' topic. Firstly, you described that you program a piece of hardware that is capable of selecting 8 areas for scanning. Now you are looking for userspace API to support such a feature. The feature of posting multiple rectangle was proposed in this RFC. Secondly, You introduced struct v4l2_ext_rect which is a future-proof version of v4l2_rect. I think that both issues should be solved in two separate patchsets. Ad 1. The selection of multiple scanning areas is a very driver-specific feature, isn't it? I think that you do not need to introduce any abstract interface. What would be other applications of the proposed interface? It is not driver specific. There are many sensors out there that supports multiple window of interest, but today we are ignoring them just because we dont have an api. The main application would be industrial imaging, where less data to read means more fps and therefore the system can run faster. From my field I can tell you that it is a hard requirement for computer vision. And it is a feature that we cannot model through v4l2 controls. Do you know other drivers that may need it? Sakari mentioned introduction of private targets for selections. I like this idea. Just define: #define V4L2_SEL_TGT_PRIVATE 0x8000 All targets that are = V4L2_SEL_TGT_PRIVATE are driver-specific. Generic applications must not use them. Non-generic application must check out the driver of video node before using selections from private set. If some target becomes more useful and accepted by more then one driver then it can be moved to generic API. The good thing about private target is that enums from different drivers can collide so the target space is not going to be trashed. If you read the previous RFCs you will see that the approach you are mentioning has been rejected. The main issue is that you cannot set atomically all the rectangles. Lets say that the configuration formed by rectangle A, B and C is legal, but the configuration A and B is not allowed by the sensor. How could you set the rectangles one by one? I have an auxiliary question. Do you have to set all rectangles at once? can you set up them one by one? Also if you tell the driver what exact configuration you will need, it will provide you with the closest possible confuration, that cannot be done if you provide rectangle by rectangle. But how to deal with multiple rectangles? Multiple rectangles is a desired feature, please take a look to the presentation on the workshop. Anyway, first try to define something like this: #define V4L2_SEL_TGT_XXX_SCANOUT0 V4L2_SEL_TGT_PRIVATE #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT (V4L2_SEL_TGT_XXX_SCANOUT0 + 1) #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS (V4L2_SEL_TGT_XXX_SCANOUT0 + 2) #define V4L2_SEL_TGT_XXX_SCANOUT0 (V4L2_SEL_TGT_PRIVATE + 16) ... -- OR-- parametrized macros similar to one below: #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n)) The application could setup all scanout areas one-by-one. By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array. The height of all consecutive area would be 0. This means disabling them effectively. Lets say rectangle A + B + C +D is legal, and A +B is also legal. You are in ABCD and you want to go to AB. How can you do it? If yo dissable C or D, the configuration is ilegal and therefor the driver will return -EINVAL. So once you are in ABCD you cannot go back... The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive rectangle by shifting them down or resetting them to height 0. Notice that as long as targets are driver specific you are free do define interaction between the targets. I hope that proposed solution is satisfactory. As stated before, please follow the previous comments on the rfc, specially the ones from Hans. BTW. I think that the HW trick you described is not cropping. By cropping you select which part of sensor area is going to be processed into compose rectangle in a buffer. You are selecting part of the sensor, therefore you are cropping the image. So technicaly you still insert the whole sensor area into the buffer. Only the lines/columns are read into the buffer. Only part of the buffer is actually updated. So there is no cropping (cropping area is equal to the whole array). Notice, that every 'cropping' area goes to different part of a buffer. So you would need also muliple targets for composing (!!!) and very long chapter in V4L2 doc describing interactions between muliple-rectangle crops and compositions. Good luck !!! :). It is not that difficult to
Re: [RFC v3] [RFC] v4l2: Support for multiple selections
Hi Ricardo, I am the designer of selection API. I hope I can help you a little. I think that there are two issues mixed in 'Mulitple selections' topic. Firstly, you described that you program a piece of hardware that is capable of selecting 8 areas for scanning. Now you are looking for userspace API to support such a feature. The feature of posting multiple rectangle was proposed in this RFC. Secondly, You introduced struct v4l2_ext_rect which is a future-proof version of v4l2_rect. I think that both issues should be solved in two separate patchsets. Ad 1. The selection of multiple scanning areas is a very driver-specific feature, isn't it? I think that you do not need to introduce any abstract interface. What would be other applications of the proposed interface? Do you know other drivers that may need it? Sakari mentioned introduction of private targets for selections. I like this idea. Just define: #define V4L2_SEL_TGT_PRIVATE 0x8000 All targets that are = V4L2_SEL_TGT_PRIVATE are driver-specific. Generic applications must not use them. Non-generic application must check out the driver of video node before using selections from private set. If some target becomes more useful and accepted by more then one driver then it can be moved to generic API. The good thing about private target is that enums from different drivers can collide so the target space is not going to be trashed. But how to deal with multiple rectangles? I have an auxiliary question. Do you have to set all rectangles at once? can you set up them one by one? Anyway, first try to define something like this: #define V4L2_SEL_TGT_XXX_SCANOUT0 V4L2_SEL_TGT_PRIVATE #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT (V4L2_SEL_TGT_XXX_SCANOUT0 + 1) #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS (V4L2_SEL_TGT_XXX_SCANOUT0 + 2) #define V4L2_SEL_TGT_XXX_SCANOUT0 (V4L2_SEL_TGT_PRIVATE + 16) ... -- OR-- parametrized macros similar to one below: #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n)) The application could setup all scanout areas one-by-one. By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array. The height of all consecutive area would be 0. This means disabling them effectively. The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive rectangle by shifting them down or resetting them to height 0. Notice that as long as targets are driver specific you are free do define interaction between the targets. I hope that proposed solution is satisfactory. BTW. I think that the HW trick you described is not cropping. By cropping you select which part of sensor area is going to be processed into compose rectangle in a buffer. So technicaly you still insert the whole sensor area into the buffer. Only part of the buffer is actually updated. So there is no cropping (cropping area is equal to the whole array). Notice, that every 'cropping' area goes to different part of a buffer. So you would need also muliple targets for composing (!!!) and very long chapter in V4L2 doc describing interactions between muliple-rectangle crops and compositions. Good luck !!! :). Currently it is a hell to understand and define interaction between single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness. I strongly recommend to use private selections. It will be much simpler to define, implement, and modify if needed. BTW2. I think that the mulitple scanout areas can be modelled using media device API. Sakari may know how to do this. Ad 2. Extended rectangles. It is a good idea because v4l2_rect lacks any place for extensions. But before adding it to V4L2 API, I would like to know the examples of actual applications. Please, point drivers that actually need it. Other thing worth mentioning is reservation of few bits from v4l2_selection::flags to describe the type of data used for rectangle, v4l2_rect or v4l2_ext_rect. This way one can avoid introducting v4l2_selection::rectangles field. I hope you find this comments useful. Regards, Tomasz Stanislawski -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v3] [RFC] v4l2: Support for multiple selections
Ping?
Any comment about this version? Shall I post a new one with the
modified device drivers?
Thanks!
On Tue, Oct 1, 2013 at 12:33 PM, Ricardo Ribalda Delgado
ricardo.riba...@gmail.com wrote:
Extend v4l2 selection API to support multiple selection areas, this way
sensors that support multiple readout areas can work with multiple areas
of insterest.
The struct v4l2_selection and v4l2_subdev_selection has been extented
with a new field rectangles. If it is value is different than zero the
pr array is used instead of the r field.
A new structure v4l2_ext_rect has been defined, containing 4 reserved
fields for future improvements, as suggested by Hans.
Two helper functiona are also added, to help the drivers that support
a single selection.
This Patch ONLY adds the modification to the core. Once it is agreed, a
new version including changes on the drivers that handle the selection
api will come.
This patch includes all the comments by Hans Verkuil.
Signed-off-by: Ricardo Ribalda Delgado ricardo.riba...@gmail.com
---
v3:
-Changes on compat-ioctl32
-Remove checks on v4l2_selection_set_rect
drivers/media/v4l2-core/v4l2-common.c | 36 +++
drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 63
+++
drivers/media/v4l2-core/v4l2-ioctl.c | 37 +---
include/media/v4l2-common.h | 6 +++
include/uapi/linux/v4l2-subdev.h | 10 -
include/uapi/linux/videodev2.h| 21 +++--
6 files changed, 162 insertions(+), 11 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-common.c
b/drivers/media/v4l2-core/v4l2-common.c
index a95e5e2..f60a2ce 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -886,3 +886,39 @@ void v4l2_get_timestamp(struct timeval *tv)
tv-tv_usec = ts.tv_nsec / NSEC_PER_USEC;
}
EXPORT_SYMBOL_GPL(v4l2_get_timestamp);
+
+int v4l2_selection_get_rect(const struct v4l2_selection *s,
+ struct v4l2_ext_rect *r)
+{
+ if (s-rectangles 1)
+ return -EINVAL;
+ if (s-rectangles == 1) {
+ *r = s-pr[0];
+ return 0;
+ }
+ if (s-r.width 0 || s-r.height 0)
+ return -EINVAL;
+ r-left = s-r.left;
+ r-top = s-r.top;
+ r-width = s-r.width;
+ r-height = s-r.height;
+ memset(r-reserved, 0, sizeof(r-reserved));
+ return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_selection_get_rect);
+
+void v4l2_selection_set_rect(struct v4l2_selection *s,
+ const struct v4l2_ext_rect *r)
+{
+ if (s-rectangles == 0) {
+ s-r.left = r-left;
+ s-r.top = r-top;
+ s-r.width = r-width;
+ s-r.height = r-height;
+ return;
+ }
+ s-pr[0] = *r;
+ s-rectangles = 1;
+ return;
+}
+EXPORT_SYMBOL_GPL(v4l2_selection_set_rect);
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 8f7a6a4..36ed3c3 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -777,6 +777,54 @@ static int put_v4l2_subdev_edid32(struct
v4l2_subdev_edid *kp, struct v4l2_subde
return 0;
}
+struct v4l2_selection32 {
+ __u32 type;
+ __u32 target;
+ __u32 flags;
+ union {
+ struct v4l2_rectr;
+ compat_uptr_t *pr;
+ };
+ __u32 rectangles;
+ __u32 reserved[8];
+} __packed;
+
+static int get_v4l2_selection32(struct v4l2_selection *kp,
+ struct v4l2_selection32 __user *up)
+{
+ if (!access_ok(VERIFY_READ, up, sizeof(struct v4l2_selection32))
+ || (copy_from_user(kp, up, sizeof(*kp
+ return -EFAULT;
+
+ if (kp-rectangles) {
+ compat_uptr_t usr_ptr;
+ if (get_user(usr_ptr, up-pr))
+ return -EFAULT;
+ kp-pr = compat_ptr(usr_ptr);
+ }
+
+ return 0;
+
+}
+
+static int put_v4l2_selection32(struct v4l2_selection *kp,
+ struct v4l2_selection32 __user *up)
+{
+ compat_uptr_t usr_ptr = 0;
+
+ if ((kp-rectangles) get_user(usr_ptr, up-pr))
+ return -EFAULT;
+
+ if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_selection32))
+ || (copy_to_user(kp, up, sizeof(*kp
+ return -EFAULT;
+
+ if (kp-rectangles)
+ put_user(usr_ptr, up-pr);
+
+ return 0;
+
+}
#define VIDIOC_G_FMT32 _IOWR('V', 4, struct v4l2_format32)
#define
[RFC v3] [RFC] v4l2: Support for multiple selections
Extend v4l2 selection API to support multiple selection areas, this way
sensors that support multiple readout areas can work with multiple areas
of insterest.
The struct v4l2_selection and v4l2_subdev_selection has been extented
with a new field rectangles. If it is value is different than zero the
pr array is used instead of the r field.
A new structure v4l2_ext_rect has been defined, containing 4 reserved
fields for future improvements, as suggested by Hans.
Two helper functiona are also added, to help the drivers that support
a single selection.
This Patch ONLY adds the modification to the core. Once it is agreed, a
new version including changes on the drivers that handle the selection
api will come.
This patch includes all the comments by Hans Verkuil.
Signed-off-by: Ricardo Ribalda Delgado ricardo.riba...@gmail.com
---
v3:
-Changes on compat-ioctl32
-Remove checks on v4l2_selection_set_rect
drivers/media/v4l2-core/v4l2-common.c | 36 +++
drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 63 +++
drivers/media/v4l2-core/v4l2-ioctl.c | 37 +---
include/media/v4l2-common.h | 6 +++
include/uapi/linux/v4l2-subdev.h | 10 -
include/uapi/linux/videodev2.h| 21 +++--
6 files changed, 162 insertions(+), 11 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-common.c
b/drivers/media/v4l2-core/v4l2-common.c
index a95e5e2..f60a2ce 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -886,3 +886,39 @@ void v4l2_get_timestamp(struct timeval *tv)
tv-tv_usec = ts.tv_nsec / NSEC_PER_USEC;
}
EXPORT_SYMBOL_GPL(v4l2_get_timestamp);
+
+int v4l2_selection_get_rect(const struct v4l2_selection *s,
+ struct v4l2_ext_rect *r)
+{
+ if (s-rectangles 1)
+ return -EINVAL;
+ if (s-rectangles == 1) {
+ *r = s-pr[0];
+ return 0;
+ }
+ if (s-r.width 0 || s-r.height 0)
+ return -EINVAL;
+ r-left = s-r.left;
+ r-top = s-r.top;
+ r-width = s-r.width;
+ r-height = s-r.height;
+ memset(r-reserved, 0, sizeof(r-reserved));
+ return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_selection_get_rect);
+
+void v4l2_selection_set_rect(struct v4l2_selection *s,
+ const struct v4l2_ext_rect *r)
+{
+ if (s-rectangles == 0) {
+ s-r.left = r-left;
+ s-r.top = r-top;
+ s-r.width = r-width;
+ s-r.height = r-height;
+ return;
+ }
+ s-pr[0] = *r;
+ s-rectangles = 1;
+ return;
+}
+EXPORT_SYMBOL_GPL(v4l2_selection_set_rect);
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 8f7a6a4..36ed3c3 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -777,6 +777,54 @@ static int put_v4l2_subdev_edid32(struct v4l2_subdev_edid
*kp, struct v4l2_subde
return 0;
}
+struct v4l2_selection32 {
+ __u32 type;
+ __u32 target;
+ __u32 flags;
+ union {
+ struct v4l2_rectr;
+ compat_uptr_t *pr;
+ };
+ __u32 rectangles;
+ __u32 reserved[8];
+} __packed;
+
+static int get_v4l2_selection32(struct v4l2_selection *kp,
+ struct v4l2_selection32 __user *up)
+{
+ if (!access_ok(VERIFY_READ, up, sizeof(struct v4l2_selection32))
+ || (copy_from_user(kp, up, sizeof(*kp
+ return -EFAULT;
+
+ if (kp-rectangles) {
+ compat_uptr_t usr_ptr;
+ if (get_user(usr_ptr, up-pr))
+ return -EFAULT;
+ kp-pr = compat_ptr(usr_ptr);
+ }
+
+ return 0;
+
+}
+
+static int put_v4l2_selection32(struct v4l2_selection *kp,
+ struct v4l2_selection32 __user *up)
+{
+ compat_uptr_t usr_ptr = 0;
+
+ if ((kp-rectangles) get_user(usr_ptr, up-pr))
+ return -EFAULT;
+
+ if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_selection32))
+ || (copy_to_user(kp, up, sizeof(*kp
+ return -EFAULT;
+
+ if (kp-rectangles)
+ put_user(usr_ptr, up-pr);
+
+ return 0;
+
+}
#define VIDIOC_G_FMT32 _IOWR('V', 4, struct v4l2_format32)
#define VIDIOC_S_FMT32 _IOWR('V', 5, struct v4l2_format32)
@@ -796,6 +844,8 @@ static int put_v4l2_subdev_edid32(struct v4l2_subdev_edid
*kp, struct v4l2_subde
#defineVIDIOC_DQEVENT32_IOR ('V', 89, struct v4l2_event32)
#define VIDIOC_CREATE_BUFS32 _IOWR('V', 92, struct v4l2_create_buffers32)
#define
