GradCurve VapourSynth plugin port of virtualdub Gradation Curves plugin

[Myrsloik]

Originally Posted by xekon

So there are no compiler errors, but VSedit crashes without any error messages when I try to use the filter.

how can I get VSedit to report the reason it crashed? to get more details, it has something to do with this filter, but i have no idea what without some kind of feedback.

...

Run it in a debugger then.

[shekh]

Some hints assuming you use Visual Studio
build just the plugin with debug info and with no optimizations
set breakpoints to all major functions
specify launch options for debugging (application and command line)

[feisty2]

	rgblab = new long[16777216];
	labrgb = new long[16777216];

and no delete[] for both arrays, basic memory leak..
use smart pointers if you want some kind of automatic GC for C++ like in Java or C#

and both arrays do not feature a runtime determined length, simply make them static if worrying about blowing the stack up

	static int32_t rgblab[16777216];
	static int32_t labrgb[16777216];

[feisty2]

or a bit more organized with C++ 14..

#include <cstdint>

auto operator""ElementsLUTTable(size_t length) {
	struct LUTTable final {
		int32_t *RGBToLab = nullptr;
		int32_t *LabToRGB = nullptr;
		LUTTable(size_t length = 0) {
			RGBToLab = new int32_t[length];
			LabToRGB = new int32_t[length];
		}
		LUTTable(const LUTTable &) = delete;
		LUTTable(LUTTable &&obj) {
			RGBToLab = obj.RGBToLab;
			LabToRGB = obj.LabToRGB;
			obj.RGBToLab = nullptr;
			obj.LabToRGB = nullptr;
		}
		auto &operator=(const LUTTable &) = delete;
		auto &operator=(LUTTable &&obj) {
			if (this != &obj) {
				RGBToLab = obj.RGBToLab;
				LabToRGB = obj.LabToRGB;
				obj.RGBToLab = nullptr;
				obj.LabToRGB = nullptr;
			}
			return *this;
		}
		~LUTTable() {
			delete[] RGBToLab;
			delete[] LabToRGB;
		}
	};
	return LUTTable(length);
}

auto table = 16777216ElementsLUTTable;

[Mystery Keeper]

Originally Posted by feisty2

or a bit more organized with C++ 14..

#include <cstdint>

auto operator""ElementsLUTTable(size_t length) {
	struct LUTTable final {
		int32_t *RGBToLab = nullptr;
		int32_t *LabToRGB = nullptr;
		LUTTable(size_t length = 0) {
			RGBToLab = new int32_t[length];
			LabToRGB = new int32_t[length];
		}
		LUTTable(const LUTTable &) = delete;
		LUTTable(LUTTable &&obj) {
			RGBToLab = obj.RGBToLab;
			LabToRGB = obj.LabToRGB;
			obj.RGBToLab = nullptr;
			obj.LabToRGB = nullptr;
		}
		auto &operator=(const LUTTable &) = delete;
		auto &operator=(LUTTable &&obj) {
			if (this != &obj) {
				RGBToLab = obj.RGBToLab;
				LabToRGB = obj.LabToRGB;
				obj.RGBToLab = nullptr;
				obj.LabToRGB = nullptr;
			}
			return *this;
		}
		~LUTTable() {
			delete[] RGBToLab;
			delete[] LabToRGB;
		}
	};
	return LUTTable(length);
}

auto table = 16777216ElementsLUTTable;

One component of good coding is explicit intent. Your example is obscure. Does it even work? Isn't the structure declared in the scope of operator? Or does "auto" pull the declaration outside?

[shekh]

There was nothing wrong about labrgb tables except missing delete/wrong singleton. God idea to fix just that issue. Tables must not be static (they take space).

[feisty2]

Originally Posted by Mystery Keeper

One component of good coding is explicit intent. Your example is obscure. Does it even work? Isn't the structure declared in the scope of operator? Or does "auto" pull the declaration outside?

it works, test code, compiled and executed smoothly under vs2015

#include <cstdint>
#include <cstdlib>
#include <iostream>

auto operator""ElementsLUTTable(size_t length) {
	struct LUTTable final {
		int32_t *RGBToLab = nullptr;
		int32_t *LabToRGB = nullptr;
		LUTTable(size_t length = 0) {
			RGBToLab = new int32_t[length];
			LabToRGB = new int32_t[length];
		}
		LUTTable(const LUTTable &) = delete;
		LUTTable(LUTTable &&obj) {
			RGBToLab = obj.RGBToLab;
			LabToRGB = obj.LabToRGB;
			obj.RGBToLab = nullptr;
			obj.LabToRGB = nullptr;
		}
		auto &operator=(const LUTTable &) = delete;
		auto &operator=(LUTTable &&obj) {
			if (this != &obj) {
				RGBToLab = obj.RGBToLab;
				LabToRGB = obj.LabToRGB;
				obj.RGBToLab = nullptr;
				obj.LabToRGB = nullptr;
			}
			return *this;
		}
		~LUTTable() {
			delete[] RGBToLab;
			delete[] LabToRGB;
		}
	};
	return LUTTable(length);
}

auto table = 16777216ElementsLUTTable;

auto main()->int {
	table.LabToRGB[123] = 123456;
	std::cout << table.LabToRGB[123] << std::endl;
	system("pause");
}

One component of good coding is explicit intent.

that's exactly what I'm trying to do.

auto value = 1.234;
auto val = 123;

such statements are legal for all basic (hardware supported) data types, but wouldn't work on user defined data types before C++11.

user defined literals (operator""xxx) have been introduced to C++ since C++11, which makes such stuff

auto cval = 3 + 1.123i; //operator""i
auto table = 16777216ElementsLUTTable; //operator""ElementsLUTTable

possible, and now you could make all your custom data types just as native as the basic data types long as you want to.

Does it even work? Isn't the structure declared in the scope of operator? Or does "auto" pull the declaration outside?

it works because the move constructor (since C++11) has been defined.

LUTTable(LUTTable &&obj) {
	RGBToLab = obj.RGBToLab;
	LabToRGB = obj.LabToRGB;
	obj.RGBToLab = nullptr;
	obj.LabToRGB = nullptr;
}

auto &operator=(LUTTable &&obj) {
	if (this != &obj) {
		RGBToLab = obj.RGBToLab;
		LabToRGB = obj.LabToRGB;
		obj.RGBToLab = nullptr;
		obj.LabToRGB = nullptr;
	}
	return *this;
}

So a C idiom is bad coding style, but setting data within an object to null within a copy constructor is good code? lolwut? A copy constructor is only meant to clone an object, it's not meant to modify the original object. Adding side-effects to a copy constructor is horrendous.

[feisty2]

Originally Posted by captaiŋadamo

LUTTable(LUTTable &&obj) {
	RGBToLab = obj.RGBToLab;
	LabToRGB = obj.LabToRGB;
	obj.RGBToLab = nullptr;
	obj.LabToRGB = nullptr;
}

auto &operator=(LUTTable &&obj) {
	if (this != &obj) {
		RGBToLab = obj.RGBToLab;
		LabToRGB = obj.LabToRGB;
		obj.RGBToLab = nullptr;
		obj.LabToRGB = nullptr;
	}
	return *this;
}

So a C idiom is bad coding style, but setting data within an object to null within a copy constructor is good code? lolwut? A copy constructor is only meant to clone an object, it's not meant to modify the original object. Adding side-effects to a copy constructor is horrendous.

LUTTable(const LUTTable &) // copy constructor
LUTTable(LUTTable &&) // MOVE constructor (since C++11)

https://msdn.microsoft.com/en-us/library/dd293665.aspx

[feisty2]

and @captaiŋadamo
it's almost 2017 now, not 1997, time to shake the obsolete ideas off, C++ has always been EVOLVING! (C++17 is just around the corner )

[xekon]

Originally Posted by shekh

Some hints assuming you use Visual Studio
build just the plugin with debug info and with no optimizations
set breakpoints to all major functions
specify launch options for debugging (application and command line)

I am using Visual Studio 2015, when I try to run it in debug mode it says that it is not a valid Win32 application, which is because there is no MAIN function, since its library.
I could add a main function, but I would assume that I would also have to pass data to the functions like it would when the library is in use, otherwise how could I find the problem.

when I read about debugging dll libraries on microsoft website they suggest debugging the host application.
So I am currently installing Qt5, and trying to get everything in place to debug VSedit while its using the GradCurve plugin.
I just hope I am barking up the right tree here, and not off in left field.

EDIT:
I updated the github https://github.com/jieiku/GradCurve/
Removed variables left behind from vdub specific stuff as well as variables from starting off with HolyWu's AddGrain filter.
There is more cleanup to do once I have this working.

[feisty2]

Fix that memory leak and ur done.

[shekh]

You need to start the host application but you dont have to compile and debug it, the debugger kicks in once your dll is loaded.

[xekon]

OH! I didn't realize that was the only problem! I will try your implementation feisty2! thank you!

[shekh]

Btw, concerning lab mode
AFAIK is was both slow and low quality, maybe just worth removing anyway.

[feisty2]

Plus it's "delete[]" for arrays...
not sure if "delete array" will still lead to memory leak (only the first element got deleted)

Edit: just googled about that and "delete" works in your case also, cuz both arrays are an array of the basic data type (no destructor). But delete[] makes more sense, so use that instead

Edit2: I'm talking about line 673 and line 674 in ur source code

[xekon]

Originally Posted by feisty2

Plus it's "delete[]" for arrays...
not sure if "delete array" will still lead to memory leak (only the first element got deleted)

Edit: just googled about that and "delete" works in your case also, cuz both arrays are an array of the basic data type (no destructor). But delete[] makes more sense, so use that instead

Edit2: I'm talking about line 673 and line 674 in ur source code

Thank you everyone!

and thank you feisty2, i switched it to using brackets.

I assumed there were problems other than the memory leak, because when I build under linux vsedit still crashes after correcting the memory leaks, im building a dll under windows and going to try vsedit under windows to see if I still crash.

it sounds like the plugin is functioning for you guys, so something must be up with my build environment.

[feisty2]

since you PMed me, some debug info from vs2015




no offense but, ur code is way too cryptic and pretty much machine code to me..

[xekon]

ah, thanks for taking a look for me. the second picture you posted is where my vsedit crashed I believe. I will keep trying to figure out the reason its crashing. very little of the code was actually written by me, I migrated the code from the vdub c++ filter. I will probably need to get a better idea of how gradation curves is supposed to work before i can fix the problem.

[jackoneill]

Is d.filename initialised with zeroes? If not, strcat may find a zero past its end. Use the "n" versions of string functions if possible, i.e. strncat. Although I don't see why this isn't str(n)cpy. And since you're using C++ you might as well use std::string.

Are you sure you want those global variables in your library?

As far as I can tell, you could call ImportCurve and PreCalcLut from GradCurveCreate. They don't seem to use any data from the filter's input frames. It also looks like oldr, oldg, oldb, medr, medg, and medb can be local variables. Then you can switch to fmParallel.

Your filter accepts integer formats, but it assumes the frames always contain float pixels. If you test your filter with integer clips, the processing loop will inevitably read and write past the end of the frame.