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

[WIP] adding a few API functions #239

Merged
merged 6 commits into from Dec 1, 2021
Merged

[WIP] adding a few API functions #239

merged 6 commits into from Dec 1, 2021

Conversation

qyot27
Copy link
Member

@qyot27 qyot27 commented Oct 20, 2021

Currently only the C interface, and I don't know if the logic of the implementations needs to check for more than it does. I did test the result using a patched avs2yuv which showed that the functions do seem to pass what they need to.

One thing to note about the endian check is that C++20 has std::endian, but I would strongly advise against using that for the C++ interface because we would no longer be able to support GCC 7 without the use of another external implementation like the filesystem supports (and I don't know of any such project for std::endian), and for a couple of OSes, GCC 7 is really the only option.

@pinterf
Copy link

pinterf commented Oct 21, 2021

Why is endianness needed? Isn't it a "full support"/"no support at all" question for a specific platform?

@qyot27
Copy link
Member Author

qyot27 commented Oct 21, 2021

Endianness is meant more like a convenience function so that if behaviors need to be switched based on it, it doesn't require using #defines directly. It was really brought to my attention due to an issue with FFmpeg when I was trying to figure out why audio wasn't working correctly on the iMac G5 - FFmpeg (and thus FFMS2) decodes audio in whatever the native endianness is, AviSynth+ serves audio in whatever the native endianness is, but the demuxer in FFmpeg was hardcoded to assume it was always getting little-endian audio, which required the fix shown there. Unlike with the video pixel formats, FFmpeg's audio formats don't have a generic endian-agnostic form that expands out to whichever one is appropriate for that platform.

Some platforms are strictly little endian or big endian, but others aren't. Windows: always little endian. x86: always little endian. PowerPC Macs: always big endian, but POWER8 introduced a little endian mode, and so 'ppc64' and 'ppc64le' are separate build targets. Linux on POWER8 or higher (like on an RCS Talos II, or in qemu) can therefore be either big or little endian. SPARC: always big endian. ARM: usually little endian, but officially supports both, and you can actually boot the Raspberry Pi 0-3 into big endian mode with the appropriate OS (NetBSD actually does support this, I'm not sure if that bit about ACPI-CA regarding the Pi4 is a hard limitation that can't be overcome, although it appears the upstream is working on it). I have no clue about whether RISC-V could be in the same boat as ARM.

And some of those platforms that support both ostensibly can work with the type opposite the OS from within it, or at least that's how some of the article write ups make it sound, so a compile-time check might not be the way plugins or clients want to handle the behavior.

@pinterf
Copy link

pinterf commented Nov 18, 2021

So -std=c++1z is for old gcc 7 and clang), which didn't know at the time of their existence the exact name of the next standard (c++17). Still, they compile c++17 dialect properly.

@pinterf
Copy link

pinterf commented Nov 18, 2021

This versioning is the future. So we can go on with it. Probably the similar counterparts should appear in the cpp interface as well.

Obviously this serves us to ease the detection of version-dependent interface changes and validity. But until then the plugins or softwares - if they decide to support pre 3.8 (IF <= 8) Avisynth (either C or C++) - will probably still use the exception oriented interface number checking method for checking IF version <8 and 8 - as every frame-property aware plugin works now. (The other method for C interface using softwares is to detect if a specific DLL/so function has an entry point or not,)

@qyot27
Copy link
Member Author

qyot27 commented Nov 18, 2021

I tried to implement C++ versions of the new checks a few days ago, but failed since I have a practically non-existent grasp of C++. So I won't be able to do that part.

@pinterf
Copy link

pinterf commented Nov 27, 2021

I was just thinking of why we can't use env->GetEnvProperty (CPP, AvsEnvProperty enum) or avs_get_env_property (C, AVS_AEP_xxx enums) for getting endianness, avisynth interface version and bugfix version info?
It's there since V8 both on CPP and C and can easily be extended.

Then we need no extra entry points on either interface, just introduce new constants to identify the request. Works fine for cpp and c interface automatically.

The rule would be for getting interface version

  • less than v8? usual EnvironmentCheck
  • (exception/error): less than v8
  • no -> call env->GetEnvProperty (avs_get_env_property ) with constant e.g. GET_HOST_INTERFACE_VERSION or GET_C_INTERFACE_VERSION, GET_ENDIANNESS
  • (exception/error): this kind of request not supported -> we are at V8 interface which did not support this
  • success: we have the requested number (version, endianness)
// AvsEnvProperty for avs_get_env_property
enum
{
  AVS_AEP_PHYSICAL_CPUS = 1,
  AVS_AEP_LOGICAL_CPUS = 2,
  AVS_AEP_THREADPOOL_THREADS = 3,
  AVS_AEP_FILTERCHAIN_THREADS = 4,
  AVS_AEP_THREAD_ID = 5,
  AVS_AEP_VERSION = 6,

  // Neo additionals
  AVS_AEP_NUM_DEVICES = 901,
  AVS_AEP_FRAME_ALIGN = 902,
  AVS_AEP_PLANE_ALIGN = 903,

  AVS_AEP_SUPPRESS_THREAD = 921,
  AVS_AEP_GETFRAME_RECURSIVE = 922
};

@qyot27
Copy link
Member Author

qyot27 commented Nov 27, 2021

Yes, that does seem to work (albeit with a bit of type casting). I'll need to clean up the commits a little to compensate for the change.

@pinterf
Copy link

pinterf commented Nov 27, 2021

Choose fine names as you want. As I see other properties has no GET_ part.

@qyot27
Copy link
Member Author

qyot27 commented Nov 27, 2021

I've retooled the PR and force-pushed, but I think due to Github's outage just now it's not picking it up immediately; hopefully it will after a few hours. I've tested against both avs2yuv (with the C interface) and the C++ test** from your post in issue 130, and it works correctly.

**just adding these three lines after the fprintf "After CreateScriptEnvironment" line:

  fprintf(stdout,"Endianness: %s\r\n", (const char*)env->GetEnvProperty(AEP_HOST_SYSTEM_ENDIANNESS));
  fprintf(stdout,"Interface version: %ld\r\n", env->GetEnvProperty(AEP_INTERFACE_VERSION));
  fprintf(stdout,"Interface bugfix version: %ld\r\n", env->GetEnvProperty(AEP_INTERFACE_BUGFIX));

@DJATOM
Copy link

DJATOM commented Nov 28, 2021

@qyot27 I have made a write access to my avs2yuv repo, you can push stuff directly if needed.

@qyot27
Copy link
Member Author

qyot27 commented Nov 28, 2021

I know, those were just kind of demo commits showing the checks in action. I'd probably have it squashed or tidied up/expanded if or when I'd push it to the main repo.

Speaking of using frame properties in avs2yuv, can y4m store colorimetry, chroma location, primaries, etc.? It doesn't look like those get preserved when ffmpeg tries to pipe it (or nut, for that matter), but for all I know that could just be an implementation issue.

@DJATOM
Copy link

DJATOM commented Nov 28, 2021

It can't, as afaik y4m doesn't support extra metadata per frame basis. All I can think of is an external json file and filter that imports such metadata from json.

@pinterf
Copy link

pinterf commented Nov 28, 2021

At least one interface change (addition) will be needed, so V9 will be necessary to introduce. (not related to the changes made above)

@pinterf
Copy link

pinterf commented Dec 1, 2021

I pushed some new commits, partially with a two new interface functions.
I have not increased IF version number yet, nor pulled the NEW_AVSVALUE changes from you.
I'm seeing the end of the tunnel, so there are really not much things left on my side, so your new IF versioning and endianness modification could be pushed as well. Then we'd increase the actual IF number from 8 to 9 when make a release (3.8? decide)
I'd like to have feedback on test30 (Expr lut and other big code clean changes), but then probably before Xmas we could release something. A proper changelog must be assembled till then, and a Wiki refresh.

Denotes situations where there isn't a breaking change to the API,
but we need to identify when a particular change, fix or addition
to various API-adjacent bits might have occurred.  Could also be
used when any new functions get added.

Since the number is modelled as 'changes since API bump' and
intended to be used in conjunction with checking the main
AVISYNTH_INTERFACE_VERSION, whenever the main INTERFACE_VERSION
gets raised, the value of INTERFACE_BUGFIX should be reset to zero.

The BUGFIX version is added here with already incremented once,
both because the addition of AVISYNTH_INTERFACE_BUGFIX_VERSION
itself would require it, but also because it's intended to signify
the fix to the C interface allowing frame properties to be read
back (which was the situation that spurred this define to exist
in the first place).
Populated by 'little', 'big', or 'middle' based on what GCC and/or
Clang report at compile time.
While we do need the compiler to support C++17 features, we can
get by on older GCC using CMake 3.6 and -std=c++-1z with some
other fixes.

CMAKE_CXX_STANDARD can be raised intelligently to 17 based on
whether we detect CMake 3.8 or higher.
NEW_AVSVALUE has been unconditionally enabled on all platforms for
some time now, so just treat it as normal code.  A few comments
still refer to the value, but as other comments explicitly state
what NEW_AVSVALUE was for originally, that context is probably
enough.
@qyot27 qyot27 merged commit 2103827 into AviSynth:master Dec 1, 2021
@qyot27 qyot27 deleted the endian branch December 1, 2021 17:32
@pinterf
Copy link

pinterf commented Dec 1, 2021

I'm writing this example in the readme:
Is it the proper way the properties would be used? (Have not tried yet, code can contain errors)

    CPP interface:
      int avisynth_if_ver = 6;
      int avisynth_bugfix_ver = 0;
      try { 
        avisynth_if_ver = env->GetEnvProperty(AEP_INTERFACE_VERSION); 
        avisynth_bugfix_ver = env->GetEnvProperty(AEP_INTERFACE_BUGFIX);      
      } 
      catch (const AvisynthError&) { 
        try { env->CheckVersion(8); avisynth_if_ver = 8; } catch (const AvisynthError&) { }
      }
      has_at_least_v8 = avisynth_if_ver >= 8; // frame properties, NewVideoFrameP, other V8 environment functions
      has_at_least_v8_1 = avisynth_if_ver > 8 || (avisynth_if_ver == 8 && avisynth_bugfix_ver >= 1); // C interface frameprop access fixed
      has_at_least_v9 = avisynth_if_ver >= 9; // IsPropertyWritable/MakePropertyWritable support, extended GetEnvProperty queries

    C interface:
      AVS_ScriptEnvironment *env = ...
      int avisynth_if_ver = 6; // guessed minimum
      int avisynth_bugfix_ver = 0;
      int retval = avs_check_version(env, 8);
      if (retval == 0) {
        avisynth_if_ver = 8;
        // V8 at least, we have avs_get_env_property but AVS_AEP_INTERFACE_VERSION query may not be supported
        int retval = avs_get_env_property(env, AVS_AEP_INTERFACE_VERSION);
        if(env->error == 0) {
          avisynth_if_ver = retval;
          retval = avs_get_env_property(env, AVS_AEP_INTERFACE_BUGFIX);
          if(env->error == 0)
            avisynth_bugfix_ver = retval;
        }
      }
      has_at_least_v8 = avisynth_if_ver >= 8; // frame properties, NewVideoFrameP, other V8 environment functions
      has_at_least_v8_1 = avisynth_if_ver > 8 || (avisynth_if_ver == 8 && avisynth_bugfix_ver >= 1); // C interface frameprop access fixed
      has_at_least_v9 = avisynth_if_ver >= 9; // IsPropertyWritable/MakePropertyWritable support, extended GetEnvProperty queries

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

Successfully merging this pull request may close these issues.

None yet

3 participants