Fix over-read behaviour of FileGroup to be the same on all platforms.

Instead of relying on the operating system's behaviour when seeking
off the end of a file, keep our own _position.  This normalises
the behaviour between POSIX and Windows.

diff --git a/src/lib/file_group.cc b/src/lib/file_group.cc
index 04e23b6b2e60889161197aa431dde484cba1619d..aaf94acf4b17a2eaf25595ba61fe9c4d5d964ed4 100644 (file)
--- a/src/lib/file_group.cc
+++ b/src/lib/file_group.cc
@@ -1,5 +1,5 @@
 /*
-    Copyright (C) 2013-2018 Carl Hetherington <cth@carlh.net>
+    Copyright (C) 2013-2020 Carl Hetherington <cth@carlh.net>
 
     This file is part of DCP-o-matic.
 
@@ -26,18 +26,18 @@
 #include "exceptions.h"
 #include "cross.h"
 #include "compose.hpp"
+#include "dcpomatic_assert.h"
 #include <sndfile.h>
 #include <cstdio>
-#include <iostream>
 
 using std::vector;
-using std::cout;
 
 /** Construct a FileGroup with no files */
 FileGroup::FileGroup ()
        : _current_path (0)
        , _current_file (0)
        , _current_size (0)
+       , _position (0)
 {
 
 }
@@ -103,55 +103,39 @@ FileGroup::ensure_open_path (size_t p) const
 int64_t
 FileGroup::seek (int64_t pos, int whence) const
 {
-       /* Convert pos to `full_pos', which is an offset from the start
-          of all the files.
-       */
-       int64_t full_pos = 0;
        switch (whence) {
        case SEEK_SET:
-               full_pos = pos;
+               _position = pos;
                break;
        case SEEK_CUR:
-               for (size_t i = 0; i < _current_path; ++i) {
-                       full_pos += boost::filesystem::file_size (_paths[i]);
-               }
-#ifdef DCPOMATIC_WINDOWS
-               full_pos += _ftelli64 (_current_file);
-#else
-               full_pos += ftell (_current_file);
-#endif
-               full_pos += pos;
+               _position += pos;
                break;
        case SEEK_END:
-               full_pos = length() - pos;
+               _position = length() - pos;
                break;
        }
 
-       /* Seek to full_pos */
+       /* Find an offset within one of the files, if _position is within a file */
        size_t i = 0;
-       int64_t sub_pos = full_pos;
-       while (i < _paths.size ()) {
+       int64_t sub_pos = _position;
+       while (i < _paths.size()) {
                boost::uintmax_t len = boost::filesystem::file_size (_paths[i]);
-               if (sub_pos < int64_t (len)) {
+               if (sub_pos < int64_t(len)) {
                        break;
                }
+               sub_pos -= int64_t(len);
                ++i;
-               if (i < _paths.size()) {
-                       /* If we've run out of files we need to seek off the end of the last file */
-                       sub_pos -= len;
-               }
        }
 
-       if (i == _paths.size ()) {
-               /* Seeking too far isn't an error; we'll seek too far in the last file which
-                * will "pass on" fseek()'s behaviour to our caller.
-                */
-               i--;
+       if (i < _paths.size()) {
+               ensure_open_path (i);
+               dcpomatic_fseek (_current_file, sub_pos, SEEK_SET);
+       } else {
+               ensure_open_path (_paths.size() - 1);
+               dcpomatic_fseek (_current_file, _current_size, SEEK_SET);
        }
 
-       ensure_open_path (i);
-       dcpomatic_fseek (_current_file, sub_pos, SEEK_SET);
-       return full_pos;
+       return _position;
 }
 
 /** Try to read some data from the current position into a buffer.
@@ -164,16 +148,32 @@ FileGroup::read (uint8_t* buffer, int amount) const
 {
        int read = 0;
        while (true) {
-               int64_t to_read = amount - read;
+
+               bool eof = false;
+               size_t to_read = amount - read;
+
+               DCPOMATIC_ASSERT (_current_file);
+
 #ifdef DCPOMATIC_WINDOWS
-               /* If we over-read from the file by too much on Windows we get a errno=22 rather than an feof condition,
-                * for unknown reasons.  So if we're going to over-read, we need to do it by a little bit, so that feof
-                * still gets triggered but there is no errno=22.
-                */
-               to_read = std::min(to_read, static_cast<int64_t>(_current_size - _ftelli64(_current_file) + 1));
+               int64_t const current_position = _ftelli64 (_current_file);
+               if (current_position == -1) {
+                       to_read = 0;
+                       eof = true;
+               } else if ((current_position + to_read) > _current_size) {
+                       to_read = _current_size - current_position;
+                       eof = true;
+               }
+#else
+               long const current_position = ftell(_current_file);
+               if ((current_position + to_read) > _current_size) {
+                       to_read = _current_size - current_position;
+                       eof = true;
+               }
 #endif
+
                int const this_time = fread (buffer + read, 1, to_read, _current_file);
                read += this_time;
+               _position += this_time;
                if (read == amount) {
                        /* Done */
                        break;
@@ -183,7 +183,7 @@ FileGroup::read (uint8_t* buffer, int amount) const
                        throw FileError (String::compose("fread error %1", errno), _paths[_current_path]);
                }
 
-               if (feof (_current_file)) {
+               if (eof) {
                        /* See if there is another file to use */
                        if ((_current_path + 1) >= _paths.size()) {
                                break;

diff --git a/src/lib/file_group.h b/src/lib/file_group.h
index 21499f5634483feac8f589ac79c797160975502e..0e26d64ecb8c5f5dbc07851224279c4f081918b0 100644 (file)
--- a/src/lib/file_group.h
+++ b/src/lib/file_group.h
@@ -53,6 +53,7 @@ private:
        mutable size_t _current_path;
        mutable FILE* _current_file;
        mutable size_t _current_size;
+       mutable int64_t _position;
 };
 
 #endif

diff --git a/test/file_group_test.cc b/test/file_group_test.cc
index 05127828cb722c39e32834a45ecb30632e6c437b..1d42741b65745783d47e67a7963b349b3a48e994 100644 (file)
--- a/test/file_group_test.cc
+++ b/test/file_group_test.cc
@@ -88,10 +88,15 @@ BOOST_AUTO_TEST_CASE (file_group_test)
        BOOST_CHECK_EQUAL (memcmp (data + pos, test, 128), 0);
        pos += 128;
 
-       /* Read overlapping B/C/D and over-reading */
+       /* Read overlapping B/C/D and over-reading by a lot */
        BOOST_CHECK_EQUAL (fg.read (test, total_length * 3), total_length - pos);
        BOOST_CHECK_EQUAL (memcmp (data + pos, test, total_length - pos), 0);
 
+       /* Over-read by a little */
+       BOOST_CHECK_EQUAL (fg.seek (0, SEEK_SET), 0);
+       BOOST_CHECK_EQUAL (fg.read (test, total_length), total_length);
+       BOOST_CHECK_EQUAL (fg.read (test, 1), 0);
+
        /* Seeking off the end of the file should not give an error */
        BOOST_CHECK_EQUAL (fg.seek (total_length * 2, SEEK_SET), total_length * 2);
        /* and attempting to read should return nothing */