-
-
Notifications
You must be signed in to change notification settings - Fork 55.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixing cap_pvpapi interface #3322
fixing cap_pvpapi interface #3322
Conversation
Okay for the person that will look into/review this, please take a look here: https://github.com/StevenPuttemans/opencv_tryout_code/blob/master/camera_interfacing/AVT_manta_camera.cpp This is a working interface with an AVT Manta Camera. However, if I look at the OpenCV implementation I find many weird constructions that assure the building will never work. Check my remark at http://code.opencv.org/issues/3947 I think we should use this PR to fix as much as possible |
@asmorkalov I took some time to debug some stuff, basically this capture implementation is completely wrong. It has hardcoded maximum resolution sizes which is just completely wrong and gives very bad results on all camera's that were not used by the original programmer. I will see if I can make it way more universal and update this PR to include all neccesary fixes. |
@@ -146,14 +146,13 @@ bool CvCaptureCAM_PvAPI::open( int index ) | |||
tPvCameraInfo camInfo; | |||
tPvIpSettings ipSettings; | |||
|
|||
|
|||
if (PvInitialize()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is error 2, this should be !PvInitialize(). The function returns 0 when initialization is busy and thus waiting untill a camera is up should depend on it being 0. Will update this already and some other snippets.
👍 |
New discussion, the suggested approach of mono8 being monochrome false and mono16 being monochrome true is incorrect. I have a monochromo manta camera, which returns mono8 as type. I know the data is stored as a bayer pattern for the proscilla series, so afaik the true and false should be switched right? |
Second discussion: I managed to completely reconfigure the initialization of the camera interface to also support the Manta series of AVT through this API. However, we should have an option to indicate in a VideoCapture if it is a manta or a proscilla series. This is important when configuring the camera settings, Proscilla has more settings then manta. Suggestion, doing something like VideoCapture(0 | PVAPI_PROSCILLA) Could be enough, but I don't really have an idea on how to approach this. |
//PvAttrUint32Get(Camera.Handle,"PacketSize",&maxSize); | ||
if (PvCaptureAdjustPacketSize(Camera.Handle,maxSize)!=ePvErrSuccess) | ||
return false; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusting the package size with a static value is dangerous, not all systems and ethernet cards support this. This is mainly done to avoid overhead, probably by the original author. It will work on some systems but fail miserably on others. Just let the standard package sizes be used and ignore the overhead it creates.
@ all readers, do check the remarks I left. |
First big update
I hope someone with a Proscilla camera can find the time to see if it still works for them. |
Unlike Manta, Prosilica does not generate a device name that includes the string "Prosilica". // Define if we have a PROSCILLA or MANTA GigE camera series bool PVAPI_MANTA = false; strtok(camInfo.DisplayName, "_"); if (strcmp(camInfo.DisplayName, "Manta") == 0){ PVAPI_MANTA = true; } Now whenever you see PVAPI_PROSILCA, change it to !PVAPI_MANTA. Here's the complete listing that deals with this: // Define if we have a PROSCILLA or MANTA GigE camera series bool PVAPI_MANTA = false; strtok(camInfo.DisplayName, "_"); if (strcmp(camInfo.DisplayName, "Manta") == 0){ PVAPI_MANTA = true; } tPvUint32 frameWidth, frameHeight, frameSize; char pixelFormat[256]; PvAttrUint32Get(Camera.Handle, "TotalBytesPerFrame", &frameSize); PvAttrUint32Get(Camera.Handle, "Width", &frameWidth); PvAttrUint32Get(Camera.Handle, "Height", &frameHeight); PvAttrEnumGet(Camera.Handle, "PixelFormat", pixelFormat,256,NULL); if(!PVAPI_MANTA){ // Retrieve the maximum possible packet size from the camera // Then adjust the packet size to that value unsigned long maxSize; PvAttrUint32Get(Camera.Handle,"PacketSize",&maxSize); if(PvCaptureAdjustPacketSize(Camera.Handle,maxSize)!=ePvErrSuccess){ return false; } } if(PVAPI_MANTA){ // Explicitly set the width and height correctly since camera can remember configurations PvAttrUint32Set(Camera.Handle, "RegionX", 0 ); PvAttrUint32Set(Camera.Handle, "RegionY", 0 ); PvAttrUint32Set(Camera.Handle, "Width", frameWidth); PvAttrUint32Set(Camera.Handle, "Height", frameHeight); } // Start the camera PvCaptureStart(Camera.Handle); if(!PVAPI_MANTA){ // Set the camera explicitly to capture data frames continuously if(PvAttrEnumSet(Camera.Handle, "AcquisitionMode", "Continuous")!= ePvErrSuccess) { fprintf(stderr,"Could not set Prosilica/Manta Acquisition Mode\n"); return false; } } if(PvCommandRun(Camera.Handle, "AcquisitionStart")!= ePvErrSuccess) { fprintf(stderr,"Could not start Prosilica/Manta acquisition\n"); return false; } if(PvAttrEnumSet(Camera.Handle, "FrameStartTriggerMode", "Freerun")!= ePvErrSuccess) { fprintf(stderr,"Error setting Prosilica/Manta trigger to \"Freerun\""); return false; } if (!PVAPI_MANTA){ // Settings depending on the pixelformat if (strcmp(pixelFormat, "Mono8")==0) { monocrome = true; grayframe = cvCreateImage(cvSize((int)frameWidth, (int)frameHeight), IPL_DEPTH_8U, 1); grayframe->widthStep = (int)frameWidth; frame = cvCreateImage(cvSize((int)frameWidth, (int)frameHeight), IPL_DEPTH_8U, 3); frame->widthStep = (int)frameWidth*3; Camera.Frame.ImageBufferSize = frameSize; Camera.Frame.ImageBuffer = grayframe->imageData; } else if (strcmp(pixelFormat, "Mono16")==0) { monocrome = true; grayframe = cvCreateImage(cvSize((int)frameWidth, (int)frameHeight), IPL_DEPTH_16U, 1); grayframe->widthStep = (int)frameWidth; frame = cvCreateImage(cvSize((int)frameWidth, (int)frameHeight), IPL_DEPTH_16U, 3); frame->widthStep = (int)frameWidth*3; Camera.Frame.ImageBufferSize = frameSize; Camera.Frame.ImageBuffer = grayframe->imageData; } else if (strcmp(pixelFormat, "Bgr24")==0) { monocrome = false; frame = cvCreateImage(cvSize((int)frameWidth, (int)frameHeight), IPL_DEPTH_8U, 3); frame->widthStep = (int)frameWidth*3; Camera.Frame.ImageBufferSize = frameSize; Camera.Frame.ImageBuffer = frame->imageData; } else return false; } if (PVAPI_MANTA){ monocrome = true; grayframe = cvCreateImage(cvSize((int)frameWidth, (int)frameHeight), IPL_DEPTH_8U, 1); grayframe->widthStep = (int)frameWidth; Camera.Frame.ImageBufferSize = frameSize; Camera.Frame.ImageBuffer = grayframe->imageData; } return true;
|
The change that polls the number of cameras is not working for me. This is due to the wait implementation inside the while loop (this might be different on Linux and Windows). if (!PvInitialize()) { // Wait until the initialisation actually succeeded in opening up a camera // Ensure multiple cameras can be opened if preferred by grabbing the current amount before the initialisation while((PvCameraCount() != (numCameras+1)) && (initializeTimeOut--)) { cvWaitKey(10); Sleep(10); } if (initializeTimeOut == 0){ fprintf(stderr,"ERROR: camera intialisation timeout [5000ms].\n"); } numCameras=PvCameraList(cameraList, MAX_CAMERAS, NULL); } I'm not sure why you switch between PvCamerList() function and PvCameraCount(). Wouldn't it be best to use just one? They seem to be interchangeble.
|
There is also an issue when you have two cameras with two different interfaces installed. For example, I have a webcam which is a USB device and a Prosilica. The prosilica is device 1; the USB is device 0. This causes issues with indexing since the code assumes that the camera with index 1 is the second PVAPI camera (counting starts at zero). Which is wrong-- it is actually the first camera. There are several places we have to touch to fix this.
The fix is not so simple. It should also involve changing how the index variable is treated.
|
In line 265 under "mono8", monocrome flag is set to false which is wrong. It should be set to true.
|
Thanks for all your remarks! Will adapt where possible :) Some remarks / discussion
|
As to the index, I suggest we get the code running for 1 proscilla or 1 manta first; simply disconnect the others for a moment. Lets then make a new PR and bug for the multiple camera handling and indexing :) |
There was a trailing whitespace error whch I resolved. |
O forget my last remark; found the reason why this fails. I just recalled that cvWaitKey ONLY works if there is an active window available. Since this is not the case at initialization, the wait never happens. I will change it into a sleep command! |
Steven, cvWaitKey() does not generate any errors. However, on my system it does not actually wait. Hence, the code that supposedly waits for 5 seconds exits very fast without any timeout and the result is that the camera is not identified properly (the code does not wait enough time for it to initalize). If you add Sleep(10) after cvWaitKey(10) then an actual wait is performed and things work OK.
|
Yeah just discovered the same thing, since I was debugging and I moved instruction by instruction it worked. I am guessing that this would solve my problem at work also, but thats for monday. BTW you know the reason of creating a 3 channel frame image in the monochrome cases? Seems overkill... |
I would also suggest changing the strcmp() function calls to stricmp() function calls when checking for pixelformat; it is a little more robust. This is for "mono8", "mono16" and "Bgr24" strings. Here's something that's been bothering me. In case of "mono8" and "mono16", there is no need to generate a color frame. I've checked this and it seems to work OK with "mono8": if (!PVAPI_MANTA){ // Settings depending on the pixelformat if (stricmp(pixelFormat, "Mono8")==0) { monocrome = true; grayframe = cvCreateImage(cvSize((int)frameWidth, (int)frameHeight), IPL_DEPTH_8U, 1); grayframe->widthStep = (int)frameWidth; /* frame = cvCreateImage(cvSize((int)frameWidth, (int)frameHeight), IPL_DEPTH_8U, 3); frame->widthStep = (int)frameWidth*3; */ Camera.Frame.ImageBufferSize = frameSize; Camera.Frame.ImageBuffer = grayframe->imageData; } else if (stricmp(pixelFormat, "Mono16")==0) { monocrome = true; grayframe = cvCreateImage(cvSize((int)frameWidth, (int)frameHeight), IPL_DEPTH_16U, 1); grayframe->widthStep = (int)frameWidth; /* frame = cvCreateImage(cvSize((int)frameWidth, (int)frameHeight), IPL_DEPTH_16U, 3); frame->widthStep = (int)frameWidth*3; */ Camera.Frame.ImageBufferSize = frameSize; Camera.Frame.ImageBuffer = grayframe->imageData; } Notice the commenting of the lines associated with the frame variable (only grayframe variable is used).
|
That's exactly what I wrote-- no need for a 3 color frame.
|
Will change! And again, Linux Manta test on monday :) |
This is something I wanted to discuss. First of all, I don't like the use of the PVAPI_MANTA variable. I read through the code and came to a conclusion that the reason for this lies in the topic explained below: initilization of camera parameters. In my view, changing camera's parameters upon initialization, unless absolutely necessary, should not be done. The necessary parameters are: Setting the AcquisitionStart, setting the AcquisitionMode and setting the FrameStartTriggerMode. All other parameters should not be set on PvCameraOpen(). The reason is that users set the values with the GigEViewer application which comes with the camera's SDK. For example, say I want a slower bit-rate so as not to clog my bandwidth, then adjusting the maximum packet size automatically in PvCameraOpen() will not allow me to do that. The same goes for ROI. if(!PVAPI_MANTA) { // Retrieve the maximum possible packet size from the camera // Then adjust the packet size to that value unsigned long maxSize; PvAttrUint32Get(Camera.Handle,"PacketSize",&maxSize); if(PvCaptureAdjustPacketSize(Camera.Handle,maxSize)!=ePvErrSuccess){ return false; } } Should be removed. It's up to the user that configured the camera to set this value. More notes: if(!PVAPI_MANTA) { // Set the camera explicitly to capture data frames continuously if(PvAttrEnumSet(Camera.Handle, "AcquisitionMode", "Continuous")!= ePvErrSuccess) { fprintf(stderr,"Could not set Prosilica/Manta Acquisition Mode\n"); return false; } } Why is there an if(!PVAPI_MANTA) here? Shouldn't this be for all cameras? Once you remove all these initializations, there should be no reason to have the PVAPI_MANTA flag. You could merge everything to one mainline. The default acquisition mode should be set to mono8 so as to support MANTA. Thanks,
|
Two issue: a bug with the initialization process and the index issue in case several cameras of different protocols are installed on the system with PVAPI cameras as well. The problem with the initialization code is that first we issue a call to: unsigned int numCameras = PvCameraList(cameraList, MAX_CAMERAS, NULL); and only then we call PvInitialize(). The problem is that PvCameraList() populates the array variable cameraList with all the visible cameras. However, since PvInitialize() is not called before PvCameraList(), the list is just random values without any cameras because there was no camera initialization. Therefore, after a call to PvInitialize(), one must also call the PvCameraList() to properly populate the cameraList array (see below for a fix). The second issue is with the index variable that is passed as a parameter to the function. The problem is that this index is just a random number that has no meaning, since the cameras are identified with the Unique ID variable. To be able to open the camera, all that is needed is to set the Unique ID to the last value in the camerList array. This approach will ensure that even if you have a different protocol camera (for example, USB and PvAPI), the index will be properly set. The problem is if you have two PVAPI cameras. I'm not sure how this will work-- it needs to be tested. Here's the suggested code change that deals with both the index issue and the initialization process. int initializeTimeOut = 500; unsigned int numCameras = PvCameraCount(); if(PvInitialize()==ePvErrSuccess) { // Wait until the initialisation actually succeeded in opening up a camera // Ensure multiple cameras can be opened if preferred by grabbing the current amount before the initialisation while((PvCameraCount()!=(numCameras+1)) && (initializeTimeOut--)) Sleep(10); if(!initializeTimeOut) { fprintf(stderr,"ERROR: camera initialization timeout [5000ms].\n"); return false; } numCameras = PvCameraList(cameraList, MAX_CAMERAS, NULL); } else { fprintf(stderr, "ERROR: Some required system resources were not available, PvInitialize().\n"); return false; } // no cameras found if(!numCameras) { fprintf(stderr, "ERROR: No cameras found.\n"); return false; } // since we've just initialized the camera with PvInitialize(), it is the last camera on our list // notice how PvInitialize() does not care about the index and so we shouldn't as well-- it has its own // unique ID, UID. Camera.UID = cameraList[numCameras-1].UniqueId; if(PvCameraIpSettingsGet(Camera.UID,&ipSettings)==ePvErrNotFound) { fprintf(stderr, "The specified camera UID %lu could not be found, PvCameraIpSettingsGet().\n", Camera.UID); return false; } if(PvCameraInfo(Camera.UID,&camInfo)==ePvErrNotFound) { fprintf(stderr, "The specified camera UID %lu could not be found, PvCameraInfo().\n", Camera.UID); return false; } else { /* struct in_addr addr; addr.s_addr = ipSettings.CurrentIpAddress; printf("Current address:\t%s\n",inet_ntoa(addr)); addr.s_addr = ipSettings.CurrentIpSubnet; printf("Current subnet:\t\t%s\n",inet_ntoa(addr)); addr.s_addr = ipSettings.CurrentIpGateway; printf("Current gateway:\t%s\n",inet_ntoa(addr)); */ }
|
Lets fire up the discussion
As a small remark. If you plan to discuss code further, place this as remarks inside the code at the files changed tab. This will keep the discussion a bit more clear. Thanks again for your help! |
Thanks Steven, I've tried implementing my suggestion with an overloaded function but the details are too deep and will cause an overhaul of the entire VideoCapture class so for now I think I'll leave it as is. It's good enough for now. Myabe when I have some more time on my hands. While we're at it, the VideoCapture(index) mechanism should be changed to accomodate for this. Now to tackle a few more bugs/issues. They all creep up when you start hooking up more than one camera. The first issue is if you call PvInitialize() more than once, it will fail (the DLL is already open). This is probably why the original author did not check the PvInitialize() return value, and is indeed a required change. As suspected, although my initial fix was able to allow for different interface cameras (USB and PVAPI) to work well, it did not handle several PVAPI cameras. This is because PvCameraList() lists all the cameras and not just one after the other. I therefore implemented a different mechanism. The code keeps trying to open cameras in the cameraList until all the list is exhausted. This way successive calls to VideoCapture() will go through all the PVAPI cameras and you will be able to access them all. See the code that uses the variable findNewCamera for details. I've made several changes to the function bool CvCaptureCAM_PvAPI::open(int index). I've tried to leave remakrs whenever applicable. By the way, I did a dummy index++ call just so the warning message of an unsued variable will not be shown; if you know of a better way please change this. First, the change to PvInitialize(): tPvCameraInfo cameraList[MAX_CAMERAS]; tPvCameraInfo camInfo; tPvIpSettings ipSettings; index++; // to avoid a warning, we don't really use index // Initialization parameters [500 x 10 ms = 5000 ms timeout] int initializeTimeOut = 500; unsigned int numCameras = PvCameraCount(); // disregard any errors, since this might be called several times and only needs to be // called once or will return an error PvInitialize(); // this if sentence is not needed due to PvInitialize() being called without reading ther result //if(PvInitialize()==ePvErrSuccess) { // Wait until the initialisation actually succeeded in opening up a camera // Ensure multiple cameras can be opened if preferred by grabbing the current amount before the initialisation while((PvCameraCount()!=(numCameras+1)) && (initializeTimeOut--)) Sleep(10); if(!initializeTimeOut) { fprintf(stderr,"ERROR: camera initialization timeout [5000ms].\n"); return false; } numCameras = PvCameraList(cameraList, MAX_CAMERAS, NULL); } // the else branch is no longer needed /* else { fprintf(stderr, "ERROR: Some required system resources were not available, PvInitialize().\n"); return false; } */ // no cameras found if(!numCameras) { fprintf(stderr, "ERROR: No cameras found.\n"); return false; } Now the change to read the next available PVAPI camera:
Thanks,
PS, I'm not that familiar with Git and don't know how to add remarks inside the code; I'll look it up for the next time. |
Added a small sample. This can later be used to add examples of how to address the properties. Want to keep this PR as basic as possible before we move on to other fixes in this API. |
Works well with two Prosilica cameras in color and in monochrome modes. Some minor cosmetic comments on the code to follow. |
{ | ||
Camera.UID = cameraList[findNewCamera].UniqueId; | ||
if(PvCameraOpen(Camera.UID, ePvAccessMaster, &(Camera.Handle))==ePvErrSuccess){ | ||
printf("Camera found and opended with UID = %d\n", (int)Camera.UID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe change this to fprintf(stderr, "Camera found ...") instead of regular printf()?
Also a minor typo: "opened" instead of "opended"
Like I said, works very well and should be committed.
|
All minor fixes applied and fixed the warnings and build errors. |
PING! :) |
tPvCameraInfo cameraList[MAX_CAMERAS]; | ||
// In order not to break the current VideoCapture interface we want to avoid warning of index not being used | ||
// So we use a bogus operation for this | ||
index++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just remove parameter name from function header to solve warnings:
bool CvCaptureCAM_PvAPI::open( int )
It looks ok for me. Please look at my comments and I can merge it after fixes. |
Updated! |
@asmorkalov fixed it, could be best to merge it now? |
👍 |
Tss I broke it :D Let me restore :) |
series are supported. Testing this with both cams for Windows and Linux exhaustively. Optimizing memory footprint by removing unused calls. Adapted with the input of Shai Added small example that illustrates how it should work.
Back in order! |
@vpisarev @asmorkalov or @SpecLad could one of you merge this? I complete ruined my other branches and want to delete my repo to recreate it. For this I first need this good branch to be merged! It has been authorized by @asmorkalov |
👍 |
Thank you so much! This litterally took ages :D |
O great! No idea why pushbot took so long but NICE! Thanks to @ShaiV for helping me out and making this possible! |
This PR tries to fix the following 3 reported issues
A complete remake of the PvAPI API interface solving bugs but also