IFileStream::tell method no longer virtual

[rh101]

• axmol version: dev - https://github.com/axmolengine/axmol/commit/16a70c9547175243a847f6c765cf05c2e9eea658
• devices test on:
• developing environments
  • NDK version: r23c
  • Xcode version: 14.2
  • Visual Studio:
    • VS version: 2019 (16.11), 2022 (17.6+)
    • MSVC version: 1929, 1934, 19.35, 19.36, 19.37
    • Windows SDK version: 10.0.22621.0
  • cmake version: Steps to Reproduce:

During updating my application from Axmol v1.0 to 2.0, I ran into an issue with IFileStream::tell(). That method was previously virtual, but now it's been in-lined, and hard-coded to this:

inline int64_t tell() const { return seek(0, SEEK_CUR); }

In my case, it's caused an issue with the virtual file system library I'm using at the moment, given the way the seek() and tell() functions work in it, as they actually have different code paths in the VFS library.

The specific library I'm using is PhysFS, which doesn't have an origin/whence parameter (SEEK_SET, SEEK_CUR, SEEK_END) like fseek, so the only way to make it compatible is to do the following:

int64_t PhysFsFileStream::seek(int64_t offset, int origin) const
{
    int result;
    switch (origin)
    {
    case SEEK_SET:
        result = PHYSFS_seek(_file, offset); // _file is type PHYSFS_File*
        break;
    case SEEK_CUR:
        result = PHYSFS_seek(_file, (PHYSFS_tell(_file) + offset));
        break;
    case SEEK_END:
        result = PHYSFS_seek(_file, PHYSFS_fileLength(_file) + offset);
        break;
    default:
        result = 0;
        break;
    }
    return result == 0 ? -1 : 0; // -1 = error, 0 = success
}

The PHYSFS_seek function in the PhysFS library also calls PHYSFS_tell internally, and using seek(0, SEEK_CUR); means a double call to PHYSFS_tell.

Using Axmol 1.0, it would have been a simple override like this, so a single call to 'PHYSFS_tell':

int64_t PhysFsFileStream::tell() const
{
    return PHYSFS_tell(_file);
}

Was there any specific reason for removing the virtual specifier on IFileStream::tell()? If it needs to stay in-lined, then that's fine, I can just have a fork with the change I need, but if there was no specific reason, then any chance it could be changed back to being a virtual method?

[halx99]

Use seek instead, and seek behavior change to same with posix lseek

[halx99]

• axmol version: dev - 16a70c9
• devices test on:

• developing environments

  • NDK version: r23c

  • Xcode version: 14.2

  • Visual Studio:

  • VS version: 2019 (16.11), 2022 (17.6+)

  • MSVC version: 1929, 1934, 19.35, 19.36, 19.37

  • Windows SDK version: 10.0.22621.0

  • cmake version: Steps to Reproduce:

During updating my application from Axmol v1.0 to 2.0, I ran into an issue with IFileStream::tell(). That method was previously virtual, but now it's been in-lined, and hard-coded to this:

inline int64_t tell() const { return seek(0, SEEK_CUR); }

In my case, it's caused an issue with the virtual file system library I'm using at the moment, given the way the seek() and tell() functions work in it, as they actually have different code paths in the VFS library.

The specific library I'm using is PhysFS, which doesn't have an origin/whence parameter (SEEK_SET, SEEK_CUR, SEEK_END) like fseek, so the only way to make it compatible is to do the following:

int64_t PhysFsFileStream::seek(int64_t offset, int origin) const
{
    int result;
    switch (origin)
    {
    case SEEK_SET:
        result = PHYSFS_seek(_file, offset); // _file is type PHYSFS_File*
        break;
    case SEEK_CUR:
        result = PHYSFS_seek(_file, (PHYSFS_tell(_file) + offset));
        break;
    case SEEK_END:
        result = PHYSFS_seek(_file, PHYSFS_fileLength(_file) + offset);
        break;
    default:
        result = 0;
        break;
    }
    return result == 0 ? -1 : 0; // -1 = error, 0 = success
}

The PHYSFS_seek function in the PhysFS library also calls PHYSFS_tell internally, and using seek(0, SEEK_CUR); means a double call to PHYSFS_tell.

Using Axmol 1.0, it would have been a simple override like this, so a single call to 'PHYSFS_tell':

int64_t PhysFsFileStream::tell() const
{
    return PHYSFS_tell(_file);
}

Was there any specific reason for removing the virtual specifier on IFileStream::tell()? If it needs to stay in-lined, then that's fine, I can just have a fork with the change I need, but if there was no specific reason, then any chance it could be changed back to being a virtual method? Maybe c style file api more easy to implement by diferrent underlaying file api, change back may be better:

tell on posix: lseek(0, SEEK_CUR)

[rh101]

Use seek instead, and seek behavior change to same with posix lseek

Oh, I understand using seek(0, SEEK_CUR); to get the current file position when dealing with files. My query was more related to the removal of the virtual on tell(), as the reason that method was initially added as virtual was because VFS libraries may implement it in a different way that is more optimal, depending on how they handle virtual files.

In the case of PhysFS, that library implements it like this:

PHYSFS_sint64 PHYSFS_tell(PHYSFS_File* handle)
{
    auto* fh = (FileHandle*)handle;
    const PHYSFS_sint64 pos = fh->io->tell(fh->io);
    const PHYSFS_sint64 retval = fh->forReading ?
        (pos - fh->buffill) + fh->bufpos :
        (pos + fh->buffill);
    return retval;
} /* PHYSFS_tell */

int PHYSFS_seek(PHYSFS_File* handle, PHYSFS_uint64 pos)
{
    auto* fh = (FileHandle*)handle;

    if (!PHYSFS_flush(handle))
    {
        return 0;
    }

    if (fh->buffer && fh->forReading)
    {
        /* avoid throwing away our precious buffer if seeking within it. */
        PHYSFS_sint64 offset = pos - PHYSFS_tell(handle); // <<<<<< notice that it calls tell() here
        if ( /* seeking within the already-buffered range? */
             /* forward? */
            ((offset >= 0) && (((size_t)offset) <= fh->buffill - fh->bufpos)) ||
            /* backward? */
            ((offset < 0) && (((size_t)-offset) <= fh->bufpos)))
        {
            fh->bufpos = (size_t)(((PHYSFS_sint64)fh->bufpos) + offset);
            return 1; /* successful seek */
        } /* if */
    } /* if */

    /* we have to fall back to a 'raw' seek. */
    fh->buffill = fh->bufpos = 0;
    return fh->io->seek(fh->io, pos);
} /* PHYSFS_seek */

Using seek for the current file position is not optimal, which is why I needed to override the IFileStream::tell(), otherwise it would do this: inline int64_t tell() const { return seek(0, SEEK_CUR); }

Then that method calls this:

int64_t PhysFsFileStream::seek(int64_t offset, int origin) const
{
..
    case SEEK_CUR:
        result = PHYSFS_seek(_file, (PHYSFS_tell(_file) + offset)); // Notice PHYSFS_tell is called here
        break;
...
}

which calls this: int PHYSFS_seek(PHYSFS_File* handle, PHYSFS_uint64 pos) which then calls this: PHYSFS_sint64 offset = pos - PHYSFS_tell(handle); // this calls PHYSFS_tell again.

and that is hitting the IO method again: fh->io->tell(fh->io);

So, effectively, it's calling the IO method twice. For Posix on PhysFS, tell() is implemented with lseek like this (as you mentioned), so lskeek is being called 2 times if I use FileStream::seek(0, SEEK_CUR);:

PHYSFS_sint64 __PHYSFS_platformTell(void *opaque)
{
    const int fd = *((int *) opaque);
    PHYSFS_sint64 retval;
    retval = (PHYSFS_sint64) lseek(fd, 0, SEEK_CUR);
    if (retval == -1)
    {
        PHYSFS_setErrorCode(errcodeFromErrno());
        return -1;
    }

    return retval;
} /* __PHYSFS_platformTell */

So, again, the issue is not the default implementation using seek, but rather how a VFS library may implement tell() in its own way, which is why the virtual was required on IFileStream::tell().

No matter though, if it's an issue to revert tell back to virtual, that is fine, since I can just maintain my own forked branch of the engine with that specific change.

[halx99]

do you implement whence of IFileStream::seek based on virutal file system?

[rh101]

do you implement whence of IFileStream::seek based on virutal file system?

Yes, but unfortunately PhysFS does not support whence:

/**
 * \fn int PHYSFS_seek(PHYSFS_File *handle, PHYSFS_uint64 pos)
 * \brief Seek to a new position within a PhysicsFS filehandle.
 *
 * The next read or write will occur at that place. Seeking past the
 *  beginning or end of the file is not allowed, and causes an error.
 *
 *   \param handle handle returned from PHYSFS_open*().
 *   \param pos number of bytes from start of file to seek to.
 *  \return nonzero on success, zero on error. Use PHYSFS_getLastErrorCode()
 *          to obtain the specific error.
 *
 * \sa PHYSFS_tell
 */
PHYSFS_DECL int PHYSFS_seek(PHYSFS_File *handle, PHYSFS_uint64 pos);

So, the only way I could get it to work was to do this:

int64_t PhysFsFileStream::seek(int64_t offset, int whence) const
{
    int result;
    switch (whence)
    {
    case SEEK_SET:
        result = PHYSFS_seek(_file, offset); // _file is type PHYSFS_File*
        break;
    case SEEK_CUR:
        result = PHYSFS_seek(_file, (PHYSFS_tell(_file) + offset));
        break;
    case SEEK_END:
        result = PHYSFS_seek(_file, PHYSFS_fileLength(_file) + offset);
        break;
    default:
        result = 0;
        break;
    }
    return result == 0 ? -1 : 0; // -1 = error, 0 = success
}

Using whence = SEEK_CUR causes it to call result = PHYSFS_seek(_file, (PHYSFS_tell(_file) + offset)); to figure out the current position etc. So, since PHYSFS_seek internally calls PHYSFS_tell as well, it ends up hitting the file IO twice (two calls to lseek).

PhysFS has many different platform-specific implementations of the file IO, so for example, posix tell is this:

PHYSFS_sint64 __PHYSFS_platformTell(void *opaque)
{
    const int fd = *((int *) opaque);
    PHYSFS_sint64 retval;
    retval = (PHYSFS_sint64) lseek(fd, 0, SEEK_CUR);
    if (retval == -1)
    {
        PHYSFS_setErrorCode(errcodeFromErrno());
        return -1;
    }

    return retval;
} /* __PHYSFS_platformTell */

ZIP file IO is this:

static PHYSFS_sint64 ZIP_tell(PHYSFS_Io *io)
{
    return ((ZIPfileinfo *) io->opaque)->uncompressed_position;
} /* ZIP_tell */

Windows IO is this:

PHYSFS_sint64 __PHYSFS_platformTell(void *opaque)
{
    HANDLE h = (HANDLE) opaque;
    PHYSFS_sint64 pos = 0;
    if (!winSetFilePointer(h, 0, &pos, FILE_CURRENT)) // this eventually calls Windows function SetFilePointerEx
    {
        PHYSFS_setErrorCode(errcodeFromWinApi());
        return -1;
    }
    return pos;
} /* __PHYSFS_platformTell */

And many more. Each have their own optimal implementation in order to get the current position.

Using FileStream::seek(0, SEEK_CUR); to get current position with PhysFS results in this call-chain:

FileStream::seek
PHYSFS_tell
lseek
PHYSFS_seek
PHYSFS_tell
lseek

But if FileStream::tell was virtual, and overriden, it would be just this:

FileStream::tell
PHYSFS_tell
lseek

[halx99]

will change back

[rh101]

will change back

Excellent, thank you!

[halx99]

the phyfs is vfs?

[rh101]

the phyfs is vfs?

Yes, I use it for accessing downloadable content in a specific format (such as ZIP), and since it's tied in through overrides in a custom IFileStream and a sub-class of FileUtils (one per platform), then it just integrates smoothly with the rest of Axmol. No changes to the Axmol code at all to get it all working either.

https://github.com/icculus/physfs

It works really well as the abstraction for many different file systems.

[halx99]

done

[rh101]

Thank you :)