Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ConvertToRGB bug #282

Open
Reel-Deal opened this issue Apr 4, 2022 · 18 comments
Open

ConvertToRGB bug #282

Reel-Deal opened this issue Apr 4, 2022 · 18 comments

Comments

@Reel-Deal
Copy link
Member

I'm back again with bug reports :)... I'm working on the Convert page and trying to make sense of it since the way it is laid out in the wiki is a little confusing. I found a bug with the ConvertToRGB filter:

ColorBars(pixel_type="RGBP12") # can also be any YUV4xxP10-14/32
ConvertToRGB()

The output is only correct for 8 or 16 bit. For intermediate bit depths or float it produces a black (for rgb) or green (yuv) image, colorbars are faintly visible. Oddly ConvertToRGB/24/32/48/64 work fine with any color format and bitdepth.

@pinterf
Copy link

pinterf commented Apr 4, 2022

Thanks for the report (it seems this week is still off for me)

@pinterf
Copy link

pinterf commented Apr 19, 2022

ConvertToRGB converts to packed RGB target.

The input bit depth check was probably developed at the very beginning of the high bit depth changes when only 8 and 16 bit planars were supported (though YUV 32bit is checked).
We need to check for exact 8 or 16 bit source for ConvertToRGB(). For float obviously there is no packed target, nor have 10-14 bit formats.

Original wiki:
"Note ConvertToRGB always converts to RGB32 – unless your source is RGB24, in which case no conversion is done."

Expected rule:
"Note ConvertToRGB always converts to RGB32 (8 bit) or RGB64 (16 bit) – unless your source is RGB24/RGB48, in which case no conversion is done. Other than 8 or 16 bit input formats result in error"

Conclusion (planned change log):

  • Fix ConvertToRGB (the function): do check for exact 8 or 16 bit input, because packed RGB formats exist only for 8 and 16 bits

EDIT: this turned out as well, planar RGBA was converted always to RGB24/48.

  • keep alpha for RGBA planar - convert RGBAP8/16 to RGB32/64, while RGBP8/16 is still RGB24/48,

@Reel-Deal
Copy link
Member Author

Hi pinterf, sounds good, I will make those notes in the documentation. Another thing that I noticed is that The YUV to RGB conversions (when using ConvertTo48/64) are always done at the current bitdepth of the source. For example, lets say you have a YUV420P8 source and then use ConvertToRGB64, from what I can tell is that the RGB conversion is done in 8-bits and then converted to 16-bit at the end. In this case, would it not be better to first to do the YUV to RGB conversion in 16 bits for any source that is 8-14 bits? This is only applicable to the ConvertToRGB48/64 filters when converting from a lower bitdepth and from YUV. When going from a higher bitdepth YUV source to rgb24/32 the bitdepth conversion happens at the end, so it's a better situation there. And of course all of the other planar ConvertToYUV/RGB filters do not do any bitdepth conversion so I documented that as well, and the rest of the legacy filters are limited to 8-bit I/O.

pinterf added a commit that referenced this issue Apr 19, 2022
@pinterf
Copy link

pinterf commented Apr 19, 2022

3.7.1 test1, see readme.
https://drive.google.com/uc?export=download&id=1Y__F8C9d4ghip2VqAfmtbz-q21PmOUR2
(No conversion order change yet.)

@Reel-Deal
Copy link
Member Author

Thanks pinterf, that fixes that issue. I have another observation which I'm not sure about. In v3.7.1, the ChromaOutPlacement was added to ConvertToYV16/YUV422 but later on we agreed that for YUV422 the chroma placement should always be MPEG2 (left) - issue #260. Yet ChromaOutPlacement still has an effect on ConvertToYV16/YUV422:

ColorBars()
mpeg1 = ConvertToYUV422(ChromaOutPlacement="MPEG1")
mpeg2 = ConvertToYUV422(ChromaOutPlacement="MPEG2")

diff(mpeg1,mpeg2)

Function Diff(clip src1, clip src2)
{
return Subtract(src1.ConvertBits(8),src2.ConvertBits(8)).Levels(120, 1, 255-120, 0, 255, coring=false)
} 

@pinterf
Copy link

pinterf commented Apr 27, 2022

This commit: 70511e6 allowed only 'top' and 'bottom' prefixed 'left': top_left and bottom_left was accepted as well and behaves like left.
'center' (mpeg1) is still a valid choice.

@kedaitinh12
Copy link

3.7.1 test1, see readme. https://drive.google.com/uc?export=download&id=1Y__F8C9d4ghip2VqAfmtbz-q21PmOUR2 (No conversion order change yet.)

your build x86 xp ver error with crabshank_filters but work with asd-g's avs+ normally. I think error from x86 ver of your only support xp??? I think you need add support newer ver in next builds
https://gitlab.com/uvz/AviSynthPlus-Plugins-Scripts/-/issues/3

@pinterf
Copy link

pinterf commented May 28, 2022

No. It just supports xp or better. So xp build is good for everyone.

@kedaitinh12
Copy link

So why they meet error with crabshank_filters???

@pinterf
Copy link

pinterf commented May 31, 2022

First I checked that all recent AviSynth+ x86 versions (with XP support or no XP, 3.7.1 3.7.2, 3.7.3) results in platform error 127 (dll could not be loaded) on my machine.

Looking into the actual DLL binary file (e.g. auto_gamma.dll) I can see that it was built with Mingw and it is a C (not cpp) plugin,
The

Then by using a proper dependency viewer GUI it looks that the plugin (auto_gamma.dll from crabshank_filters) wants to load all Avisynth functions with a "decorated" function name.

This plugins wants to access these AviSynth functions

_avs_add_function@20
_avs_get_frame@8
_avs_get_height_p@8
_avs_get_pitch_p@8
_avs_get_read_ptr_p@8
_avs_get_row_size_p@8
_avs_get_write_ptr_p@8
_avs_is_rgb64@4
_avs_make_writable@8
_avs_new_c_filter@20
_avs_release_clip@4
_avs_set_to_clip@8

But the proper usage of them is to give their name without the name mangling:

avs_add_function
avs_get_frame
avs_get_height_p
avs_get_pitch_p
avs_get_read_ptr_p
avs_get_row_size_p
avs_get_write_ptr_p
avs_is_rgb64
avs_make_writable
avs_new_c_filter
avs_release_clip
avs_set_to_clip

Important note that before 3.7.2 the extra AviSynth+ C function names were (erronously) "decorated".
Example:
Version < 3.7.2: _avs_is_rgb64@4 (wrong - it was a bug)
Version >= 3.7.2: avs_is_rgb64 (O.K., there is no mangling like at all other functions

Summary:
Mixed name decorations of the C interface was fixed in 3.7.2.
C plugins that would interface with official Windows Avisynth+ releases must access the functions with the undecorated names and they will work properl after Avisynth+ 3.7.2 even if the newer HBD specific or frame property related functions are used.

@kedaitinh12
Copy link

Oh, i found asd-g's build with clang now support crabshank_filters x86 ver now
https://gitlab.com/uvz/AviSynthPlus-Builds

@Asd-g
Copy link

Asd-g commented Mar 1, 2023

First I checked that all recent AviSynth+ x86 versions (with XP support or no XP, 3.7.1 3.7.2, 3.7.3) results in platform error 127 (dll could not be loaded) on my machine.

Looking into the actual DLL binary file (e.g. auto_gamma.dll) I can see that it was built with Mingw and it is a C (not cpp) plugin, The

Then by using a proper dependency viewer GUI it looks that the plugin (auto_gamma.dll from crabshank_filters) wants to load all Avisynth functions with a "decorated" function name.

This plugins wants to access these AviSynth functions

_avs_add_function@20
_avs_get_frame@8
_avs_get_height_p@8
_avs_get_pitch_p@8
_avs_get_read_ptr_p@8
_avs_get_row_size_p@8
_avs_get_write_ptr_p@8
_avs_is_rgb64@4
_avs_make_writable@8
_avs_new_c_filter@20
_avs_release_clip@4
_avs_set_to_clip@8

But the proper usage of them is to give their name without the name mangling:

avs_add_function
avs_get_frame
avs_get_height_p
avs_get_pitch_p
avs_get_read_ptr_p
avs_get_row_size_p
avs_get_write_ptr_p
avs_is_rgb64
avs_make_writable
avs_new_c_filter
avs_release_clip
avs_set_to_clip

Important note that before 3.7.2 the extra AviSynth+ C function names were (erronously) "decorated". Example: Version < 3.7.2: _avs_is_rgb64@4 (wrong - it was a bug) Version >= 3.7.2: avs_is_rgb64 (O.K., there is no mangling like at all other functions

Summary: Mixed name decorations of the C interface was fixed in 3.7.2. C plugins that would interface with official Windows Avisynth+ releases must access the functions with the undecorated names and they will work properl after Avisynth+ 3.7.2 even if the newer HBD specific or frame property related functions are used.

@pinterf, when clang-cl+lld-link or icx(it uses lld-link) is used for building avs+ x86, both decorated and undecorated functions are exported (for example, _avs_add_function@20 and avs_add_function) and the decorated functions are first (ordinal).
So when one have clang-cl+lld-link/icx build he can load plugins with both decorated and undecorated functions.
If one have msvc build he can load only plugins with undecorated functions.

When a plugin using C api is linked against avisynth.lib built with clang-cl/icx it expects decorated functions (ordinal lld-link place them before undecorated).

Obviously link.exe and lld-link.exe behaves differently with .def files. Maybe include also decorated functions for msvc too?

Here for example the exported functions from avisynth.dll built with clang-cl+lld-link -
funcs.txt

@pinterf
Copy link

pinterf commented Mar 1, 2023

Like in e.g. assrender, where I have to put alternative names as well? I think this was needed in order to classic Avisynth 2.6 recognize the plugin.
assrender.def:

LIBRARY assrender
EXPORTS
	avisynth_c_plugin_init@4 = _avisynth_c_plugin_init@4

@pinterf
Copy link

pinterf commented Mar 1, 2023

This one seems to do the task for MSVC + x86, but the actual avs_copy_frame_props (and all the other functions) must be removed from avisynth.def .

extern "C"
void AVSC_CC avs_copy_frame_props(AVS_ScriptEnvironment * p, const AVS_VideoFrame * src, AVS_VideoFrame * dst)
{
#if defined(X86_32) && defined(MSVC_PURE)
  #pragma comment(linker, "/EXPORT:" __FUNCTION__ "=" __FUNCDNAME__ )
#endif

EDIT:
With the undecorated functions removed from avisynth.def unfortunately

  • clang cl build no longer included undecorated names
  • the above pragma trick is not working

It seems that putting undecorated function names in avisynth.def (as now)

  • (clang cl) adds the undecorated names besides the decorated ones
  • (msvc) only the undecorated names will exist, decorated names are removed

The MSVC+X86 pragma in avisynth_c.cpp works only if there are no avisynth.def entries (warning:, there can only one exist, linker will take the first occurance) but this is not possible because of clang - or we have to have two sets of avisynth.def - one for msvc and one for clangcl?? - I think this is no go.

(This is the possible solution that I wanted to avoid)
The only common thing that works for both MSVC and clang cl build is declaring dual function names in avisynth.def; msvc by default generates decorated names as well. Like this:
avs_video_frame_get_pixel_type = _avs_video_frame_get_pixel_type@4

@Asd-g
Copy link

Asd-g commented Mar 2, 2023

Yes, avs_video_frame_get_pixel_type = _avs_video_frame_get_pixel_type@4 in def file or /export:"avs_video_frame_get_pixel_type=_avs_video_frame_get_pixel_type@4 in avisynth_c.cpp (or as linker option) make the exported functions to be the same for both msvc and clang-cl.

@pinterf
Copy link

pinterf commented Mar 2, 2023

I'm gonna go ahead with the extending the def file. I couldn't check how mingw behaves; I've seen that CMakeList.txt is using .def for mingw as well, but I was not able to figure out how to build with a 32 bit mingw.

@Asd-g
Copy link

Asd-g commented Mar 2, 2023

I guess you mean mingw 32-bit with built-in gcc and not clang-cl/cl? I can try to build

pinterf added a commit that referenced this issue Mar 3, 2023
It helps mingw32 C plugins to see decorated names
@qyot27
Copy link
Member

qyot27 commented May 2, 2023

...Several months later, because I'm in the process of doing a refresh of my build guide...

Yeah, it breaks compilation of AviSynth+ under 32-bit MinGW-GCC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants