Plugin API: a proper File Stream API

[ivan-mogilko]

For #2226.

This is a draft made primarily for testing these changes on Android.

Done in a more primitive way first, without a IAGSStream struct, but added F* functions (FSeek, FLength etc) into IAGSEngine instead. Plugin works with stream using int32 handles. This is a working variant, although an "ugly" one.

UPDATE: added a IAGSStream interface to plugin API.

Overall changes to plugin API look like this:

#define AGSSTREAM_FILE_OPEN         0
#define AGSSTREAM_FILE_CREATE       1
#define AGSSTREAM_FILE_CREATEALWAYS 2

#define AGSSTREAM_MODE_READ  0x1
#define AGSSTREAM_MODE_WRITE 0x2
#define AGSSTREAM_MODE_READWRITE (AGSSTREAM_MODE_READ | AGSSTREAM_MODE_WRITE)

#define AGSSTREAM_SEEK_SET 0
#define AGSSTREAM_SEEK_CUR 1
#define AGSSTREAM_SEEK_END 2

class IAGSStream {
public:
  // Flushes and closes the stream, deallocates the stream object.
  // After calling this the IAGSStream pointer becomes INVALID.
  virtual void   Close() = 0;
  // Returns an optional stream's source description.
  // This may be a file path, or a resource name, or anything of that kind.
  virtual const char *GetPath() = 0;
  // Reads number of bytes into the provided buffer
  virtual size_t Read(void *buffer, size_t len) = 0;
  // Writes number of bytes from the provided buffer
  virtual size_t Write(void *buffer, size_t len) = 0;
  // Returns the total stream's length in bytes
  virtual int64_t GetLength() = 0;
  // Returns stream's position
  virtual int64_t GetPosition() = 0;
  // Tells whether the stream's position is at its end
  virtual bool   EOS() = 0;
  // Seeks to offset from the origin, see AGSSTREAM_SEEK_* constants
  virtual void   Seek(int64_t offset, int origin) = 0;
  // Flushes stream, forcing it to write any buffered data to the
  // underlying device. Note that the effect may depend on implementation.
  virtual void   Flush() = 0;
};

  // *** BELOW ARE INTERFACE VERSION 28 AND ABOVE ONLY
  // Opens a data stream, resolving a script path.
  // File mode should contain one of the AGSSTREAM_FILE_* values,
  // work mode should contain flag set of the AGSSTREAM_MODE_* values.
  // Returns IAGSStream object, or null on failure.
  // IAGSStream must be disposed by calling its Close() function.
  AGSIFUNC(IAGSStream*) OpenFileStream(const char *script_path, int file_mode, int work_mode);
  // Returns IAGSStream object identified by the given stream handle.
  // This lets to retrieve IAGSStream object from a handle received in a event callback.
  // Returns null if handle is invalid.
  AGSIFUNC(IAGSStream*) GetFileStreamByHandle(int32 fhandle);

[ericoporto]

without a IAGSStream

Do we need all iagsstream (or at all) or can we make a smaller part of it, so we have more flexibility? I am just trying to figure what is really needed, because once this is external, it has to be kept in that API - I remember you were thinking about some change to streams.

Edit: now I see the GitHub issue refers to a different IAGSStream and not IAGSStream.h, so I may be making a confusion.

[ivan-mogilko]

Okay, I made a successful test with plugin, and made theora video run on Android through this interface, being streamed by the engine:
https://github.com/ivan-mogilko/ags-spritevideo/tree/android--newfileapi

Do we need all iagsstream (or at all) or can we make a smaller part of it, so we have more flexibility? I am just trying to figure what is really needed, because once this is external, it has to be kept in that API - I remember you were thinking about some change to streams.

Edit: now I see the GitHub issue refers to a different IAGSStream and not IAGSStream.h, so I may be making a confusion.

It's all under question. Years ago I've been thinking to make Stream base class to implement plugin interface directly. That's an option. But existing IAGSStream is way too cumbersome, there's no need for that.
So I'd probably get rid of that, and make a wrapper around Stream ptr for now.

In regards to change to streams, here's an approximate way of how this may be done:
https://github.com/ivan-mogilko/uwsav-dump/blob/master/utils/stream.h
this is almost like Nick Sonneveld wanted to do, and even simpler because Stream is not a virtual class anymore, but only a thin wrapper over StreamBase. But it has explicit ReadIntLE functions instead of having file data endianness as a property. Whether this will suit ags or not may be decided later.

[ivan-mogilko]

Added IAGSStream variant of stream API, updated PR description.

[ivan-mogilko]

So, to clarify on implementation, I am making our base Stream class to implement an interface (contract) identical to the one exposed in plugin API, because it already matches the requirements, and pass this stream object to plugins. But this does not impose any restrictions on any future changes to these classes. If this becomes inconvenient at any point to pass Stream object to plugins directly, then we may as well switch to another class, or write a wrapper over Stream. It is all good so long as the returned object implements exact interface.

I also added two standard C headers to the plugin api header: stddef.h and stdint.h. This is for int64_t and size_t. I believe these are pretty common today, so there's no need to declare our custom types there. In the worst case, we might add a preprocessor switch there and define these types (or ask plugin authors to include such declaration prior to plugin header).
This also means that custom int32 type may be removed from plugin api and replaced by a standard int32_t. Or int32 declaration may be left for backwards compatibility (to avoid people having to fix their plugin compilation).

I still have doubts about size_t, and whether it is okay to have it in plugin API. As this type's size depends on build configuration. It may be argued that this type's size should be in sync with pointer size (32-bit, 64-bit etc), and then it will only be required to have plugin build match engine build; but this may not be always true (?), and I wonder if it also may depend on compiler and standard library implementation.
I shall investigate this, and maybe will have to replace with something else, like uint32_t or uint64_t (but in such case Stream class likely cannot be passed directly anymore).

EDIT: did a quick check on SDL, and their rwops use size_t of course (rwops may be a good reference for the stream api):
SDL2: https://github.com/libsdl-org/SDL/blob/cc7c0a2dabe016e454e83f607465558188ed7548/include/SDL_rwops.h#L74
SDL3: https://github.com/libsdl-org/SDL/blob/d4448fe3d2cfaec73a2c7b0e9394fa87be4f1ab6/include/SDL3/SDL_rwops.h#L84
since SDL is dynamically linked, its case is close to our plugins.

---

In terms of API, my biggest doubt right now is stream closing. I did it with a function in IAGSEngine interface for the test, but it may be more convenient to have a Close method in IAGSStream instead. Problem is that such method must also deallocate the stream object and possibly do something else in the engine, if engine wants to track streams opened for plugins, for example. That means this cannot be done with pure Stream object, there got to be a wrapper instead.

Why it's more convenient to have Close in IAGSStream: because if we want to support streams provided by plugins too at some point, it will be safer to just call Close instead of remembering which plugin this stream came from and call its "CloseStream" function.

[ivan-mogilko]

Alright, updated once more. Please see PR's description for API details. In summary, now the IAGSStream closes itself with Close method, which is supposed to deallocate the stream object (so pointer becomes invalid after this). Also added Flush, EOS and GetPath() methods, for sake of completeness. I think that everything else may be done by wrapping or combining existing methods.

I had to make a wrapper over our Stream object for this to work.

I'm testing this using my plugin here in this temp branch, where it utilizes this interface to read video:
https://github.com/ivan-mogilko/ags-spritevideo/tree/android--newfileapi3

BTW, with this one can read video or image files packaged inside *.ags pack too (like ogvs normally are packaged in ags games).

[ivan-mogilko]

...forgot to update api version.

[ericoporto]

I am a bit lost on one specific API

  // Returns IAGSStream object identified by the given stream handle.
  // This lets to retrieve IAGSStream object from a handle received in a event callback.
  // Returns null if handle is invalid.
  AGSIFUNC(IAGSStream*) GetFileStreamByHandle(int32 fhandle);

What is the use-case here? I am trying to figure what event callback this is meant to work with.

[ivan-mogilko]

What is the use-case here? I am trying to figure what event callback this is meant to work with.

Right now there are 2 events that pass int32 "file handler" into plugins:
AGSE_RESTOREGAME and AGSE_SAVEGAME. These "handlers" are meant to be used with IAGSEngine::FRead and FWrite functions.
I'd like to provide an option to get the IAGSStream for these callbacks, but there's no way to pass a pointer unless I change the callback prototype. So for the time being I added this, as sort of a bridge between old-style file handle to new-style iagsstream object.

In the future we might redesign how the events work, maybe even provide more specific prototypes for each event (and deprecate GetFileStreamByHandle). But for the time being, this is a workaround: pass a int32 filehandle and let plugin get IAGSStream from it if it needs to.

[ericoporto]

That is fine. The new api looks ok API wise when comparing with the rest of the current API.

My minor nitpick would be the following

Seek(int64_t offset, int origin) = 0;

Origin here is not a position, it's one of the macros that work like enums here - I guess for some C reason. But the macro doesn't mention they are related to origin.

#define AGSSTREAM_SEEK_SET 0
#define AGSSTREAM_SEEK_CUR 1
#define AGSSTREAM_SEEK_END 2

I would suggest renaming origin to origin_type and mention it in the macro.

Seek(int64_t offset, int origin_type) = 0;

#define AGSSTREAM_SEEK_ORIGIN_SET 0
#define AGSSTREAM_SEEK_ORIGIN_CUR 1
#define AGSSTREAM_SEEK_ORIGIN_END 2

It's verbose but it makes it easier to understand (I think).

[ivan-mogilko]

No, sorry, I do not want to do this. I've been already struggling to keep these macro names short. It's one thing when the macro is used as a global setting that is set once per program (like SDL hints), but this is a common operation.
"Origin" for a "seek" is a well defined term in stream interfaces too, it's used in many libraries (another term is "whence"). Again, this is not something unique to AGS api, so once a person learns how stream seek works, they can do it everywhere.
Here's an example from SDL, for instance:
https://github.com/libsdl-org/SDL/blob/4722269fb62315640cbdbd8916cfc937ee3603d4/include/SDL3/SDL_rwops.h#L70-L76

We may plan to rework plugin API for ags4, put everything in a namespace and turn macros to enums/enum classes (for compilers that support them). That may shorten typing things too.

[ericoporto]

The issue is the comment is "Seeks to offset from the origin" and origin being an int, it's hard to understand that origin is a small set of possibilities - and not some other offset. I would suggest at least rewording the comment to "Seeks to offset from the beginning, current position or end of stream".

[ivan-mogilko]

I tried this:

// Stream seek origins
// Seek from the beginning of a stream (towards positive offset)
#define AGSSTREAM_SEEK_SET 0
// Seek from the current position (towards positive or negative offset)
#define AGSSTREAM_SEEK_CUR 1
// Seek from the end of a stream (towards negative offset)
#define AGSSTREAM_SEEK_END 2

  // Seeks to offset from the origin defined by AGSSTREAM_SEEK_* constants:
  //  * AGSSTREAM_SEEK_SET - seek from the beginning;
  //  * AGSSTREAM_SEEK_CUR - seek from the current position;
  //  * AGSSTREAM_SEEK_END - seek from the end (pass negative offset)
  virtual void   Seek(int64_t offset, int origin) = 0;

[ericoporto]

Looks good! Thanks, this is easier to understand! :)

[ivan-mogilko]

Also made Seek return new position instead of bool, following examples from SDL and .NET framework.

[ivan-mogilko]

I have seconds thoughts about this, or rather the way it is done, following my experiments with #2256 (and others).

The problem with the current implementation is that the stream interface exposed to plugins is a different type from our common stream interface. When giving out stream to plugins this requires a wrapper, which is not very nice but tolerable. However, there may also be an opposite situation, where the stream object will be passed from plugin to engine. I see two hypothetical cases when this may happen:

1. Plugin API has some function that accepts a IAGSStream. Realistic example: functions that open a video or sound decoder for plugins to use. Plugin asks engine to open a stream for it, and then opens a decoder by passing this engine's stream pointer.
2. Plugin provides its own stream type for the engine to use in some circumstance. I do not have an actual practical example for this yet, but in theory this may happen if plugin is intended to stream something from a custom package type, or plugin's memory, etc.

In this situation, imo, it would be convenient if the stream interface would directly match internal engine's stream interface. If it does not, then there will be extra code branching, and maybe additional need for wrappers whenever such stream is used.
With our own stream this becomes particularily silly, because IAGSStream hides a Common::Stream in a wrapper, but when the engine receives IAGSStream it will have to wrap it again in order to pass into own functions expecting Common::Stream.

When I tried matching interfaces at first while writing this PR, I met 2 problems:

1. Some args or return values may be different;
2. Disposing of a stream object.

Problem 1 turned out to be mostly a non-issue, as "extended" types such as size_t and int64_t may be allowed in plugin api. The only remaining inconvenience is a method GetPath that returns a const String& in our Stream class and const char* in plugin api. This may have following solutions:

• make GetPath return const char* in out stream class also;
• actually remove GetPath from the stream interface as redundant (this is a helper method for diagnostic purposes, and i do not think any common libs or frameworks have this).

Problem 2 is a more complicated one. There are 2 classic approaches for the plugin interface:

• create/destroy pair of global factory functions;
• create factory function, and have a sort of "dispose" method in the object's interface that deletes the object itself.

The second approach, although technically possible, will be inconsistent with our stream class, and generally with common-use C++ classes which objects may be allocated on a stack. Hence this is unsuitable if implemented directly in Stream, unless we make a rule to never allocate Stream object on a stack, and possibly enforce this with some trick in code...

The first approach seems easy at first: plugin opens a stream using IAGSEngine::OpenFileStream and destroys it using IAGSEngine::FreeFileStream (or similar name).
The issue with this is that destruction is indirect to an object.
In case of using IAGSStream inside a plugin, this is not exactly a technical issue, but rather an inconvenience.
All the engine's API methods are accessed through a IAGSEngine pointer, received at one time as an argument to AGS_EngineStartup callback. This means that in order to destroy a stream, plugin should keep this global pointer along with the stream. Either stored in a global variable (most existing plugins do so), or paired in some wrapper struct. For example, smart pointers like unique_ptr allow to have custom deleter, which may store an engine interface in this situation.

The issue becomes somewhat more complicated in case the IAGSStream pointer came from a plugin, and depending on how it is done. It's one thing when plugin provides a sort of a "Stream Factory" interface, which also may be stored along with the stream ptr. But there easily may be a situation when engine won't know where the stream ptr came from. The example of such situation is given by me above:

Plugin API has some function that accepts a IAGSStream.
Basically, if we only accept a IAGSStream alone, we would not be able to tell where it came from, and given the IAGSStream itself does not have its own Dispose method, won't be able to know how to properly free this object.

So this is a blocking issue in the first approach (create/destroy factory functions).
Which means that stream disposal likely have to be a IAGSStream's own method. Which in turn makes implementing this interface in our Stream class inconvenient.

This puts me in some heavy doubts. May be I'm missing something obvious, or a tolerable workaround. I'd like to spend some more time to think about this...

[ivan-mogilko]

After thinking this over for a while, I can see only two potential solutions for the problem of "dispose" method, that would allow to avoid using extra wrappers. This refers to any objects shared between engine and plugin, that may be freely deleted: stream, audio and video processors (decoders, players, filters), and anything of that kind.

These solutions are:

1. Actually have a Dispose() method in the internal object interface in the engine, but add a comment to never use it directly because it frees the object's memory (especially if they are allocated on stack).
   For objects received from plugins, these may be stored in a std::unique_ptr (or shared_ptr) with custom deleter that calls this Dispose() method instead of delete operator. This would let to not handle this difference manually.

2. An alternative curious idea is to pass these objects in plugin API in extra small structs, pairing object's pointer and a pointer to deleter function. As an example:

template <typename TInterface>
struct DisposableInterface
{
    TInterface *Object;
    void (*Dispose)(TInterface *obj);
};

So when the stream, or any other disposable object is passed from plugin to engine, it's done using this struct instead, for example:

typedef DisposableInterface<IAGSStream> DisposableStream;

IAGSAudioDecoder *OpenAudioDecoder(DisposableStream *stream);

The struct itself is not owned, and should not be deleted by the engine, of course. We simply take values out from it.
Similarly to the first solution, these received objects may be stored in a unique_ptr, except that custom deleter would be using this provided Dispose function pointer.