search

alliedvision / gst-vimbasrc

Official vimbasrc element for use of Vimba with GStreamer
Other
10 stars 9 forks source link

Automatically set OffsetX/OffsetY to position the RoI in the centre of the sensor #8

Closed EdTorbett closed 2 years ago

EdTorbett commented 2 years ago

When a width/height less than the maximum sensor size is specified, it's not automatically centred in the sensor but placed at offset 0,0. This means that the centre of the returned images do not correspond with the optical sensor of the lens/camera.
In order to centre it, it would be necessary to obtain the sensor width and height which the plugin does not expose as parameters. Instead, the existing approach used for width/height is copied, and a value of INT_MAX is interpreted as 'centre the RoI within the sensor for this axis'. I have made the assumption that, like width/height, this behaviour should be the default.

NiklasKroeger-AlliedVision commented 2 years ago

Thank you very much for the suggestion and the effort you put into this! It is very much appreciated.

I will need some time to look into this change in behaviour, as changes like this might impact existing uses of the element itself and could lead to issues of users that do not expect this.

EdTorbett commented 2 years ago

Hi @NiklasKroeger-AlliedVision, I agree that this could break some backwards compatibility, in which case I believe the only change required to return it to the original behaviour would be to set the defaults for both OffsetX and OffsetY props back to 0.

As a heads up, I am making a number of additional changes to our fork in order to support changing parameters (in particular the maximum auto exposure time limit) while aquisition is active. As part of this I've removed some boilerplate and fixed a couple of bugs which I can submit PRs for separately as well if you want.

NiklasKroeger-AlliedVision commented 2 years ago

I believe the only change required to return it to the original behaviour would be to set the defaults for both OffsetX and OffsetY props back to 0

Than we would need some magic number for OffsetX and OffsetY that again enables the automatic centring of the ROI. Thinking out loud perhaps a -1 would make sense for that instead of relying on G_MAXINT to detect the special case since a negative offset would in my mind never really make sense as an actual value. But again, that is something we would also need to discuss internally.

As a heads up, I am making a number of additional changes to our fork in order to support changing parameters (in particular the maximum auto exposure time limit) while aquisition is active. As part of this I've removed some boilerplate and fixed a couple of bugs which I can submit PRs for separately as well if you want.

I actually noticed that you had some additional changes in your fork. Thanks for offering this. We are happy about any suggestion for improvements on our open source projects. So if you feel comfortable sharing them with us feel free to open pull requests for those as well.

EdTorbett commented 2 years ago

Than we would need some magic number for OffsetX and OffsetY that again enables the automatic centring of the ROI. Thinking out loud perhaps a -1 would make sense for that instead of relying on G_MAXINT to detect the special case since a negative offset would in my mind never really make sense as an actual value. But again, that is something we would also need to discuss internally.

I would argue that G_MAXINT serves this purpose well enough as 1) It's far larger than any sensor will ever be and 2) It's already used as the magic number for Width/Height. However, I leave this decision entirely up to you!

So if you feel comfortable sharing them with us feel free to open pull requests for those as well.

Given it's a GPL project I'm perfectly happy to do this. Chances are you'll have to independently decide what you wish to bring in as my changes will be focussed quite specifically on my use case, but again I'll leave that decision in your hands.

NiklasKroeger-AlliedVision commented 2 years ago

I would argue that G_MAXINT serves this purpose well enough as 1) It's far larger than any sensor will ever be and 2) It's already used as the magic number for Width/Height. However, I leave this decision entirely up to you!

My thinking of -1 was more along the lines that if we return to setting the default value to 0, triggering the auto-centred ROI would require the user to add OffsetX=2147483647 which seems harder to remember than passing OffsetX=-1. Or is it possible to do something like OffsetX=G_MAXINT as parameter to the element? Still would seem like an odd parameter to me.... Maybe I need to play around with different values to see what makes most sense from the user perspective.

Given it's a GPL project I'm perfectly happy to do this. Chances are you'll have to independently decide what you wish to bring in as my changes will be focussed quite specifically on my use case, but again I'll leave that decision in your hands.

Just to prevent any confusion here. The license of this repository is the "GNU Library General Public License, version 2.0" (LGPLv2 - as found on the GNU website here). Technically speaking a GNU General Public License, I just want to make sure that it is not confused with the similar named "GNU General Public License" (GPL - e.g. in version 3 on the GNU website here). I hope this does not change your mind. I just want to prevent any possible confusion from the similar names in case anyone else finds this PR in the future.

EdTorbett commented 2 years ago

Just to prevent any confusion here. The license of this repository is the "GNU Library General Public License, version 2.0" (LGPLv2 - as found on the GNU website here). Technically speaking a GNU General Public License, I just want to make sure that it is not confused with the similar named "GNU General Public License" (GPL - e.g. in version 3 on the GNU website here). I hope this does not change your mind. I just want to prevent any possible confusion from the similar names in case anyone else finds this PR in the future.

Apologies for being a little ambiguous - I am aware of and happy with the terms of the license and was simply referring to the terms regarding redistribution of modified source, as they are common across LGPL and GPL.

NiklasKroeger-AlliedVision commented 2 years ago

Sorry for the long delay in getting back to this!

I gathered some internal feedback and responses on the behaviour are mixed. We came to the following conclusions:

  1. Having a way to auto-centre the ROI is definitely desirable and a great addition
  2. The ROI should not be centred by default. This would a a major difference in behaviour compared to our regular camera API and we do not want to deviate too much to prevent confusion for existing users or user who might migrate from gst-vimbasrc to Vimba. Turning on centring should be done explicitly. We currently can imagine two ways to do this:
    1. Introduce a magic number that can be set for offsetx and offsety to enable centering along that axis (basically what @EdTorbett is doing in this PR)
    2. Add a new property called something like center-roi which would accept values such as None, x, y, and both (or maybe xy)

Some thoughts on both options:

regarding 2.i

regarding 2.ii

Just writing this out I think 2.i (magic number for offsetx and offsety) should be preferred. This is already very close the the implementation @EdTorbett provided here.

@EdTorbett do you have any thoughts on this? Maybe I missed some things.

EdTorbett commented 2 years ago

@NiklasKroeger-AlliedVision took a bit before I could get back to this but these suggestions have now been applied. Thanks!