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

A couple of little requests for new Rust crate that's a basis for plugins #314

Closed
Enyium opened this issue Jan 25, 2023 · 1 comment
Closed

Comments

@Enyium
Copy link

Enyium commented Jan 25, 2023

Hi,

I wrote a Rust crate that can be used to write AviSynth+ plugins in Rust. Before publishing, I'd like to clear up some things, some of which brought me into a legal gray area license-wise, I'd guess (because I'm currently temporarily forced to duplicate a bit of code 1:1 for technical reasons, and I'm accessing a private class member, which isn't necessarily "solely... the interfaces defined in avisynth.h").

Getting type letter from AVSValue

Rust works with pattern matching. The AVSValue methods IsClip(), IsBool(), IsInt() etc. already check the internal type letter member short type to be a certain letter. My Rust code needs to find out the exact type of an AVSValue, not just whether it's suitable for certain type-related actions. For that, I need to be able to directly retrieve the letter instead of unnecessarily calling all Is...() functions, who in turn do letter checks.

Currently I'm doing this hack (C++):

int16_t AVSValue_TypeLetter(const AVSValue& self) {
    short* type_member = (short*)(&self); // First member.
    return *type_member;
}

The letters aren't a secret anyways, since they're used in parameter type strings in IScriptEnvironment::AddFunction(). IScriptEnvironment::propGetType() also returns letters.

You could argue that, even though type is a short, because of its usage in AddFunction(), it's unlikely to ever contain a value other than an ASCII character. So, the new AVSValue method could return a char. However, there's also the precedent of propGetType(), which returns an enum that will be of type int. So, it could also be an enum, this time possibly with the type constrained to short or char.

VideoFrameBuffer descructor should be public

The descructor of VideoFrameBuffer is protected. My crate uses CXX, which does code generation, and the destructor not being public gives me a C++ compiler error when trying to call GetWritePtr() (I want to call the VideoFrameBuffer, not the VideoFrame method). VideoFrame, IScriptEnvironment and IJobCompletion also have public destructors without having public constructors.

All enums need names

CXX checks enums that come from C++ for correctness (see here). This works with C++ enum and enum class, provided there's a name. Since some enums in avisynth.h miss a name, I currently have copies of them in my C++ code that I gave a name and where I turned enum to enum class to not have conflicts.

I suggest defining new enum names with a prefix, probably Avs, which is already used by some enums. Unfortunately, avisynth.h isn't consistent and also uses the prefix AVS or none at all. (This could probably be improved by giving them consistent names and providing compat typedefs with the old names, BTW.)

In my code, I named the enums I needed: AvsSampleType, AvsPlane, AvsImageType, AvsColorSpace. I derived ...ColorSpace from the prefix CS_ of its variants. But if I'm not mistaken, the AviSynth+ documentation prefers the term "color format" nowadays. So, maybe it should be named AvsColorFormat or even something else.

Plane enum should name 0

The enum that also defines PLANAR_Y should have a new variant with value 0. Its name could, e.g., be DEFAULT_PLANE. If possible compiler-wise, it could then also be used instead of 0 in avisynth.h where the regex plane ?= ?0 can be found, similar to int align=FRAME_ALIGN with NewVideoFrame()/NewVideoFrameP().

This would make it possible in my Rust code to have a proper enum instead of the integer type i32 in a couple of locations.

Unnecessary mutable pointer to value that's only read

Please change the pointer in signature of IScriptEnvironment::NewVideoFrameP() to const. It is currently PVideoFrame* propSrc. But it's only a source and copyFrameProps() that's used in the implementation also has const PVideoFrame& src in its signature. Such changes don't break the public API, right?

The argument propSrc as a snake case name would also be more consistent.

@Enyium
Copy link
Author

Enyium commented Feb 5, 2023

Everything accomplished.

@Enyium Enyium closed this as completed Feb 5, 2023
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

1 participant