This iFeature/libzip1.0 seek tell fix

[Den271]

This PR implements Tell/Seek support on ZipFileByteStream level,
it implements the idea of rewinding by read operation the underlying libzip structure if libzip zip_source layer doesn't support ZIP_SOURCE_SEEK functionality, no matter whether it is compressed or not.

ByteStream::size_type ZipFileByteStream::Seek(size_type by, std::ios::seekdir dir)
{
...
if ((_file->src->supports & ZIP_SOURCE_MAKE_COMMAND_BITMASK(ZIP_SOURCE_SEEK)) == 0)     // if the source doesn't support seek operation
    {
        return SeekByRewind(by, whence);    // Emulation of Seek operation by rewind
    }
...

Updated libzip with seek/tell operations for stored files only.
After reviewing the libzip I believe it doesn't make sense to push the Seek/Tell emulation for compressed content inside the zip_source_deflate.c because there could be another compression/decompression algos/layers and the implementation has similar idea of rewinding the file by read/decompress operation.

Note: I've tested it on VS2013&Windows7 platform

[danielweck]

If I am not mistaken, this PR (libzip1.0_seek_tell_fix feature branch) contains Windows-specific fixes that are also implemented in the feature branch that fixes memory leaks (xml_memleak_fix, see #190). Therefore, there are common code diffs in both PRs (vcxproj Visual Studio files, and some *.h and *.cpp tweaks to enable MSBUILD compilation). For the sake of readability (i.e. code reviews) and traceability (i.e. once the PR is merged), it would be nice to extract the Windows-specific code changes into a separate feature branch. I realize that this means more admin work though (juggling branches in local repository copy), so this may be an unreasonable request. I'll leave it to you :)

[nleme]

I made a quick code review of this pull request. I just have some observations:

(1) First of all, a point related to what Daniel said above. I.e., there are some changes in this pull request whose purpose is to make this code buildable in Visual C++. We have to thread carefully here, because we want to keep the Readium SDK still buildable in Xcode and NDK. Therefore, we should probably build these changes in those environments to make sure that everything is still building in them.

In fact, I'm curious about what the Clang and GCC compilers are going to say about changes like the removal of the empty function body in ePub3/ThirdParty/google-url/base/logging.h: functions like LogMessage(), that used to have a body of {} now have nothing. I wonder if that will be fine by those compilers too. Compiling this pull request code in Xcode and NDK and then running it should be enough to make sure that these changes are fine.

(2) A small tidbit: in the file ePub3/utilities/integer_sequence.h, you added a "#if 1" with its associated "#endif". I guess this is not needed, therefore it can be removed.

(3) I found interesting the solution that you found in order to add the Seek and Tell functionality back. Instead of adding the Seek and Tell as another "layered source" (that weird implementation of the Chain of Responsibility pattern in C from the libzip guys), you added the code to support Seek and Tell directly into the ZipFileByteStream class. That is an interesting choice and avoids having to navigate the code inside libzip with its C weirdness. However, this ties the ZipFileByteStream class and libzip really close together: from this point on, ZipFileByteStream depends on libzip, and it would be a larger change to move to a different ZIP file library. I don't think that it is a big problem because ZipFileByteStream is just a subclass of ByteStream, and we can always create another subclass (something like AlternateZipFileByteStream) if we need. However, I just wanted to point this out.

So, with those observations, I think that the pull request is good and it can be merged, just after we check observation (1): it would not be good if develop gets broken for iOS and Android after the merge.

[Den271]

@nleme ,

In fact, I'm curious about what the Clang and GCC compilers are going to say about changes like the removal of the empty function body in ePub3/ThirdParty/google-url/base/logging.h: functions like LogMessage(), that used to have a body of {} now have nothing. I wonder if that will be fine by those compilers too. Compiling this pull request code in Xcode and NDK and then running it should be enough to make sure that these changes are fine.

please see the google-url\base\logging.cc file, it contains all the definitions of the bodies of the commented empty ones in logging.h. And the google-url\base\logging.cc couldn't be compiled because of multiple body definitions, so I removed the empty bodies in logging.h file.
I think this is correct, and if I'm wrong please let me know

(3) I found interesting the solution that you found in order to add the Seek and Tell functionality back. Instead of adding the Seek and Tell as another "layered source" (that weird implementation of the Chain of Responsibility pattern in C from the libzip guys), you added the code to support Seek and Tell directly into the ZipFileByteStream class. That is an interesting choice and avoids having to navigate the code inside libzip with its C weirdness. However, this ties the ZipFileByteStream class and libzip really close together: from this point on, ZipFileByteStream depends on libzip, and it would be a larger change to move to a different ZIP file library. I don't think that it is a big problem because ZipFileByteStream is just a subclass of ByteStream, and we can always create another subclass (something like AlternateZipFileByteStream) if we need. However, I just wanted to point this out.

I had the previous implementation of it in zip_source_deflate.c. But unfortunately I executed some dangerous git command and cleared that local changes while switching to the other branch. Then I reviewed the idea I supposed the most robust place for the seek emulation is the layer on top of all that 'layered' stack. This make sense because the libzip can support different types of compression/decompression algorithms(which means different layer chains), and the 'native' approach would require to add and test the similar 'rewind' code into every one(or create the new additional layer on top of them), there are at least two such a places (deflate and pkware algos). Thats why I re-implemented that seek command simulation right in ZipFileByteStream , and the code looks clearer because of c++ style here.
Here is the list of compression methods, etc (from zip.h)

#define ZIP_CM_DEFAULT        -1  /* better of deflate or store */
#define ZIP_CM_STORE           0  /* stored (uncompressed) */
#define ZIP_CM_SHRINK          1  /* shrunk */
#define ZIP_CM_REDUCE_1        2  /* reduced with factor 1 */
#define ZIP_CM_REDUCE_2        3  /* reduced with factor 2 */
#define ZIP_CM_REDUCE_3        4  /* reduced with factor 3 */
#define ZIP_CM_REDUCE_4        5  /* reduced with factor 4 */
#define ZIP_CM_IMPLODE         6  /* imploded */
/* 7 - Reserved for Tokenizing compression algorithm */
#define ZIP_CM_DEFLATE         8  /* deflated */
#define ZIP_CM_DEFLATE64       9  /* deflate64 */
#define ZIP_CM_PKWARE_IMPLODE 10  /* PKWARE imploding */
/* 11 - Reserved by PKWARE */
#define ZIP_CM_BZIP2          12  /* compressed using BZIP2 algorithm */
/* 13 - Reserved by PKWARE */
#define ZIP_CM_LZMA       14  /* LZMA (EFS) */
/* 15-17 - Reserved by PKWARE */
#define ZIP_CM_TERSE          18  /* compressed using IBM TERSE (new) */
#define ZIP_CM_LZ77           19  /* IBM LZ77 z Architecture (PFS) */
#define ZIP_CM_WAVPACK        97  /* WavPack compressed data */
#define ZIP_CM_PPMD       98  /* PPMd version I, Rev 1 */

[Den271]

@danielweck

it would be nice to extract the Windows-specific code changes into a separate feature branch. I realize that this means more admin work though (juggling branches in local repository copy), so this may be an unreasonable request. I'll leave it to you :)

I'll re-org the PRs/new branches to separate this two concerns, probably by today/tomorrow

[Den271]

@danielweck,

it would be nice to extract the Windows-specific code changes into a separate feature branch. I realize that this means more admin work though (juggling branches in local repository copy), so this may be an unreasonable request. I'll leave it to you :)

I've separated into the Feature/vs2013 compilation fix #192 PR. Probably this could help with integration with other platforns

[nleme]

We may want to hold on this pull request... So, I created a working copy on my machine of the feature branch "feature/libzip1.0_seek_tell_fix" and I tried to compile the Readium SDK inside Xcode. The problem, then, is that currently I'm getting two compilation errors when I try to do so:

In file included from /Users/nelson_leme/Documents/DevWork/readium-sdk-libzip-mods/ePub3/ThirdParty/libzip/zip_add.c:36:
In file included from /Users/nelson_leme/Documents/DevWork/readium-sdk-libzip-mods/ePub3/ThirdParty/libzip/zipint.h:56:
/Users/nelson_leme/Documents/DevWork/readium-sdk-libzip-mods/ePub3/ThirdParty/libzip/zip.h:59:10: error: 'zipconf.h' file not found with <angled> include; use "quotes" instead
#include <zipconf.h>
         ^~~~~~~~~~~
         "zipconf.h"
In file included from /Users/nelson_leme/Documents/DevWork/readium-sdk-libzip-mods/ePub3/ThirdParty/libzip/zip_add.c:36:
/Users/nelson_leme/Documents/DevWork/readium-sdk-libzip-mods/ePub3/ThirdParty/libzip/zipint.h:124:2: error: unsupported size of off_t
#error unsupported size of off_t
 ^
2 errors generated.

Unless I'm doing something wrong (notice that the only thing I did was: create working copy of feature branch -> open xcodeproj file in Xcode 6 -> Build) if this feature branch is merged into develop, then develop would not build in Xcode. I already verified that the line in question, in the first error, was added in the feature branch.

Daniel do you want to double check what I did? In any case, I'll start investigating this issue, to see what would be needed in order to compile the code in Xcode. My guess is that we will need some "#ifdef APPLE" around those includes.

[danielweck]

Trying to compile with XCode (OSX launcher app):
(1) zipconf.h does not exist
(2) "unsupported size of off_t"
(just like Nelson's report above)
Now, because HAVE_CONFIG_H is undefined at the top of zipint.h => Config.h is not included, and therefore SIZEOF_OFF_T is undefined (thus the second error). So I replaced zipconf.h with Config.h, and the compilation moves further ahead...except it stops at errors in zip_entry_free.c:
"Semantic Issue: No member named ch_filename in struct zip_entry" etc.

[Den271]

Hi Nelson,

'zipconf.h' file not found with include; use "quotes" instead

it seems like this is a header paths configuration issue, for the windows configuration epub3.vcxproj has the following include paths:
$(MSBuildProjectDirectory)\include;
$(MSBuildProjectDirectory)\ReadiumSDK\include;
$(MSBuildProjectDirectory)....\ePub3;
$(MSBuildProjectDirectory)....\ePub3\utilities;
$(MSBuildProjectDirectory)....\ePub3\ePub;
$(MSBuildProjectDirectory)....\ePub3\xml;

$(MSBuildProjectDirectory)\ReadiumSDK\Prebuilt\Include;

$(MSBuildProjectDirectory)....\ePub3\ThirdParty;

$(MSBuildProjectDirectory)....\ePub3\ThirdParty\google-url\src;

$(MSBuildProjectDirectory)....\ePub3\ThirdParty\utf8-cpp\include;

$(MSBuildProjectDirectory)....\ePub3\ThirdParty\icu4c\include;

$(MSBuildProjectDirectory)....\Platform\Windows\include\libzip

The highlighted paths has the zipconf.h inside

BTW, “zipconf.h -- platform specific include file

This file was generated automatically by CMake

based on ../cmake-zipconf.h.in.”

So, it would be good to have this file generated for every platform, or just try to copy the previous zipconf.h from the latest working build.

Anyway, it seems to me that I should try the updates for all the platforms, currently working on Android build and configurations

Thanks,
Dennis

From: Nelson Leme [mailto:[email protected]]
Sent: Friday, September 04, 2015 3:06 AM
To: readium/readium-sdk
Cc: DennisPopovDn
Subject: Re: [readium-sdk] This iFeature/libzip1.0 seek tell fix (#191)

We may want to hold on this pull request... So, I created a working copy on my machine of the feature branch "feature/libzip1.0_seek_tell_fix" and I tried to compile the Readium SDK inside Xcode. The problem, then, is that currently I'm getting two compilation errors when I try to do so:

In file included from /Users/nelson_leme/Documents/DevWork/readium-sdk-libzip-mods/ePub3/ThirdParty/libzip/zip_add.c:36:
In file included from /Users/nelson_leme/Documents/DevWork/readium-sdk-libzip-mods/ePub3/ThirdParty/libzip/zipint.h:56:
/Users/nelson_leme/Documents/DevWork/readium-sdk-libzip-mods/ePub3/ThirdParty/libzip/zip.h:59:10: error: 'zipconf.h' file not found with include; use "quotes" instead
#include
^~~~~~~~~~~
"zipconf.h"
In file included from /Users/nelson_leme/Documents/DevWork/readium-sdk-libzip-mods/ePub3/ThirdParty/libzip/zip_add.c:36:
/Users/nelson_leme/Documents/DevWork/readium-sdk-libzip-mods/ePub3/ThirdParty/libzip/zipint.h:124:2: error: unsupported size of off_t
#error unsupported size of off_t
^
2 errors generated.

Unless I'm doing something wrong (notice that the only thing I did was: create working copy of feature branch -> open xcodeproj file in Xcode 6 -> Build) if this feature branch is merged into develop, then develop would not build in Xcode. I already verified that the line in question, in the first error, was added in the feature branch.

Daniel do you want to double check what I did? In any case, I'll start investigating this issue, to see what would be needed in order to compile the code in Xcode. My guess is that we will need some "#ifdef APPLE" around those includes.

—
Reply to this email directly or view it on GitHub #191 (comment) . https://github.com/notifications/beacon/AFU9-7o2WR2N5Cg3blMomwLWf3fC4rDOks5ouNfdgaJpZM4FxJlg.gif

[danielweck]

make -f Makefile.am
=>
make_zip_err_str.sh missing file

I guess the ziplib folder does not contain all the necessary files from the lib dist?

[Den271]

make_zip_err_str.sh missing file

it seems like #191 has that file
https://github.com/readium/readium-sdk/pull/191/files#diff-ed1c4bf110539f92a9e2a7af44583202

[Den271]

@danielweck,

Now, because HAVE_CONFIG_H is undefined at the top of zipint.h => Config.h is not included, and therefore SIZEOF_OFF_T is undefined (thus the second error).

Windows project has HAVE_CONFIG_H defined in its configuration.. something like "-DHAVE_CONFIG_H" command line option

[danielweck]

@DennisPopovDn correct, make -f Makefile.am now seems to work fine (I must have pulled an incomplete branch, somehow!). I'm going to try to force the HAVE_CONFIG_H flag.

[danielweck]

Alright, so I am now forcing the HAVE_CONFIG_H flag at the top of zipint.h, which means that Config.h is included (note that I corrected the upper case 'C'), which in turn includes zipconf.h which was re-generated by make -f Makefile.am (by the way, I changed angle brackets to quotes in zip.h, rather than modifying the header path), but I still get errors in zip_entry_free.c:
"Semantic Issue: No member named ch_filename in struct zip_entry"
etc.

[Den271]

but I still get errors in zip_entry_free.c

zip_entry_free.c is not in win project at all..
this is old file

" zip_entry_free.c -- free struct zip_entry
Copyright (C) 1999-2007 Dieter Baron and Thomas Klausner"

there are several other old files, moment.. all that files generates errors because refer to obsolete structs, moment... I'll prepare the list of actual files
from CMakeFileList.txt
SET(LIBZIP_SOURCES
zip_add.c
zip_add_dir.c
zip_add_entry.c
zip_buffer.c
zip_close.c
zip_delete.c
zip_dir_add.c
zip_dirent.c
zip_discard.c
zip_entry.c
zip_err_str.c
zip_error.c
zip_error_clear.c
zip_error_get.c
zip_error_get_sys_type.c
zip_error_strerror.c
zip_error_to_str.c
zip_extra_field.c
zip_extra_field_api.c
zip_fclose.c
zip_fdopen.c
zip_file_add.c
zip_file_error_clear.c
zip_file_error_get.c
zip_file_get_comment.c
zip_file_get_external_attributes.c
zip_file_get_offset.c
zip_file_rename.c
zip_file_replace.c
zip_file_set_comment.c
zip_file_set_external_attributes.c
zip_file_set_mtime.c
zip_file_strerror.c
zip_filerange_crc.c
zip_fopen.c
zip_fopen_encrypted.c
zip_fopen_index.c
zip_fopen_index_encrypted.c
zip_fread.c
zip_get_archive_comment.c
zip_get_archive_flag.c
zip_get_compression_implementation.c
zip_get_encryption_implementation.c
zip_get_file_comment.c
zip_get_name.c
zip_get_num_entries.c
zip_get_num_files.c
zip_io_util.c
zip_memdup.c
zip_name_locate.c
zip_new.c
zip_open.c
zip_rename.c
zip_replace.c
zip_set_archive_comment.c
zip_set_archive_flag.c
zip_set_default_password.c
zip_set_file_comment.c
zip_set_file_compression.c
zip_set_name.c
zip_source_begin_write.c
zip_source_buffer.c
zip_source_call.c
zip_source_close.c
zip_source_commit_write.c
zip_source_crc.c
zip_source_deflate.c
zip_source_error.c
zip_source_filep.c
zip_source_free.c
zip_source_function.c
zip_source_is_deleted.c
zip_source_layered.c
zip_source_open.c
zip_source_pkware.c
zip_source_read.c
zip_source_remove.c
zip_source_rollback_write.c
zip_source_seek.c
zip_source_seek_write.c
zip_source_stat.c
zip_source_supports.c
zip_source_tell.c
zip_source_tell_write.c
zip_source_window.c
zip_source_write.c
zip_source_zip.c
zip_source_zip_new.c
zip_stat.c
zip_stat_index.c
zip_stat_init.c
zip_strerror.c
zip_string.c
zip_unchange.c
zip_unchange_all.c
zip_unchange_archive.c
zip_unchange_data.c
zip_utf-8.c
)

• some windows related ones

This files are up-to-date and related to the latest libzip 1.0.. the other files might be obsolete.. and they hadn't been included into project

I think we could delete the old(obsolete) files from this branch after the integration

[danielweck]

@DennisPopovDn I updated the feature branch to fix the main compile errors (removed obsolete files + updated XCode project file), but we still have:
"undeclared identifier _stricmp" in _zip_name_locate() (see zipint.h #define strcasecmp _stricmp)

[Den271]

_stricmp not found in,,,
in Config.h .. undef
#define HAVE__STRICMP
?
Probably platform specific zipconf.h should have that correct defines..
Or it required some conditional macros depending to the platform

[danielweck]

@DennisPopovDn yes I commented #define HAVE__STRICMP, and now have errors about HAVE_FSEEKO and HAVE_FTELLO, so I added #defines in Config.h (zipconf.h did not have anything) ... compiling...

[danielweck]

zip_source_seek => zip_source_call return negative offset.

[Den271]

it seems like there is some low level error like incorrect direntry structure initialization, I'm trying to recompile it on windows

[Den271]

@danielweck , I was able to reproduce it on MacOS, now investigating

[Den271]

it fails on the following condition

static int
_zip_fseek(FILE *f, zip_int64_t offset, int whence, zip_error_t *error)
{
    if (offset > ZIP_FSEEK_MAX || offset < ZIP_FSEEK_MIN) {
    zip_error_set(error, ZIP_ER_SEEK, EOVERFLOW);
    return -1;
    }

and ZIP_FSEEK_MAX and ZIP_FSEEK_MIN are 16-bit integers, because of some preprocessor defines.. and the current offset is out of bound and failed...

[Den271]

and there are several other issues, in progress
Well, to temporary workaround the issue above I just commented out the above check.. it started to work,
also fixed Sanitised issue in ZipFileByteStream::Open
see:c73aa64

[Den271]

It seems like the current code of this branch works for SDKLauncher-OSX, however some additional testing for seek/tell operation in OSX is required, the question is
What is the usual test scenario for byte-range requests and seek operations under OSX? (Which .epub with media files, etc)

[danielweck]

@DennisPopovDn try any EPUB3 file with a large video, or audio Media Overlays. On OSX / iOS, QuickTime will make HTTP byte-range requests during playback / seeking.

[nleme]

I just double checked and I was able to compile the Readium SDK in Xcode with Daniel's changes. Thanks Daniel! :-) I'll see if I can build the iOS Launcher with that Readium SDK and see if I can play video files fine. That would pretty much settle that the new libzip library is working fine. I'll let you guys know what I find out.

By the way, we still have to try to compile this feature branch in Android. I have not had the time to do that yet, but I'll try after I'm done with running the iOS Launcher.

[nleme]

Unfortunately, I just found out a problem: I was able to compile fine when I was solely opening the xcodeproj for the Readium SDK. However, once I actually switched to the iOS Launcher (which contains the Readium SDK project as a submodule) I was not able to compile anymore. :-( Let me paste below the error:

/Users/nelson_leme/Documents/DevWork/ios-launcher-libzip-mods/readium-sdk/ePub3/ThirdParty/libzip/mkstemp.c:75:8: error: implicit declaration of function 'getpid' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        pid = getpid();
              ^
1 error generated.

I'll try to find out myself what is going wrong here, and also if I can find a fix for this. I'll let you guys know what I find out.

[nleme]

OK! I was able to fix the issue above. All I had to do was to make the following modification at the beginning of the mkstemp.c file:

#include <sys/types.h>
#include <sys/stat.h>

#include <assert.h>
#include <ctype.h>
#include <errno.h>
#include <fcntl.h>
#ifdef _WIN32
#include <io.h>
#endif
#include <stdio.h>
#include <stdlib.h>

#include <sys/types.h> // <=== Added include
#include <unistd.h> // <=== Added include


#ifndef O_BINARY
#define O_BINARY 0
#endif

I was able to compile everything (yey!) but when I tried to link, I'm getting some 60 link errors, all like the one below:

Undefined symbols for architecture x86_64:
  "__zip_buffer_data", referenced from:
      __zip_find_central_dir in libePub3-iOS.a(zip_open.o)
      __zip_read_cdir in libePub3-iOS.a(zip_open.o)
      __zip_cdir_write in libePub3-iOS.a(zip_dirent.o)
      __zip_ef_utf8 in libePub3-iOS.a(zip_dirent.o)

I think this has nothing to do with the libzip changes per se, it is a matter of setting my compiler to build for the right platforms. I'll be working on those and I'll let you know.

[nleme]

I figured out what the problem is. It is pretty simple, actually: some source files (such zip_buffer.c) are not being compiled. Hence, the linker cannot find the assembly code for the functions in those files.

The interesting thing is that the files in question do show up in the project navigator, but for some reason are not being compiled. I'll investigate that and modify the project file as needed.

[nleme]

I just fixed that previous problem. The issue was very much Xcode related. Even though all the new libzip files were included in the project, they were not earmarked to the "ePub-iOS" target, and therefore they were not being compiled. Once I marked all of them as part of the "ePub-iOS" target, all of them got compiled and I did not get any more link errors. :-)

I'm able to fully build the iOS Launcher now. I will try running the iOS Launcher soon, including media playback, and then we can know for sure that the new libzip is working fine in iOS.

[nleme]

OK, I just pushed a commit with the changes that I had made to allow the Readium SDK with the new libzip code to be compiled in Xcode.

However, I have no way to confirm that my changes have not messed up things in Windows. Dennis, can you check if things are still compiling fine in Windows? If not, let me know, and we can work out this (probably by adding some #ifdef APPLE - but I'm avoiding those, unless they are actually necessary).

Also, I'm going to proceed now with running some tests with the iOS Launcher and playing some media files. I'll let you guys know what I find out, but if we pass those tests, then we are good to go as regards to iOS (we still need to check Android).

[Den271]

@nleme, thanks, I'll check the windows compilation soon.

[nleme]

OK! I was able to open some ePub3 books fine with the new libzip code. More importantly, though, I was able to open our traditional "Shared Culture" book, and play the video that it contains on its first page. We know for a fact that Quicktime, when playing this video, asks for specific ranges inside the video file. And the video played fine. :-)

Therefore, I think that the new libzip code is working fine, including with byte ranges. Of course, it will be good to have more testing, but I would say that from the point of view of iOS, we are good to go to merge this into develop.

However, now we need to do the same process on Android. I'm going to start that now, and I'll keep this thread up to date with my findings.

[danielweck]

Tested with OSX launcher app. Works fine (including Media Overlays, i.e. HTTP byte ranges)

[danielweck]

Follow-up on this:
Note DRM-Inside's now merged PR (zip f_seek):
https://github.com/readium/readium-sdk/pull/219/files#diff-e8c489d80c21c04a0f36514e70875fd3