pinterf / AviSynthPlus

AviSynth with improvements
http://avs-plus.net
208 stars 24 forks source link

Expr: crash when YV12 -> RGBP* #15

Closed magiblot closed 5 years ago

magiblot commented 6 years ago

Hi, I have found the following to cause a crash instead of throwing an error:

#Source colorspace: YV12
Expr(last,"x 255.0 /","x 255.0 /","x 255.0 /",format="RGBPS") # or any other RGB bitdepth

I guess because input planes U, V have a different width and height (in pixels) than the output G, B planes?

In addition, imagine my intention was to process the source Y channel for each output RGB plane, instead of processing U and V. Isn't there any way to do so?

Anyway, thank you, pinterf. This is a very great function.

pinterf commented 6 years ago

Thanks, fixed in https://github.com/pinterf/AviSynthPlus/commit/b471465fac1ab800e205452515594db85fb026b5. Now it gives error message when output clip format has different subsampling factor. For plane conversion use CombinePlanes: http://avisynth.nl/index.php/CombinePlanes

magiblot commented 6 years ago

Thanks.

Regarding the question I asked you, CombinePlanes makes a whole copy of the video with the planes rearranged. I thought plane selection (e.g. x.Y) in Expr could optimize this a bit. In addition, if I were to use the Y plane as input for each output plane, the same memory address would be read each time, and this could improve cache efficiency (but I don't know how Expr works internally, so these assumptions might be wrong).

Also, I noticed the following:

#AviSynth+ r2722
v1 = Expr("255","0","0",format="RGBP") #Output looks green
v2 = Expr("0","255","0",format="RGBP") #Output looks blue
v3 = Expr("0","0","255",format="RGBP") #Output looks red

Is this un-intuitive plane order a bug or an undocumented feature? (I could not find any related information in http://avisynth.nl/index.php/Expr)

Cheers.

pinterf commented 6 years ago

Feature. Internally planar RGB plane order is G, B and R. That's why you see the different order. Yes, I'd lean to the more conveniant R G B order (I have to change two letters in the source) but don't know if others in the community already have used GBR order. Perhaps you could ask it at doom9. I guess, since using Expr for planar RGB is still a power user function, not much people used it.

Regarding the other question: what you want is quite a special case - one plane as input, split into three results. So you have to use existing tools. As I experienced it's never a plane copy which makes bottlenecks. CombinePlanes does internal optimization, if it can it avoids plane copy and just rearranges internal plane pointers

pinterf commented 6 years ago

Originally this filter came from Vapoursynth where plane order for planar rgb is r=0, g=1, b=2 so the order of expressions is similar. So I consider this gbr behaviour as a bug and will be corrected.

magiblot commented 6 years ago

Lots of thanks. All my questions have been answered.

magiblot commented 6 years ago

Well, not actually. I am indeed making a filter which performs the HSV to RGB conversion route and takes the Y plane of the source video as brightness (V), then splits it into each RGB channel. Before the last release (r2728) I was allowed to do the following:

#c is a planar YUV video
Y = Expr(c, limiter_expr, format="Y32") #extends to full range and converts to Y32 in a single step
RGB = Expr(Y, "x", "x 2.0 /", "x 3.0 /", format="RGBPS") #a single Y plane is used as input  for each RGB output plane, again in a single step.
#Obviously the expressions in a real use case would be more complex

However, in the last release, each call to Expr in the code above vulnerates the new rule Expr: inputs and outputs must have the same number of planes. So I used CombinePlanes to comply with the new defaults:

d = CombinePlanes(c,planes="Y",source_planes="Y",pixel_type="Y"+String(bitDepth))
Y = Expr(d, limiter_expr, format="Y32")  #I believe the new parameters of Expr can do this automatically
YYY = CombinePlanes(Y,Y,Y,planes="RGB",source_planes="YYY",pixel_type="RGBPS")
hsvRGB = Expr(YYY, "x", "x 2.0 /", "x 3.0 /", format="RGBPS")

But the rendering speed dropped from 40 fps to 35 with a 1080p source video.

Would you approve to make this rule less restrictive?

pinterf commented 6 years ago

The new check is OK, but it seems that it could be made less strict: the number of planes in each input clips should be above or equal than of output clip. That will allow Y = Expr(c, limiter_expr, format="Y32") But did this one really work? RGB = Expr(Y, "x", "x 2.0 /", "x 3.0 /", format="RGBPS") #a single Y plane is used as input for each RGB output plane, again in a single step. Edit: Could you please test this build: https://drive.google.com/open?id=15OrDP7j_smFSM0sAqufrTo1QKreV0UR0

magiblot commented 6 years ago

Thanks. It works wonderfully. The rendering speed got back to 40 fps, I am happy now.

I also tried to reproduce crashes with weird combinations. In the Y->YV12 case I properly got the message Expr: output must not be a subsampled format for Y-only output(s). It worked well with e.g. Y->YV24.

Lots of thanks.

pinterf commented 6 years ago

O.K. The final description will be like that (look at the multi Y clip usage in the last example):

magiblot commented 6 years ago

It's very clearly explained. Thank you very much.

PS.: It could be noted that the second point doesn't apply when the output format is sub-sampled.