3
\$\begingroup\$

I'm working on a program that will (hopefully) eventually allow me to perform frame rate analysis like Digital Foundry does. The code below works (as far as I know), but before I iterate on it, I'd like some feedback as I'm new to both C++ and OpenCV. I say "as far as I know" because I'm not sure how "lossless" my lossless capture is or of the correctness of the program. Is this code a good start?

#include "opencv2\opencv.hpp"
#include <fstream>

using namespace cv;
using namespace std;

int main(int argv, char** argc) {
    Mat curFrame;
    Mat pastFrame;
    VideoCapture vid("E:\\videos\\video.mkv");
    ofstream textFile("diffs.txt");
    int numDiff = 0;
    int numFrames = 0;

    if (!textFile.is_open()) {
        return -1;
    }

    if (!vid.isOpened()) {
        return -2;
    }

    vid.read(pastFrame);
    while (vid.read(curFrame)) {
        for (int i = 0; i < curFrame.rows; i += 1) {
            cv::Vec3b* curPixel = curFrame.ptr<cv::Vec3b>(i);
            cv::Vec3b* pastPixel = pastFrame.ptr<cv::Vec3b>(i);
            for (int j = 0; j < curFrame.cols; j += 1) {
                bool found = false;
                if (curPixel[j][0] != pastPixel[j][0]) {
                    numDiff += 1;
                    found = true;
                } else if (!found && curPixel[j][1] != pastPixel[j][1]) {
                    numDiff += 1;
                    found = true;
                } else if (!found && curPixel[j][2] != pastPixel[j][2]) {
                    numDiff += 1;
                }
            }
        }
        numFrames += 1;
        //cout << "Frame " << numFrames << endl;
        double size = curFrame.rows * curFrame.cols;
        textFile << numFrames << "," << fixed << setprecision(2) << numDiff / size * 100 << "\n";
        pastFrame = curFrame.clone();
        numDiff = 0;
    }

    textFile.close();
    vid.release();
    return 0;
}
\$\endgroup\$
6
\$\begingroup\$

Its pretty good:

Don't do this:

using namespace cv;
using namespace std;

Its a bad habit that one day will get you into a lot of trouble. The reason the namespace names are so short is so that adding std:: or cv:: before identifiers is not overburdensome.

Read more about the issues: Why is “using namespace std” considered bad practice?

I suppose this is valid:

int main(int argv, char** argc) {

But most people use this:

int main(int argc, char* argv[]) {
                         ^^^^   => argv: argument value
                       ^     ^^ => Easier to see its an array of pointers rather **
             ^^^^               => argc: argument count

Declare variables at point of usage.

In C++ we don't tend to declare all the variables at the top of the function. It is much nicer to declare them just before you use them. Remember all objects have constructors that are run so declaring it before you need it may cost you in execution if you don't need it.

But the main reason is that types are important. If you declare it at the point you use it then it becomes easy to see the type and thus reason about it. If I have to scroll back to the top of the function it makes it hard to keep track of thing.

Streams can detect when they have gone bad.

Not opening the file is not the only type of error. So only testing for that seems like a short cut that may backfire.

    if (!textFile.is_open()) {
        return -1;
    }

I would use:

    if (!textFile) {    // If a std::fstream is used in a boolean context
        return -1;      // it converts itself to bool be checking is_good()
    }                   // which will fail if you can't read from it in any
                        // way.

Not sure if that is true for VideoCapture type (have a look). But given the read works like a std::stream (see the while loop) I am suspecting it does.

    if (!vid.isOpened()) {
        return -2;
    }

Prefer pre-increment

        for (int i = 0; i < curFrame.rows; i += 1) {
            for (int j = 0; j < curFrame.cols; j += 1) {

Sure that works. But prefer ++i (or ++j). It is more idiomatic. Also when you start incrementing using iterators the pre-increment is the cheapest increment.

Don't need found

                bool found = false;
                if (curPixel[j][0] != pastPixel[j][0]) {
                    numDiff += 1;
                    found = true;
                } else if (!found && curPixel[j][1] != pastPixel[j][1]) {
                    numDiff += 1;
                    found = true;
                } else if (!found && curPixel[j][2] != pastPixel[j][2]) {
                    numDiff += 1;
                }

Each of those pieces of code are completely unique. Because you use else only one of those branches can be executed.

I would simplify and write as:

            numDiff += ((curPixel[j][0] != pastPixel[j][0]) ||
                        (curPixel[j][1] != pastPixel[j][1]) ||
                        (curPixel[j][2] != pastPixel[j][2]))  ? 1 : 0;

Based on Toby's comments this can be simplified too:

            numDiff += curPixel[j] != pastPixel[j] ? 1 : 0;

Cloning is probably expensive

        pastFrame = curFrame.clone();

Have your tried to see if a move operation exists?

        pastFrame = std::move(curFrame);

Don't manually close files

    textFile.close();

Unless you are checking to see if the close worked and generating an error message when it fails. Otherwise just let the constructor call close and throw away any potential exceptions that are generated.

On the same note. Do you really need to manually release the vid?

    vid.release();

Version 2

#include "opencv2\opencv.hpp"
#include <fstream>

int main(int argc, char* argv[])
{
    std::ofstream textFile("diffs.txt");    
    if (!textFile.is_open()) {
        return -1;
    }

    cv::VideoCapture vid("E:\\videos\\video.mkv");
    if (!vid.isOpened()) {
        return -2;
    }

    int numFrames = 0;

    cv::Mat pastFrame;
    vid.read(pastFrame);

    cv::Mat curFrame;
    while (vid.read(curFrame)) {
        int numDiff = 0;
        for (int i = 0; i < curFrame.rows; ++j) {

            cv::Vec3b* curPixel = curFrame.ptr<cv::Vec3b>(i);
            cv::Vec3b* pastPixel = pastFrame.ptr<cv::Vec3b>(i);

            for (int j = 0; j < curFrame.cols; ++j) {
                numDiff += curPixel[j] != pastPixel[j] ? 1 : 0;
            }
        }
        numFrames += 1;
        double size = curFrame.rows * curFrame.cols;
        textFile << numFrames << ","
                 << fixed 
                 << setprecision(2) << numDiff / size * 100
                 << "\n";
        pastFrame = curFrame.clone();
    }
}
\$\endgroup\$
1
\$\begingroup\$

Here, you accept program arguments:

int main(int argv, char** argc) {

But then you never use argc or argv (note that it's conventional to call the count argc, and the values argv, not the other way around)!

Instead, the input and output files are hard-coded, forcing you to move files around. Why not allow the user to specify input and output as arguments? And if not specified, it's probably best to default to standard input and output streams, so that your program can be used in a pipeline, rather than being constrained to work only on files.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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