Skip to content

added _syscalls interface; cleaned up _stdio interface; removed some...

Jan Sablatnig requested to merge jansablatnig/liboggz:fix-file_access into master

I)

Oggz supports access of its underlying file via stdio FILE streams, using oggz_open_stdio(). This access then boils down to using fread/fwrite/fseek/ftell/fflush access to the given file stream.

At least it used to. However, the patch abda2f59 on Oct 13th, 2010 changed that by replacing the fread(file) with read(fileno(file)). The reason given is that stdio streams otherwise stall out low bitrate incoming streams (I presume because the caching in fread() can introduce an additional delay).

However, using the stdio interface and the syscalls interface (read()/write()/ lseek()) at the same time yields "implementation dependent results". This should be avoided, but there are cases where it cannot be avoided with the current build.

A) Incompatible accesses within oggz

When random access reading an ogg file and jumping (via oggz_seek), both read() and fseek() (and ftell(), probably) are used. For a consistent access, we would need to switch from fseek() and ftell() to lseek().

When random access writing an ogg file and jumping (via oggz_seek), both fwrite() and lseek() (and fflush(), probably) are used as by the change above. For a consistent access, we would need to switch from fwrite() to write(), as well (and drop fflush()).

B) Incompatibility between oggz access and caller access.

When reading (or, after the above, writing) an OGG file, it is entirely legal for the calling application to also use the FILE it passed to ogg, for instance to skip over a gap or some such. Since a FILE pointer is used, the calling application should use the stdio interface. As mentioned above, this is incompatible with the read() call in the current build.

While I agree that this ladder point is probably quite rare, it is nonetheless faulty or at the very least misleading to telegraph to the caller that the stdio interface is being used if in fact the syscalls interface is used.

The solution here would to change the oggz interface to work with a file descriptor instead of a FILE pointer.

II)

Both the oggz-seek and the oggz-tell interface use "long" as a file offset. This does work in many cases (on 64-bit systems, "long" is also a 64-bit type and therefore big enough), but unfortunately not in all cases. On most 32-bit systems, "long" is typically a 32-bit type, while the file offset type "off_t" is 64-bits (at least, if one specifies -D_FILE_OFFSET_BITS=64). This implies that the current oggz lib does not support random access on ogg files of more than 2GiB on 32-bit systems. I work with many files that are larger than 2GiB and I do still need to support 32-bit systems.

The solution is to use off_t as the type of the offset for both ftell() and fseek() (by using ftello() and fseeko()). The system lseek() already uses off_t, but the oggz interface needs to carry this type through to the caller.

CHANGES

oggz_open_syscalls() added which accesses the underlying file via direct syscalls (read()/write()/lseek()).

oggz_open_stdio() now accesses the underlying file exclusively via stdio calls (in particular, fread() is used instead of read()). If the underlying file is a pipe (especially with low bitrate), you may want to switch to syscalls(), or use the stdio_unlocked interface (e.g. fread_unlocked()), for this you should check the implementation of oggz_open_stdio() and clone your own oggz_open_stdio_unlocked().

Also, oggz_open_stdio() no longer closes the given FILE file during oggz_close(), as this is generally seen as bad style (a library should only close a resource that it opened). If you need oggz_close() to continue to close your file, use something like this: static int my_close(void * handle) { fclose((FILE ) handle); return 0; } .. OGGZ oggz = oggz_open_stdio(file, flags); if (oggz) oggz_io_set_close(oggz, my_close, (void*) file); ..

Oggz should now be 64-bit clean for file offsets (by using off_t), therefore files that are bigger than 2GiB should be handled correctly on all systems, including seeking.

Inconsistent handling of file and non-file modes in oggz_purge() fixed.

Inconsistent handling of EOF in file modes in oggz_get_next_page() fixed.

File size in oggz_offset_end() now determined the standard way (by seeking and telling, no fstat). Please report if this causes a problem/slowdown in your application. (This could alternatively be handled via a dedicated IO callback such as OggzIOSize).

Merge request reports