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

Questions for Rust crate #318

Open
Enyium opened this issue Jan 26, 2023 · 9 comments
Open

Questions for Rust crate #318

Enyium opened this issue Jan 26, 2023 · 9 comments

Comments

@Enyium
Copy link

Enyium commented Jan 26, 2023

During the development of my Rust crate that can be used to write AviSynth+ plugins, these questions came up. Please don't hesitate to answer only a subset of questions at a time, if need be. I'll keep commenting out questions that were already answered to keep things clear (ideally, always quote me).

  • How should a filter react when another filter's GetFrame() method returns a null pointer PVideoFrame? I experienced process crashes when returning a null pointer PVideoFrame myself. When a filter implementation in a third-party Rust plugin yields a frame with incorrect metrics (width, height, color format), I already block the frame and deliver a frame with an error text message instead. I could also yield such a message frame to a Rust plugin when a third-party C++ plugin yields a null pointer PVideoFrame, if you don't have a better idea about how to handle this. When I'd do that, I'd like to write a reference to the plugin or plugin function onto the frame, however. For this, I'd need a function to, e.g., get the .avs script function name that's associated with the IClip. Is that realistic? Are you willing to implement this?
  • Can a filter always safely fetch its VideoInfo from an IClip it depends on (i.e., retrieves its audio and frames from), without lifetime issues on filter chain destruction? In other words: Can you rely on your filter being destructed before the filters you depend on?
  • May VideoInfo::image_type validly and meaningfully contain IT_BFF or IT_TFF when the IT_FIELDBASED bit is 0? For frame-based clips, are the IT_BFF/IT_TFF flags read by AviSynth+, or may they validly and meaningfully be read by plugins? If yes, why is GetParity() documented as working with frame-based clips? Wouldn't that be two sources for the same piece of information? If no, please document in avisynth.h for VideoInfo::image_type that, if IT_FIELDBASED isn't set, the other bits are ignored.

  • Can the return values from IClip::GetParity() be derived from VideoInfo::image_type? If not, why?

  • What's the enum defining CS_MPEG2_CHROMA_PLACEMENT, CS_YUY2_CHROMA_PLACEMENT, CS_TOPLEFT_CHROMA_PLACEMENT etc. about? Is it this? The large comment in struct VideoInfo says: Other YV12 specific (not used?) C 20-22 Chroma Placement values 0-4 see CS_xxx_CHROMA_PLACEMENT. So, it's currently always irrelevant? How can it become relevant, because the color format constants must not change, right? Adding another VideoInfo member, as with image_type?

@qyot27
Copy link
Member

qyot27 commented Jan 26, 2023

In an audio buffer, are the 3 bytes of a SAMPLE_INT24 integer on a big endian system (Mac) really in opposite order towards a little endian system like one running Windows?

Output of audio from AviSynth+ matches the endianness of the system. This led to issues in FFmpeg because the AviSynth demuxer in FFmpeg used to make a hard assumption of little endian audio, and that caused garbage audio output on the iMac G5 (and would also on newer big endian systems running Linux, BSD, etc.). This was resolved in this commit. The same file on a typical x86 system played correctly.

Same system endianness dependency for other sample and pixel component types?

Yes, but the difference is that for pixel formats, FFmpeg abstracts big endian/little endian formats (example). There is no equivalent abstraction for audio endianness, which is why that needed to be adjusted in the previously-referenced commit.

No matter what endianness a source file was saved in?

It depends on how the decoder in the source filter handles it. Most source filters available on non-Windows OSes rely on libavcodec to handle the decoding, so it would be returning the same endianness that FFmpeg would if given the file directly.

@pinterf
Copy link

pinterf commented Jan 26, 2023

  • What's the use case for IScriptEnvironment::Allocate() and IScriptEnvironment::Free(), to which the AvsAllocType enum belongs? Only audio buffers?

They refer to Avisynth's internal buffer pool. Resizers and other internal filters are using it, they are part of Avisynth final, documented interface since V8. I think early Avisynth+ developers defined these functions to have a quicker way of allocation instead of relying on the actual OS and compiler. For the full history you can check this topic: https://forum.doom9.org/showthread.php?t=168856 (I didn't even know C++ when they started the Avisynth+ project, so sometimes I'm as clueless as you)

There is no detailed documentation on this SDK part, I just put them here to have a reference.
https://avisynthplus.readthedocs.io/en/latest/avisynthdoc/FilterSDK/Cplusplus_api.html#allocate-v8

@pinterf
Copy link

pinterf commented Jan 26, 2023

  • IScriptEnvironment::PlanarChromaAlignment(), to which the PlanarChromaAlignmentMode enum belongs to, is undocumented. What does it do?

I've never seen a use case for it. Maybe it covered a pre Avisynth+ workaround to the plane alignment problems?
Before Avisynth+ plane pointers were not necessarily aligned, not even to 16 bytes.

This is what I found about them:
https://forum.doom9.org/showpost.php?p=1602004&postcount=12
https://avisynthplus.readthedocs.io/en/latest/avisynthdoc/FilterSDK/Cplusplus_api.html#alignplanar

@pinterf
Copy link

pinterf commented Jan 26, 2023

  • A third-party C++ plugin author could set both VideoInfo::image_type flags IT_BFF and IT_TFF. That would be invalid, right? It would be good if the BFF-TFF information would've been defined to occupy just one bit. Or is there a reason why this isn't the case? When I receive a VideoInfo with both the IT_BFF and IT_TFF image_type flags set, how should I interpret the information? What bit should I prefer? Or should I view bits 11 the same as 00? (The answers should be documented.)

As far as I know the usage of BFF-TFF is already a hybrid one in some recent plugins. Avisynth+ handles frame properties, but specifically the VaporSynth originated _Field and _Fieldbased are still not handled inside.
See the relevant frame property names here:
http://avisynth.nl/index.php/Internal_functions#Field

I don't know what to do with them. In Avisynth they are clip properties - through VideoInfo bits. In all other case these variables are supported as frame properties - e.g. plugins and source filters can set it and use it - even overriding the setting which is set 'officially" in Avisynth.#317

(And a task for myself (and @Reel-Deal to push me if I forget about it :) ) : I noticed that I haven't copied the topic Reserved frame property names in http://avisynth.nl/index.php/Internal_functions#Field into our refreshed .rst based doc)

@pinterf
Copy link

pinterf commented Jan 26, 2023

  • Why does INeoEnv have the smart pointer type PNeoEnv? Is it wrong to cast the pointer? Must the PNeoEnv constructor be used that takes an IScriptEnvironment? Why does its doc comment call IScriptEnvironment2 a "legacy interface"? Why isn't IScriptEnvironment2 removed if nobody may depend on it? (I'm aware that this part of the API is unstable.)

This is all history. Both IScriptEnvironment2 and NeoEnv. Especially the latter one. They contain internal interface - always left undocumented - stuff which you are expected to never use.

@pinterf
Copy link

pinterf commented Jan 26, 2023

  • IScriptEnvironment::ManageCache() is undocumented. What does it do?

Used only for internal purposes. Magic. After Avisynth v2.5 developers made it public, but I think at that time there was no IScriptEnvironment2 or NeoEnv for non-stable internal variants of environment interface calls.

@Enyium
Copy link
Author

Enyium commented Jan 26, 2023

@pinterf

As far as I know the usage of BFF-TFF is already a hybrid one in some recent plugins. Avisynth+ handles frame properties, but specifically the VaporSynth originated _Field and _Fieldbased are still not handled inside. See the relevant frame property names here: http://avisynth.nl/index.php/Internal_functions#Field

What do you mean by "hybrid usage"?

I don't know what to do with them. In Avisynth they are clip properties - through VideoInfo bits. In all other case these variables are supported as frame properties - e.g. plugins and source filters can set it and use it - even overriding the setting which is set 'officially" in Avisynth.

According to the definition, _FieldBased is a clip property that I'd expect to be the same for every frame of a clip. This information actually shouldn't be stored in frame properties.

(The value of _Field would also be quite easy to deduce from previously mentioned clip properties, at least if you know the frame index. But if it helps you from the worm's-eye view of frame processing.)

If this risks slowing things down, I'd think these frame props shouldn't be made available in principle. Perhaps provide a script function to populate these props, so they are there only when they're needed.


But all of this doesn't answer my question how the ambiguity of VideoInfo::image_type is to be handled. _FieldBased and _Field, being ints and not bit fields, are basically enums that can only contain invalid enum values, but not multiple enum values at once. What interpretation of both IT_BFF and IT_TFF being set would be the most natural? Is one of TFF/BFF more common to such a degree that its bit being set can make you ignore the other bit? Or is it more reasonable and to be recommended to view both being set as BFF-TFF information being unknown? It sounds to me like the latter would be the way to go.

My Rust crate currently has image_type information as is_top_field_first: Option<bool> (3 possible values) with None meaning frame-based, Some(true) TFF and Some(false) BFF. If now there's a "BFF-TFF unknown" state, this would need to be mapped somehow, possibly also as None. But this would then be data loss and make a field-based clip with unknown BFF-TFF info into a frame-based clip. How bad is that from my side, given that another filter already didn't provide BFF-TFF info? Will such a filter also have been wrong about the clip being field-based? I could also trust the field-based-ness and assume BFF or TFF (which?!), because it's got to be one of the two.

@Enyium
Copy link
Author

Enyium commented Jan 26, 2023

@pinterf

This is all history. Both IScriptEnvironment2 and NeoEnv. Especially the latter one. They contain internal interface - always left undocumented - stuff which you are expected to never use.

A doc comment in avisynth.h reads: "you are welcome to test it and give your feedback about any ideas, improvements, or issues you might have." I thought it was a good way to provide unstable functionality. You can test whether something would work for your plugin and give feedback for possible future improvement of AviSynth+, which would mean the functions go over into IScriptEnvironment.

I tried casting IScriptEnvironment into IScriptEnvironment2 and call InternalFunctionExists() on it, and it worked. The same function from INeoEnv didn't work however.

My IScriptEnvironment equivalent is called Env and makes available IScriptEnvironment functionality. I made the subtype UnstableEnv available via env.unstable(), which provides internal_fn_exists() and could be extended with more functions. I documented UnstableEnv as not to be used in plugin releases, and env.unstable() would cause a compiler warning as soon as the respective Rust feature is available to do that.

Even if IScriptEnvironment2 and INeoEnv are history in their current state, wouldn't it be good if there could still be that testing ground of an unstable API, before immortalizing something in IScriptEnvironment?

@Reel-Deal
Copy link
Member

(And a task for myself (and @Reel-Deal to push me if I forget about it :) ) : I noticed that I haven't copied the topic Reserved frame property names in http://avisynth.nl/index.php/Internal_functions#Field into our refreshed .rst based doc)

Lucky for you, I already added that to the docs some weeks ago :) ... just need to finish other parts of the syntax.

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

4 participants