2
\$\begingroup\$

This code is supposed to grab live camera feed, display feed in a window, mark in rectangles all detected faces, get the biggest detected face (by total area), display it in separate window, convert it to grayscale and finally save as PNG to hard disk, in project directory.

Any ideas for optimizing this code? It has to be OpenCV 2.4.5 compliant.

I kindly ask only people familiar with OpenCV2 to give their advice. They know what I mean as for lot of us there is sometimes problem adapting from OpenCV1 to OpenCV2.

#include "opencv2/objdetect/objdetect.hpp"
#include "opencv2/highgui/highgui.hpp"
#include "opencv2/imgproc/imgproc.hpp"

#include <iostream>
#include <stdio.h>

using namespace std;
using namespace cv;

// Function Headers
void detectAndDisplay(Mat frame);

// Global variables
// Copy this file from opencv/data/haarscascades to target folder
string face_cascade_name = "c:/haarcascade_frontalface_alt.xml";
CascadeClassifier face_cascade;
string window_name = "Capture - Face detection";
int filenumber; // Number of file to be saved
string filename;

// Function main
int main(void)
{
    VideoCapture capture(0);

    if (!capture.isOpened())  // check if we succeeded
        return -1;

    // Load the cascade
    if (!face_cascade.load(face_cascade_name))
    {
        printf("--(!)Error loading\n");
        return (-1);
    };

    // Read the video stream
    Mat frame;

    for (;;)
    {
        capture >> frame;

        // Apply the classifier to the frame
        if (!frame.empty())
        {
            detectAndDisplay(frame);
        }
        else
        {
            printf(" --(!) No captured frame -- Break!");
            break;
        }

        int c = waitKey(10);

        if (27 == char(c))
        {
            break;
        }
    }

    return 0;
}

// Function detectAndDisplay
void detectAndDisplay(Mat frame)
{
    std::vector<Rect> faces;
    Mat frame_gray;
    Mat crop;
    Mat res;
    Mat gray;
    string text;
    stringstream sstm;

    cvtColor(frame, frame_gray, COLOR_BGR2GRAY);
    equalizeHist(frame_gray, frame_gray);

// Detect faces
    face_cascade.detectMultiScale(frame_gray, faces, 1.1, 2, 0 | CASCADE_SCALE_IMAGE, Size(30, 30));

// Set Region of Interest
    cv::Rect roi_b;
    cv::Rect roi_c;

    size_t ic = 0; // ic is index of current element
    int ac = 0; // ac is area of current element

    size_t ib = 0; // ib is index of biggest element
    int ab = 0; // ab is area of biggest element

    for (ic = 0; ic < faces.size(); ic++) // Iterate through all current elements (detected faces)

    {
        roi_c.x = faces[ic].x;
        roi_c.y = faces[ic].y;
        roi_c.width = (faces[ic].width);
        roi_c.height = (faces[ic].height);

        ac = roi_c.width * roi_c.height; // Get the area of current element (detected face)

        roi_b.x = faces[ib].x;
        roi_b.y = faces[ib].y;
        roi_b.width = (faces[ib].width);
        roi_b.height = (faces[ib].height);

        ab = roi_b.width * roi_b.height; // Get the area of biggest element, at beginning it is same as "current" element

        if (ac > ab)
        {
            ib = ic;
            roi_b.x = faces[ib].x;
            roi_b.y = faces[ib].y;
            roi_b.width = (faces[ib].width);
            roi_b.height = (faces[ib].height);
        }

        crop = frame(roi_b);
        resize(crop, res, Size(128, 128), 0, 0, INTER_LINEAR); // This will be needed later while saving images
        cvtColor(crop, gray, CV_BGR2GRAY); // Convert cropped image to Grayscale

        // Form a filename
        filename = "";
        stringstream ssfn;
        ssfn << filenumber << ".png";
        filename = ssfn.str();
        filenumber++;

        imwrite(filename, gray);

        Point pt1(faces[ic].x, faces[ic].y); // Display detected faces on main window - live stream from camera
        Point pt2((faces[ic].x + faces[ic].height), (faces[ic].y + faces[ic].width));
        rectangle(frame, pt1, pt2, Scalar(0, 255, 0), 2, 8, 0);
    }

// Show image
    sstm << "Crop area size: " << roi_b.width << "x" << roi_b.height << " Filename: " << filename;
    text = sstm.str();

    putText(frame, text, cvPoint(30, 30), FONT_HERSHEY_COMPLEX_SMALL, 0.8, cvScalar(0, 0, 255), 1, CV_AA);
    imshow("original", frame);

    if (!crop.empty())
    {
        imshow("detected", crop);
    }
    else
        destroyWindow("detected");
}
\$\endgroup\$
10
\$\begingroup\$

Your header asks for optimization, but your question asks for any ideas for improving the code. I'll answer the latter. If you need to optimize in terms of execution speed or memory use, I recommend using a profiler to measure what the critical path is.

Overall design

Your program uses a very C-like structure. Using an object-oriented approach can usually offer many benefits. For example, it will limit the scope of your global variables, since they can be member variables instead.

Your program uses both C++ and C-style IO. I strongly advice you to pick one of them. My recommendation is C++ iostreams, as they are type-safe.

Inconsistent style

Your programming style is inconsitent; you do different things in different parts of the program. That looks very untidy. For example, at one point you do:

if (!capture.isOpened())  // check if we succeeded
    return -1;

whereas later in the same function, another one-line if gets braces:

if (27 == char(c))
{
    break;
}

Personally, I prefer to write if statements like this:

if (!capture.isOpened()) return -1;

or over multiple lines and with braces. This avoids the risk of code like this:

if (!foo.bar())
    printf(" --(!) No captured frame -- Break!");
    foo.cleanup()

(In the previous snippet, foo.cleanup() is likely supposed to be run only if the if triggers, but will be run regardless.)

Another example is that at one point you do return -1;, but later you do return (-1);. Prefer the former; return is not a function.

Readability

Some of your variables have very poor names. Instead of this:

size_t ic = 0; // ic is index of current element

do this:

std::size_t index_current = 0;

Another thing that greatly boosts readability is avoiding literals in function calls. For example:

face_cascade.detectMultiScale(frame_gray, faces, 1.1, 2,
        0 | CASCADE_SCALE_IMAGE, Size(30, 30));

What's 1.1? 2? Why is the size 30x30? What do they mean? Are they correct? What possible values can they have? I think it's much better to define symbolic constants instead:

const float size_factor = 1.1;
const std::size_t num_buffers = 2;
const Size face_size(30, 30);

face_cascade.detectMultiScale(frame_gray, faces, size_factor, num_buffers, 0 | CASCADE_SCALE_IMAGE, face_size);

(The names I have given them are likely not what they actually mean. This is just an example.) An addition benefit with this is that code like this:

const float percentage = 20000f;
do_something(some_file, percentage);

Is more likely to be picked up as a possible error than this:

do_something(some_file, 20000f);

On the same note, 0 | CASCADE_SCALE_IMAGE is always exactly the same as CASCADE_SCALE_IMAGE, so you can skip the superfluous 0 |.

Comments

I think comments above the relevant line is more readable than comments beside the relevant line. In other words, prefer

// Get the area of biggest element, at beginning it is same as "current" element
ab = roi_b.width * roi_b.height;

Over

ab = roi_b.width * roi_b.height; // Get the area of biggest element, at beginning it is same as "current" element

Also, some of your comments are unnecessary. Comments should explain why, rather than what or how. Comments should not repeat what the code is already saying. // Function main is a great example of a comment that does not add information. Another example is // check if we succeeded.

Personally, I think brief statements of what is going on are good, as long as it doesn't get too much. For example the // Detect faces comment -- I think that's OK.

Small smells and other details

  • Avoid polluting the global namespace with using directives.
  • In C++, int main(void) and int main() is exactly the same. Drop void -- this is not C.
  • I personally don't like Yoda Conditions, but that's just a matter of preference.
  • Limit the scope of variables as much as possible. For example, ic should be restricted to the for loop, as should filename.
  • Your program has very little error handling.
  • If C++11 is an option, consider using a range-based (foreach-style) for loop

Like this:

for (auto const& face : faces)
{
    roi_c.x = face.x;
    roi_c.y = face.y;

    // ...
}
\$\endgroup\$
  • \$\begingroup\$ It is a rework of OpenCV2 example. I know about OO approach as I come from Java/C# world. OpenCV framework itself is all mixed up. Even on their official tutorials C and C++ are heavily mixed. Latest trend is to do what you suggest - to exclude C and adopt only C++. Considering that it is framework who currently supports both approaches even long time users of framework are somewhat confused and mix both approaches. You can notice that through many questions on Stackoverflow. BTW I am using Eclipse CDT with autoformat (Allman/BSD style). Anyway, nice review and detailed, I gave you +1. \$\endgroup\$ – Nenad Bulatovic Jul 4 '13 at 18:04
1
\$\begingroup\$

This is rather messy code, lots of inconsistencies and I doubt it really works. It looks like something you have written and not bothered to review yourself. Writing code is normally an iterative process (for me anyway), whereby you write something and then consider whether it is any good. Then you refine the solution and consider again. Your idea of what is "any good" will develop over time, but inconsistency and repetition are clearly not good.

A few observations:

Embedded paths: It is best not to embed paths in your code. For example the include file paths should be defined during the build (with -I) and the face_cascade_name variable should get its value some other way, eg through arguments to main.


In this code, faces is a vector of Rect and roi_c is also a Rect.

roi_c.x = faces[ic].x;
roi_c.y = faces[ic].y;
roi_c.width = (faces[ic].width);
roi_c.height = (faces[ic].height);

ac = roi_c.width * roi_c.height; // area of current element

roi_b.x = faces[ib].x;
roi_b.y = faces[ib].y;
roi_b.width = (faces[ib].width);
roi_b.height = (faces[ib].height);

ab = roi_b.width * roi_b.height; // area of biggest element

if (ac > ab)
{
    ib = ic;
    roi_b.x = faces[ib].x;
    roi_b.y = faces[ib].y;
    roi_b.width = (faces[ib].width);
    roi_b.height = (faces[ib].height);
}
crop = frame(roi_b);

But roi_c is never used again and roi_b is assigned the same value twice! My guess is the second assignment should be of roi_c to roi_b, but the whole would be better as:

int area_c = faces[ic].width * faces[ic].height;
int area_b = faces[ib].width * faces[ib].height;

if (area_c > area_b) {
    roi = faces[ic];
} else {
    roi = faces[ib];
}
crop = frame(roi);

Your coud also pass by reference to frame instead of passing by value. And you might add an area method to the Rect class.

\$\endgroup\$
  • \$\begingroup\$ "...and I doubt it really works". Yes, it works. I agree, instead of { ib = ic; roi_b.x = faces[ib].x; roi_b.y = faces[ib].y; roi_b.width = (faces[ib].width); roi_b.height = (faces[ib].height); } I could use just { roi_b.x = faces[ic].x; roi_b.y = faces[ic].y; roi_b.width = (faces[ic].width); roi_b.height = (faces[ic].height); } . It works both ways. It took a LOT of time to code that tiny piece of code. When I was too tired to even look at monitor I posted it here, hoping to get some good advice not comment such as "..and not bothered to review yourself. " \$\endgroup\$ – Nenad Bulatovic Jul 4 '13 at 17:54
  • \$\begingroup\$ You assigned roi_b.x/y/width/height with exactly the same values twice, once before the if (ac > ab) and once inside that condition. Do you not think that is wrong? \$\endgroup\$ – William Morris Jul 4 '13 at 18:04
  • \$\begingroup\$ No, because of: ib = ic; it is NOT exactly the same value. If condition ac > ab is met then ib = ic so the duplicate of this code (when it occurs second time) roi_b.x = faces[ib].x; roi_b.y = faces[ib].y; roi_b.width = (faces[ib].width); roi_b.height = (faces[ib].height); actually becomes (actually) this: roi_b.x = faces[ic].x; roi_b.y = faces[ic].y; roi_b.width = (faces[ic].width); roi_b.height = (faces[ic].height); \$\endgroup\$ – Nenad Bulatovic Jul 4 '13 at 18:11
  • 1
    \$\begingroup\$ Oops! I missed that. The refactoring in my answer is still much better code, in my opinion. But you are free to ignore that of course. My answer was unnecessarily blunt, for which I apologise. \$\endgroup\$ – William Morris Jul 4 '13 at 18:41
  • \$\begingroup\$ "refactoring in my answer is still much better code" +1 for that. I will carefully look at it when I get good sleep. As for "Embedded paths" unfortunately, from reasons unknown, at least in this configuration (eclipse CDT, windows 7, MinGW) eclipse CAN'T find relative path, even if additional files are placed next to .exe in /debug. However, if exe is executed from command prompt, then it finds it. Anyway just because of debugging purposes I hardcoded path - but your observation about this is correct. \$\endgroup\$ – Nenad Bulatovic Jul 4 '13 at 18:51

protected by Jamal Feb 14 '15 at 16:36

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.