newbie filter writing problem

[feisty2]

scripts ain't enough no more, gotta learn to do some filters
and I decided to start with simple ones, the "hello world" plugin (invert.c in the sdk folder) worked just ok, but I got my ass into troubles when I was writing the "hello world" neighbor filter, replace the center pixel with the average of 4 closest pixels around it


#include "VapourSynth.h"
#include "VSHelper.h"

typedef struct {
VSNodeRef *node;
const VSVideoInfo *vi;
} TestData;

static void VS_CC testInit(VSMap *in, VSMap *out, void **instanceData, VSNode *node, VSCore *core, const VSAPI *vsapi) {
TestData *d = (TestData *)* instanceData;
vsapi->setVideoInfo(d->vi, 1, node);
}

static const VSFrameRef *VS_CC testGetFrame(int n, int activationReason, void **instanceData, void **frameData, VSFrameContext *frameCtx, VSCore *core, const VSAPI *vsapi) {
TestData *d = (TestData *)* instanceData;

if (activationReason == arInitial) {
vsapi->requestFrameFilter(n, d->node, frameCtx);
}
else if (activationReason == arAllFramesReady) {
const VSFrameRef *src = vsapi->getFrameFilter(n, d->node, frameCtx);
const VSFormat *fi = d->vi->format;
int height = vsapi->getFrameHeight(src, 0);
int width = vsapi->getFrameWidth(src, 0);

VSFrameRef *dst = vsapi->newVideoFrame(fi, width, height, src, core);

int plane;
for (plane = 0; plane < fi->numPlanes; plane++) {
const float *srcp = reinterpret_cast <const float *> (vsapi->getReadPtr(src, plane));
int src_stride = vsapi->getStride(src, plane) / 4;
float *dstp = reinterpret_cast <float *> (vsapi->getWritePtr(dst, plane));
int dst_stride = vsapi->getStride(dst, plane) / 4;

int h = vsapi->getFrameHeight(src, plane);
int y;
int w = vsapi->getFrameWidth(src, plane);
int x;

const float *above = srcp - src_stride;
const float *below = srcp + src_stride;

for (y = 1; y < h-1; y++) {
for (x = 1; x < w-1; x++)
dstp[x] = (srcp[x-1] + srcp[x+1] + above[x] + below[x]) / 4.f;

dstp += dst_stride;
srcp += src_stride;
above += src_stride;
below += src_stride;
}
}

vsapi->freeFrame(src);

return dst;
}

return 0;
}

static void VS_CC testFree(void *instanceData, VSCore *core, const VSAPI *vsapi) {
TestData *d = (TestData *)instanceData;
vsapi->freeNode(d->node);
free(d);
}

static void VS_CC testCreate(const VSMap *in, VSMap *out, void *userData, VSCore *core, const VSAPI *vsapi) {
TestData d;
TestData *data;

d.node = vsapi->propGetNode(in, "clip", 0, 0);
d.vi = vsapi->getVideoInfo(d.node);
data = (TestData *)malloc(sizeof(d));
*data = d;
vsapi->createFilter(in, out, "Test", testInit, testGetFrame, testFree, fmParallel, 0, data, core);
}


VS_EXTERNAL_API(void) VapourSynthPluginInit(VSConfigPlugin configFunc, VSRegisterFunction registerFunc, VSPlugin *plugin) {
configFunc("com.newbie.test", "test", "Test Filter", VAPOURSYNTH_API_VERSION, 1, plugin);
registerFunc("test", "clip:clip;", testCreate, 0, plugin);
}



it was compiled successfully but crashed when I called it in the python script

what did I do wrong?

[Groucho2004]

Can you not debug VS plugins?

[feisty2]

the debugger stated "dstp[x] = (srcp[x-1] + srcp[x+1] + above[x] + below[x]) / 4.f;" caused the crash
but it just feels correct to me

[Groucho2004]

the debugger stated "dstp[x] = (srcp[x-1] + srcp[x+1] + above[x] + below[x]) / 4.f;" caused the crash
but it just feels correct to me
Somewhere in that loop you're probably exceeding the array bounds.

[Groucho2004]

the debugger stated "dstp[x] = (srcp[x-1] + srcp[x+1] + above[x] + below[x]) / 4.f;" caused the crash
but it just feels correct to me
In that statement, you're treating "above" and "below" as arrays. However, they're declared as const float. What's that all about?

[feisty2]

Somewhere in that loop you're probably exceeding the array bounds.

actually no, I did

for (y = 1; y < h-1; y++) {
for (x = 1; x < w-1; x++)

intentionally instead of

for (y = 0; y < h; y++) {
for (x = 0; x < w; x++)

to avoid just that

[StainlessS]

I dont speak parseltongue but

srcp and dstp are pointer to float, whereas w is not width in float, (maybe).

EDIT: ie use w = w/4, or explicitly as int w = vsapi->getFrameWidth(src, plane) / 4; (or / sizeof(float), or whatever it is).

[Groucho2004]

Actually, I've no idea what is going in in your code. I don't know the VSAPI so I better shut up.
However, all these float variables seem very odd to me.

[feisty2]

In that statement, you're treating "above" and "below" as arrays. However, they're declared as const float. What's that all about?

and srcp and dstp are float pointers as well, and used as arrays in the official sample filter here
https://github.com/vapoursynth/vapoursynth/blob/master/sdk/invert_example.c

I don't see "above" or "below" any different from them

[StainlessS]

See my post, that is your problem.

[feisty2]

Actually, I've no idea what is going in in your code. I don't know the VSAPI so I better shut up.
However, all these float variables seem very odd to me.

the clip to be processed is 32bits float format

[vcmohan]

your above and below are not in loop. You defined above and below outside loop. Change them to be within loop ( after loop of h) and starting above = srcp ( since you are starting loop h value of 1.

[feisty2]

@vcmohan
thx, tried but didn't work
@StainlessS
I don't see the point of doing that, but still tried, and didn't work either
:(

[jackoneill]

the debugger stated "dstp[x] = (srcp[x-1] + srcp[x+1] + above[x] + below[x]) / 4.f;" caused the crash
but it just feels correct to me

When you have multiple loads/stores in a line, you can find out exactly which is the culprit by looking at the disassembly. The debugger will tell you exactly which instruction caused the crash.

Here you are reading from one line above the beginning of the frame, i.e. above[x] is causing the crash. above is src - stride, but you didn't move src to the second line before entering the loop.

[Myrsloik]

You forgot that you have to increment srcp by src_stride before entering the loop. You start at line 1 in your loop but the pointer is still to line 0.

Also, keeping above and below as separate is pointless and sometimes generates worse code than srcp[x + src_stride]. Fun fact.

[StainlessS]

OK, then you have two problems, w is related to byte width instead of float and you are starting with srcp at y=0 instead of y=1.

EDIT: You will I think have to treat all dst edge pixels as special case.

[feisty2]

so I copy-pasted the whole dstp += dst_stride; srcp += src_stride; above += src_stride; below += src_stride; thing before the loop and it works now

thanks to all you guys :)

[StainlessS]

Feisty2, did you need to change this line or not ?


int w = vsapi->getFrameWidth(src, plane);


or does it magically know that it is float whereas the read/write pointer stuff just returns point to byte.

EDIT: OK, think I got it, the stride is difference in address and not in sizeof(float) units and so that has to be
divided by sizeof(float) due to pointer arithmetic, whereas width is returned as float width.
My mistake, sorry.

[feisty2]

Feisty2, did you need to change this line or not ?


int w = vsapi->getFrameWidth(src, plane);


or does it magically know that it is float whereas the read/write pointer stuff just returns point to byte.

no need to touch it, it just returns the "width" value stored in the frame properties, like, if the clip is 4096x2160, it just returns 4096

[StainlessS]

Thank you, i realized my mistake, as in above edit.

[feisty2]

so I guess here's a not too shabby advanced "hello world" sample filter, compared to "invert.c",
it also features:
1. multiple inputs
2. high precision support (works on 32bits float clips)
3. neighbor processing (why would anyone even bother to write a new plugin if you can just do ur thang with std.Expr?)
the plugin takes 2 inputs, [clip1, clip2], the left and right pixel around the center pixel will take from clip1,
above and below pixel will take from clip2, and the center pixel will be replaced with the average of all 4 pixels


#include "VapourSynth.h"
#include "VSHelper.h"

typedef struct {
VSNodeRef *node1;
VSNodeRef *node2;
const VSVideoInfo *vi;
} TestData;

static void VS_CC testInit(VSMap *in, VSMap *out, void **instanceData, VSNode *node, VSCore *core, const VSAPI *vsapi) {
TestData *d = (TestData *)* instanceData;
vsapi->setVideoInfo(d->vi, 1, node);
}

static const VSFrameRef *VS_CC testGetFrame(int n, int activationReason, void **instanceData, void **frameData, VSFrameContext *frameCtx, VSCore *core, const VSAPI *vsapi) {
TestData *d = (TestData *)* instanceData;

if (activationReason == arInitial) {
vsapi->requestFrameFilter(n, d->node1, frameCtx);
vsapi->requestFrameFilter(n, d->node2, frameCtx);
}
else if (activationReason == arAllFramesReady) {
const VSFrameRef *src1 = vsapi->getFrameFilter(n, d->node1, frameCtx);
const VSFrameRef *src2 = vsapi->getFrameFilter(n, d->node2, frameCtx);
const VSFormat *fi = d->vi->format;
int height = vsapi->getFrameHeight(src1, 0);
int width = vsapi->getFrameWidth(src1, 0);

VSFrameRef *dst = vsapi->newVideoFrame(fi, width, height, src1, core);

int plane;
for (plane = 0; plane < fi->numPlanes; plane++) {
const float *srcp1 = reinterpret_cast <const float *> (vsapi->getReadPtr(src1, plane));
const float *srcp2 = reinterpret_cast <const float *> (vsapi->getReadPtr(src2, plane));
int stride = vsapi->getStride(src1, plane) / 4;
float *dstp = reinterpret_cast <float *> (vsapi->getWritePtr(dst, plane));

int h = vsapi->getFrameHeight(src1, plane);
int y;
int w = vsapi->getFrameWidth(src1, plane);
int x;

const float *above = srcp2 - stride;
const float *below = srcp2 + stride;

dstp += stride;
srcp1 += stride;
srcp2 += stride;
above += stride;
below += stride;

for (y = 1; y < h-1; y++) {
for (x = 1; x < w-1; x++)
dstp[x] = (srcp1[x-1] + srcp1[x+1] + above[x] + below[x]) / 4.f;

dstp += stride;
srcp1 += stride;
srcp2 += stride;
above += stride;
below += stride;
}
}

vsapi->freeFrame(src1);
vsapi->freeFrame(src2);

return dst;
}

return 0;
}

static void VS_CC testFree(void *instanceData, VSCore *core, const VSAPI *vsapi) {
TestData *d = (TestData *)instanceData;
vsapi->freeNode(d->node1);
vsapi->freeNode(d->node2);
free(d);
}

static void VS_CC testCreate(const VSMap *in, VSMap *out, void *userData, VSCore *core, const VSAPI *vsapi) {
TestData d;
TestData *data;

d.node1 = vsapi->propGetNode(in, "clips", 0, 0);
d.node2 = vsapi->propGetNode(in, "clips", 1, 0);
d.vi = vsapi->getVideoInfo(d.node1);
data = (TestData *)malloc(sizeof(d));
*data = d;
vsapi->createFilter(in, out, "Test", testInit, testGetFrame, testFree, fmParallel, 0, data, core);
}


VS_EXTERNAL_API(void) VapourSynthPluginInit(VSConfigPlugin configFunc, VSRegisterFunction registerFunc, VSPlugin *plugin) {
configFunc("com.newbie.test", "test", "Test Filter", VAPOURSYNTH_API_VERSION, 1, plugin);
registerFunc("test", "clips:clip[];", testCreate, 0, plugin);
}

[StainlessS]

I guess you could do away with 1 batch of dstp += stride etc by moving the lot just before the for(x ...) loop, although it may be less clear what the intent is.
Would also do away with the final iteration stride additions which serve no purpose.

[vcmohan]

@feisty2. OK. The code can be cleaned up a bit. But what are you intending to do with the border pixels? You just left them untouched and can have any values.

[feisty2]

@StainlessS and @vcmohan
didn't do CopyPad and code cleanup cuz I don't want any extra complexity on a hello world plugin...

[vcmohan]

since you have come this far, it is necessary to do something about border pixels. This will make any one reading your code to get inspired what needs to be done.
vapoursynth accepts 8bit, 16bit, unsigned integer pixels also. So include in your code processes for them also. If only float data is to be accepted you have to test input and give warnings. Otherwise you will get crashes.

[feisty2]

since you have come this far, it is necessary to do something about border pixels. This will make any one reading your code to get inspired what needs to be done.
vapoursynth accepts 8bit, 16bit, unsigned integer pixels also. So include in your code processes for them also. If only float data is to be accepted you have to test input and give warnings. Otherwise you will get crashes.

okay, will add them later, I'll just pretend that uint8_t and uint16_t vids don't exist cuz float is way better than them (both over/underflow handling and precision), I'll set an error on int vids