search

joshdoe / gst-plugins-vision

GStreamer plugins related to the field of machine vision
Other
134 stars 50 forks source link

pylon refactoring #42

Closed mrstecklo closed 4 years ago

mrstecklo commented 4 years ago

Made some refactoring to gstpylonsrc.c. There were lots of repetative code (e.g. color adjustments for red, green, blue is identical).

I'm improving only pylonsrc plugin and now I'm heading towards using PylonFeaturePersistenceLoad() to use configuration files to set all features. Generally it's because of not all of them work properly on both my cameras. For example one camera does not have AcquisitionFrameRate feature, but has AquisitionFrameRateAbs. Using configuration file will somewhat conflict with some features being set to default if not set explicitly. So I will use tri-state flags to determine if feature is set/notset/default. If configuration file is not used, all features will be processed in "legacy" mode for compatibility, i.e if it is not set explicitly, it is default. If configuration file is used, all features will be marked as notset if not set explicitly.

I'm also thinking of modifying CMakeLists to be able to specify GStreamer install prefix manually, because I've installed GStreamer with gst-build and it does not set any environment variables and pkg-config knows nothing about installation.

mrstecklo commented 4 years ago

I'm also confused with this "common/genicampixelformat.h" All functions are defined in header with static qualifier. Is there any reason for this?

joshdoe commented 4 years ago

I'm also confused with this "common/genicampixelformat.h" All functions are defined in header with static qualifier. Is there any reason for this?

These functions used to be in the niimaqdx or pleora plugin, then I pulled them out since they were being used by multiple plugins. The static qualifiers I think were left by accident, though I think in a header file it doesn't matter one way or the other in most cases.

joshdoe commented 4 years ago

If you don't have the GStreamer installer environment variable setup, you can specify GSTREAMER_ROOT to CMake.

joshdoe commented 4 years ago

Overall these changes make sense, I'll review and test tomorrow. In one of my applications this plugin was failing to start correctly, I think because the app actively sets each property to it's default, some of which fail, at least with my cameras. Some of the defaults don't make much sense to me, so I had been planning to look at properties, hopefully your changes are fixing this behavior.

mrstecklo commented 4 years ago

If you don't have the GStreamer installer environment variable setup, you can specify GSTREAMER_ROOT to CMake.

Thanks, I'll try it

The static qualifiers I think were left by accident, though I think in a header file it doesn't matter one way or the other in most cases.

It is not very bad, but leads to static linkage. So new instance of each function is created for every inclusion of header

mrstecklo commented 4 years ago

Grouped separate enumerations for gain/exposure limits and manual values. Think this was over complicated

joshdoe commented 4 years ago

@mrstecklo I've been making a habit of running gst-indent over code, actually I have it as a pre-commit hook. I just ran it, and there are lots of formatting fixups needed. Since you already have some changes on top of this, I'll let you finish those changes, but then you or I should run gst-indent.

mrstecklo commented 4 years ago

OK, I'll do it before opening a new pull request