vapoursynth / vapoursynth

A video processing framework with simplicity in mind
http://www.vapoursynth.com/
GNU Lesser General Public License v2.1
1.64k stars 151 forks source link

API4's modified constant values causing problems with api3 filters #726

Closed AkarinVS closed 3 years ago

AkarinVS commented 3 years ago

API4 modified some interface constants, e.g. https://github.com/vapoursynth/vapoursynth/blob/b335771beb4d79d1d3f638ae1996ca87d83a214d/include/VapourSynth4.h#L80-L84 vs. https://github.com/vapoursynth/vapoursynth/blob/b335771beb4d79d1d3f638ae1996ca87d83a214d/include/VapourSynth.h#L80-L84

And these constants are also exposed to the Python API, which means that simple filter uses like: https://github.com/AmusementClub/mvsfunc/blob/0b2c6a84de789d9483762fa88a813680f151980c/mvsfunc.py#L473

    clip = core.fmtc.matrix(clip, mat=matrix, fulls=fulls, fulld=fulld, col_fam=vs.RGB)

won't work for filters (fmtc.matrix here) still using the v3 API.

This is serious issue, as API4 VS is supposed to be interoperable with v3 plugins to ease the transition and it's also possible that some filter might never get native API4 support. It's important to ensure API compatibility on those exposed constants.

Similarly, every single use of format constant is going to be affected by this issue.

I don't think there is a simple solution to this problem while keeping those different constant values. Detecting the values and transforming them on the fly when an api3 filter is used is going to be very fragile and error-prone.

What should we do? Requiring a hard transition like the Python 2 -> Python 3 transition will definitely not work as experience tells us that the user will just keep on using api3 VS forever (e.g. I still have actively used Python 2 scripts to this day....)

Thanks.

sekrit-twc commented 3 years ago

This one is nasty. Two possible solutions:

  1. Revert v4 constants. Divide by 1000000 when packing VSColorFamily into VSPresetFormat
  2. Add legacy v3 constants to vspy and use them in scripts.
AkarinVS commented 3 years ago

I think the only viable solution is to make all exposed equivalent constants (color space, integer/float, preset formats) equal between the two API versions.

But this is a very significant change, especially for the preset formats, and will mean all api4 plugin must be rebuilt.

Just changing the constant exposed to Python won't help, as it will break v4 filters that take a format constant from argument and internally pass it directly to the v4 API.

myrsloik commented 3 years ago

This is only a problem for the cf constants, all pf ones get laundered through the API and fixed magically and I don't think a single filter is broken due to it. It's also trivial to add compatibility handling for only one specific filter to rewrite an argument. Or we could ask the fmtconv author to sneakily check for both. See ShufflePlanes as an example: https://github.com/vapoursynth/vapoursynth/blob/doodle1/src/core/simplefilters.cpp#L561

EleonoreMizo commented 3 years ago

I added some internal wrapping functions in fmtconv so both new and old constants will be accepted from the next release (r24). https://github.com/EleonoreMizo/fmtconv/commit/eb847d926a28ee1bd1c1b089cbab5118954c12cc

AkarinVS commented 3 years ago

If the only issue is the color space constants, why not make api4 use the same constant as api3? Then existing filters don't need to be changed at all.

Requiring such changes will still make the transition harder than necessary as not all filters have source available.

myrsloik commented 3 years ago

If the only issue is the color space constants, why not make api4 use the same constant as api3? Then existing filters don't need to be changed at all. Requiring such changes will still make the transition harder than necessary as not all filters have source available.

All (1) filters have now been fixed. I changed the constants because I want formats to be easily serializable into a single uint32 with only 4 bits reserved for colorspace. This problem is over.