Plugin: Turn

[WarpEnterprises]

one missing basic filter (mainly along with importing photos):

TurnLeft and TurnRight (http://members.aon.at/archi/warpenterprises)

(only AviSynth 2.0x and RGB so far)

[Bidoche]

Yes it was missing.
To do it, you add to import from vdub, a waste for such a basic feature...

I was planning to add into 3.0 core (well, I still am :p).

Looking at your code, I see you were stuck by the same problem as I:
How do I turn a YUY2 VideoFrame !? :p

Edit: my mistake: you had to DO it with vdub...

[sh0dan]

If you're ok with it, I'll add this to the 2.5 core, and add YV12 - that way we won't have any conflicts.

Had it planned for some time for 2.5, but never got around to it. Perhaps Turn180() should also be implemented - at least as an alias for flipvertical().fliphorizontal().

I should perhaps also have a look at redesigning Layer for 2.5 (it should be possible to make more readable/maintainable code).

[WarpEnterprises]

Of course, although the code is quite ugly (you know, I'm new @ C++) but it works and is surprisingly fast although it is no way optimized.

[Bidoche]

Well, everyone is new at C++ here, sh0dan is, you are, I am... :)

Finally, we can't turn YUY2 VideoFrame.... :p

[Belgabor]

Well, you can, but it would be like ConvertToYV12().Turn().ConvertToYUY2() :p

[WarpEnterprises]

Bug corrected (thanks to Lyptus).

download (http://members.aon.at/archi/warpenterprises)

[Kurosu]

What I think could be a core function now works for RGB24/RGB32/YUV2/YV12/I420. I forgot YUV format.

I don't see how it could be optimized to used parallel instruction sets. But it can be added very easily to the plugin thanks to a little rewriting.

Source code is included (it's a GPL project after all) in the package. I optimized a bit the C code (10% more speed ?), but is now less readable.

WarpEnterprises, if you feel like cheated on this, I will delete the attachment.

[WarpEnterprises]

Of course, keep it! My main intention was that there is working code that can be added to the core - and now there is :)

BTW mine is for 2.0 yours for 2.5

[Bidoche]

Damn ! Why all of you always want to do classes who do everything...

Here you should do one PClip subclass for each case (TurnLeftRGB, TurnRightRGB, TurnLeftYV12 ....)

Calling functions with a direction parameter and use it inside to choose between the two case is not the best option (not even a signle line of code shared between the two branch....)
Those choices should be done at the higher level possible, ie in the Create Function of Turn, and when we are at it, we divide into colorspace too...
It will make code much more understandable.

NB: don't take it for yourself, it's just the core is full of that and it makes things harder to understand.

[sh0dan]

@Bidoche: What difference does it make - it works, and the code is fairly clear?

@Kurosu:
Why do you test:

else if (vi.IsYV12()) {
if (vi.height%2) env->ThrowError("Turn: YUY2 data must have MOD2 height");
if (vi.width%2) env->ThrowError("Turn: YUY2 data must have MOD2 width");
TurnPlanFunc = TurnYV12;
}

YV12 will always be mod2 in both width and height.

But otherwise, it's very nice. I'll add the code ASAP.

[Kurosu]

@Shodan
Because I was first only accepting MOD8 resolution. What surprises me is that YV12 data can actually be processed, while I thought the U field data was interleaved, like this way:

U planar:
line 0 + line 2 U
line 1 + line 3 U
...

It seems to work fine, but it would have been a nightmare otherwise.

@Bidoche
I think the main problem of the new APIs are that no older plugin can be reused without a recompile. That's what keeps Gabest from building a public avs 2.5 textsub filter (afaik). In what I've seen from DCOM in DirectX, it would be pretty usefull for the plugin developpers.

@WarpEnterprises
Thanks. GPL at the work :)

[sh0dan]

@Kurosu: No - thank god it's not - you could actually use same loop for all three planes - but - again - there is no difference in the real world.

[Bidoche]

@sh0dan

I'd rather say: it works and it's relatively clear (in its case, some piece in the core are far uglier).
Anyway it's still a design flaw to consistantly test colorspace to branch when you could have done it in Turn::Create with some polymorphism.

And for now, the number or colorspace is not that big.
But imagine if there are 10 of them, what it'd be :
if (colorspace1) do process1;
else if (colorspace2) do process2;
else if....
else if (colorspace10) do process10;
or just a switch in the create calling diffent subclass (for code sharing we use inheritance)

The second will produce much more cleaner code (and save a recurrent test at the cost of a virtual table).
For maintaining large amount of code, readibility is important. Imagine you discover that Turn is buggy in RGB24, isn't it better to use the class list of VC6 to go to source rather that dig into the Turn global code searching for the RGB24 code path....

@Kurosu

What is the connection ? I don't get your point...
If you're saying that Turn is a valuable addition, I am totally ok with that.
In fact I am willing to make it VideoFrame method (with some others things) to make easier to plugins developpers to develop effects.


NB: I think maybe turn left and right can be done with the same code using either positive or negative pitch (should be checked)

[Kurosu]

@Bidoche

I didn't see your later post, and I therefore edited my post. I'll try to elaborate. The way the Avisynth API is now oblige the plugin developpers to offer their plugn recompiled for all major API changes, like AVS2.0x and AVS2.5. That's troublesome, as nothing in the filter has changed. Some developpers (like Gabest) have proposed some solution. Still, it's the plugin that tackle the problem, while I think it should be the environment/API/core. It saves some trouble to the plugin writers (see the perplexity of most of us about the name space thingie proposed by Gabest). And it offers a clean solution, once and for all, with no particular work or 'trick' to do by the plugin writer.

I take for example DirectX, where the interface is instanciated and refers to precise feature. If you want maximum compatibility, you can use the older interfaces, which will still be supported by any new API, but would lack some advanced features. But this way, DirectX 9 can still run DirectX 3 games for instance.

In the end, the way we write code depends on the way we master C++. I've learned how to write AVS plugin by reading source code from other people, and I'm only slowly getting some things that are subtilities for me. In the end, we don't really want to bother with them but rather spend time on optimizations/ additionnal features.

I could have written more specific functions, like TurnRightYUY2 , TurnLeftYUY2, ... but it only saves me a simple 'if' at the upper level of the functions (which doesn't hurt speed this way, I believe). I did it for the various colorspaces, because it was saving some bigger 'if' code blocks.

[Bidoche]

@Kurosu

I am not really aware of problems related to the API offered to exterior plugins. But I fear it was not really made easily extensible from the start....


Don't believe I go hunt plugins authors about their code not being that clear, but I would about core code (at least in 3.0)
And I understand that many learn by mimicking existing code so someone has to start giving the good example :p


And my point about moving the if upwards is:
your way : you make an if per GetFrame call
my way : you test only once in Create and then it's definitely branched ok.

[Kurosu]

@Sh0dan
About YV12 interlacing, then I have trouble understanding the corresponding page on avisynth.org documentation (in addition to my post being not very clear):
http://www.avisynth.org/index.php?page=ColorSpaces

@Bidoche
In the end, I have no idea how to work with the PClip subclasses. I think the function pointers handle this in a more Cish-way, and do the job. Doing what you suggest (at the higher level possible, still not the Create function), I would just add additionnal specific functions

For the negative pitch trick, it has also to work with the start and end values of the loop iterator. If it needs a non-fixed step (either +1 or -1), I think that keeping separate blocks for each case (Right or Left) is better.

[neuron2]

@Bidoche

I'm a C embedded systems programmer and not very familiar with C++ idioms. Can you please explain your idea with a simple example? I.e., how would I use polymorphism in the way that you described (instantiating different code in the Create)? Thank you.

[WarpEnterprises]

Yes, an example would be helpful.
This could help to elaborate the possible ways to deal with e.g. many colorspaces or sample sizes ...

[Bidoche]

Ok I will rewrite it.

[sh0dan]

Originally posted by Kurosu
@Sh0dan
About YV12 interlacing, then I have trouble understanding the corresponding page on avisynth.org documentation (in addition to my post being not very clear):
http://www.avisynth.org/index.php?page=ColorSpaces


Interlaced YV12 is not easy to understand.. I tried to do another illustration - did that help? What part of it is unclear?

[Kurosu]

Ok, then my plugin doesn't always do a proper job. This should be managed in in relation with VideoInformation.IsFieldBased() I think. Consider that 4x4 block:

Y11 Y21 Y31 Y41 \ line 1
U11/V11 U21/V21 /

Y12 Y22 Y32 Y42 \ line 2
U12/V12 U22/V22 /

Y13 Y23 Y33 Y43 \ line 3
U11/V11 U21/V21 /

Y14 Y24 Y34 Y44 \ line 4
U12/V12 U22/V22 /

Transpose it (as done in turn left), with C being chroma info (U and V):
Y41 Y42 Y43 Y44
C21 C22 C21 C22

Y31 Y32 Y33 Y34
C21 C22 C21 C22

Y21 Y22 Y23 Y24
C11 C12 C11 C12

Y11 Y12 Y13 Y14
C11 C12 C11 C12

I tried to avoid those considerations, but then...
- if still Field Based, with C = (C11+C12+C21+C22)/4 ((CODE tag used, otherwise it's unreadable):

Y41 Y42 Y43 Y44
C C
Y31 Y32 Y33 Y34
C C
Y21 Y22 Y23 Y24
C C
Y11 Y12 Y13 Y14
C C

That's a pretty horrible decimation. And it _is_ needed if fieldbased property is kept.
- if I change to Frame Based:

Y41 Y42 Y43 Y44
C'11 C'21=C'11
Y31 Y32 Y33 Y34
C'11 C'21=C'11
Y21 Y22 Y23 Y24
C'12 C'22=C'12
Y11 Y12 Y13 Y14
C'12 C'22=C'12

Quite better. That's what my filter assumes in fact. Strangely enough, some interlaced sequences aren't Field Based, while it would be cleaner to make it field based (but quite a pain to manage, especially for decomb, I fear, and my other plugin, TPRIVTC) for interlaced material. That's the case of the TNGsample.vob from Neuron2. And what had me make such a mistake.

Now consider Frame based input:

Y11 Y21 Y31 Y41 \ line 1
U11/V11 U21/V21 /

Y12 Y22 Y32 Y42 \ line 2
U11/V11 U21/V21 /

Y13 Y23 Y33 Y43 \ line 3
U12/V12 U22/V22 /

Y14 Y24 Y34 Y44 \ line 4
U12/V12 U22/V22 /

Transpose:
Y41 Y42 Y43 Y44
C21 C21 C22 C22

Y31 Y32 Y33 Y34
C21 C21 C22 C22

Y21 Y22 Y23 Y24
C11 C11 C12 C12

Y11 Y12 Y13 Y14
C11 C11 C12 C12

This ok, and doesn't cause any trouble.

For what the FAQ states, I was confused by your reply: I thought it was ok, but YV12 interlacing really is a problem, and makes the turn filter, as it is now, fails on this kind of material.

[sh0dan]

I can't quite follow you 100% ;)

How big is the penalty when invoking a separatefields before, and weave after your filter, if the clip is fieldbased?

[Bidoche]

Here is a modified turn.cpp.

It's incomplete but I think it should be enough to show my point

The main change is that it use different PClip subclass according cases.
I tried too to do both left and right in the same loop (hop it works).

[Kurosu]

@Sh0dan
I haven't tested that, and I do think it's the filter's job to take care of FieldBased video. What I tried to demonstrate is that if you keep the FieldBased property on the VideoInformation, you have to drop precision (the 4x4 block have to use only one chroma info (U,V couple) !). If not, this 4x4 block will have 2 chroma info (out of the 4 at first).

On YUY2 and YV12 pixel formats, the turn function losses precision. I think that a clip in YUY2 processed by TurnRight().TurnLeft() will have in the end as much chroma information as a YV12 sequence (TurnRight() blends horizontal chroma data of clip, then TurnLeft() blends horizontal chroma data of clip.TurnRight(), ie vertical data of clip).

@Bidoche
Once it's approved, I'll have a look at it, for sure.

[WarpEnterprises]

@Kurosu: color information is decimated for sure in YUY2/YV12, that was the reason I didn't even consider handling this as it invokes some sort of merging.

BTW does someone of you know a usage for turn ?

The only use so far is for turning imported bitmaps...

[Bidoche]

I have used it (vdub version) to insert vertical subtitles in kanjis.
And at that time, I was very annoyed to have to use vdub.

[sh0dan]

AVSValue __cdecl Create_TurnLeft(AVSValue args, void* user_data, IScriptEnvironment* env) {
PClip clip = args[0].AsClip();
switch(clip.GetVideoInfo.color_space)
{
case CS_RGB24: return new TurnRGB(clip, true, true);
case CS_RGB32: return new TurnRGB(clip, true, false);
case CS_YUY2: return new TurnYUY2(clip, true, env);
case CS_YV12: return new TurnYV12(clip, true, env);
}
env->ThrowError("Turn : invalid colorspace");
}


You should also test for I420 - in general I'd recommend using VideoInfo.isYV12(), etc - making cleaner and not making it prone to problems. I420 is transparent to the filters - and they should treat it just like YV12. This is why using the isYV12() is a good thing.

Actually making YV12 a general "planar" routine would be possible - it would (coded as it should) be able to support future planar formats. (ie. 4:4:4 or similar).

But it seems like some nice changes - could you finish it up, then I'll add it to the core.

[Bidoche]

Then we just add a case CS_I420 in the switch.

Using a switch rather than ISYV12() and cie with imbricated ifs is more readable (even if I didn't use specialised class ).

Even if I didn't make a planeTurn method now, it'll still be possible to move the right piece of code in the future.

[sh0dan]

Couldn't you just trigger the planar rotate if the video is planar, and otherwise do the switch for interleaved formats? Way cleaner and it'll work instantly, if we decide to support other planar formats.

A more subjective opinion is, that I don't think the internal filters should promote the usage of direct references to the colorspace constants.
Using the functions are IMO the cleanest way, as they can be extended, whereas the direct colorspace references cannot. I know it's a subjective matter - do what you feel is the best!

[Bidoche]

You mean that maybe one day, how colorspaces are defined may change.
I thought about that (for 3.0) and I believe it won't hurt if redefine them as class instance as long as they keep the same interface.
I am not sure switch work with classes through. Can someone fill in ?

[Bidoche]

Here is a new version, almost complete.
It's just missing YUY2 since I am not sure about turning it.
Maybe we should either forbid it (throw an error) or convert to YV12 in the process since it will have get the loss of precision anyway.

[Kurosu]

@Bidoche
In spite of the loss of data due to merging chroma information, turn should be available in YUY2. Although most people are encoding to YV12 (with an mpeg-4 encoder), and not that many codecs store data YUY2 (HuffYUV and ?), we should leave this possibility to the end-user. Converting to YV12 is even more lossy.

Concerning YV12, I think we're not over, in particular with the interlaced chroma, unless we use Sh0dan's trick (SeparateField+Weave). I didn't update the interface (ie, I haven't used Bidoche code), but here is a version (http://kurosu.inforezo.org/avs/turn.zip) with some basic attempts at dealing with it (yes it's _very_ ugly).

It makes me wonder about something: XviD support interlaced encoding. Would it be reported correctly by avisynth? I'm asking this because, I'd like to reencode an interlaced (yet frame based) content in xvid. The avi would be opened by avisynth as fieldbased, therefore enabling me to really test this small addition.

If dealing with it as framebased doesn't give as good results, then a test is needed in Turn::Create at the CS_YV12 and CS_I420 cases.

[EDIT]
Link modified (case sensitive, but with no capital letter)

[Bidoche]

For dealing with interlaced, my first assumption (that I made in 3.0 (I provide a Turn there at a per frame basis)) was to simply forbid it.

But if you say that it still can (and should) be done, I'd be happy to follow you.

Edit: Btw I failed to download your link

[Kurosu]

@Bidoche
- link modified
- I need to see the results on a trully fieldbased video (I like the knowledge about the field processing in avisynth to make a simple test) to see if it should be:
. forbidden (deinterlacing needed first, with decomb for instance)
. accepted

YUV2 works; why not offering the support and let the user choose if the chroma blending (not really decimation) is worth the turn ability? Then again, the user should consider what will be his output colorspace before applying turn:
- if RGB, then do a ConvertToRGB() first because YUY2 and YV12 interlaced are processed in a lossy way
- if YV12, converting at the very start to YV12 seems obvious

btw, the YV12 frame version does chroma and luma in the same loop. I haven't tested speedwise if it's smarter (some kind of loop unrolling).

[Bidoche]

@Kurosu

After a YUY2 Turn we will have the same chroma for each four pixels block (tell me if I am wrong).
What difference to YV12 then ?
Maybe then the user could be offered the choice between YUYV=2 and YV12 output.

I don't know if it's better speedwise to do both U and V in the same loop.
But it's certainly better strutural-wise and lazyness-wise to use the same loop for each.

[Kurosu]

About YUY2: yes, only one (U,v) couple is left for each 2x2 pixels block, therefore the same information as the same video stream in YV12. But this way, it's still YUY2 data. I wonder what the speed penalty is to do a ConvertToYV12().Turn() over a Turn() "YUY2->YV12". It involves both memory allocation and memory through output. But is a special Turn() that automatically does YUY2->YV12 conversion really worth it?

About the YV12 framebased version: I mean, Y,U and V are done at the same time, this way:
move four luma bytes
move one U byte
move one V byte

I'll try to test all the possible types of loop speedwise (I can't profile, thus I can only use results from QueryPerformanceCounter())

[Bidoche]

It's probaly faster if all the planes can fit into the cache, otherwise really slower.