Skip to content

Commit

Permalink
Final chunk of refactoring for Byte Range streams
Browse files Browse the repository at this point in the history 
This checkin includes the final chunk of refactoring to support Byte Range streams. Evidently, there may be further checkins as
we take feedback on these changes, as well as we fix bugs. But this should be the last piece of redesign in terms of the
latest discussions we had around Byte Range streams.

To summarize very quickly, this change removes any change previously made in the Cocoa HTTP Server code. It also removes
the temporary way that the code had to decide between using Byte Ranges or not. Finally, it also refactors how Byte Streams are
built upon for usage by the Cocoa HTTP Server: it first gets the "raw" byte stream, but later switches to the byte streams that
can have ContentFilter objects (and that may support byte ranges).
  • Loading branch information
@nleme
nleme committed Nov 14, 2014
1 parent 2363be1 commit cb8869b
Show file tree
Hide file tree
Showing 12 changed files with 189 additions and 153 deletions.
9 changes: 8 additions & 1 deletion Platform/Apple/RDServices/Main/RDPackage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,13 @@
- (instancetype)initWithPackage:(void *)package;

// Returns the resource at the given relative path or nil if it doesn't exist.
- (RDPackageResource *)resourceAtRelativePath:(NSString *)relativePath isRangeRequest:(BOOL)isRangeRequest;
- (RDPackageResource *)resourceAtRelativePath:(NSString *)relativePath;

// Gets the current Byte Stream and returns the proper Byte Stream for the case.
// There can be three possible byte streams:
// - A simple ZipFileByteStream when no ContentFilter objects apply for this resource.
// - A FilterChainByteStreamRange when a Byte Range request has been made, and only one ContentFilter object applies.
// - A FilterChainByteStream when it is not a Byte Range request or more than one ContentFilter applies.
- (void *)getProperByteStream:(NSString *)relativePath currentByteStream:(void *)currentByteStream isRangeRequest:(BOOL)isRangeRequest;

@end
99 changes: 52 additions & 47 deletions Platform/Apple/RDServices/Main/RDPackage.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@


@interface RDPackage() <RDPackageResourceDelegate> {
@private std::vector<std::shared_ptr<ePub3::ByteStream>> m_byteStreamVector;
@private RDMediaOverlaysSmilModel *m_mediaOverlaysSmilModel;
@private RDNavigationElement *m_navElemListOfFigures;
@private RDNavigationElement *m_navElemListOfIllustrations;
Expand Down Expand Up @@ -260,18 +259,6 @@ - (NSString *)packageID {
}


- (void)packageResourceWillDeallocate:(RDPackageResource *)packageResource {
for (auto i = m_byteStreamVector.begin(); i != m_byteStreamVector.end(); i++) {
if (i->get() == packageResource.byteStream) {
m_byteStreamVector.erase(i);
return;
}
}

NSLog(@"The byte stream was not found!");
}


- (RDNavigationElement *)pageList {
if (m_navElemPageList == nil) {
ePub3::NavigationTable *navTable = m_package->PageList().get();
Expand Down Expand Up @@ -318,7 +305,7 @@ - (NSString *)renditionOrientation {
}


- (RDPackageResource *)resourceAtRelativePath:(NSString *)relativePath isRangeRequest:(BOOL)isRangeRequest {
- (RDPackageResource *)resourceAtRelativePath:(NSString *)relativePath {
if (relativePath == nil || relativePath.length == 0) {
return nil;
}
Expand All @@ -330,51 +317,26 @@ - (RDPackageResource *)resourceAtRelativePath:(NSString *)relativePath isRangeRe
}

ePub3::string s = ePub3::string(relativePath.UTF8String);
ePub3::ConstManifestItemPtr manifestItem = m_package->ManifestItemAtRelativePath(s);

if (manifestItem == nullptr) {
NSLog(@"Relative path '%@' does not have a manifest item!", relativePath);
return nil;
}

ePub3::ManifestItemPtr m = std::const_pointer_cast<ePub3::ManifestItem>(manifestItem);

// TODO: the following part of the code is still WORK IN PROGRESS.
// We are currently in the middle of a refactoring process,
// but we should check in to avoid losing all the other changes
// that we already made. Therefore, this piece of code below should
// change quite a lot in future checkins (or disappear completely).

ePub3::ByteStreamPtr byteStream = nullptr;
if (isRangeRequest)
{
byteStream = m_package->GetFilterChainByteStreamRange(m);
if (byteStream == nullptr)
{
byteStream = m_package->GetFilterChainByteStream(m);
}
}
else
{
byteStream = m_package->GetFilterChainByteStream(m);
}

std::unique_ptr<ePub3::ByteStream> byteStream = m_package->ReadStreamForRelativePath(s);
if (byteStream == nullptr)
{
NSLog(@"Relative path '%@' does not have a byte stream!", relativePath);
NSLog(@"This resource is not present in the book.");

This comment has been minimized.

Sign in to view

Copy link
@danielweck

danielweck Nov 16, 2014

Member

I regularly come across EPUBs that have missing resources, so I suggest that we continue to log "relativePath" in the console.
return nil;
}

// ----- End of Work in Progress code (see previous comment) -----
ePub3::ConstManifestItemPtr manifestItem = m_package->ManifestItemAtRelativePath(s);
if (manifestItem == nullptr) {
NSLog(@"Relative path '%@' does not have a manifest item!", relativePath);
return nil;
}

RDPackageResource *resource = [[RDPackageResource alloc]
initWithDelegate:self
byteStream:byteStream.get()
byteStream:byteStream.release()
package:self
relativePath:relativePath];

if (resource != nil) {
m_byteStreamVector.push_back(std::move(byteStream));
const ePub3::ManifestItem::MimeType &mediaType = manifestItem->MediaType();
resource.mimeType = [NSString stringWithUTF8String:mediaType.c_str()];
}
Expand Down Expand Up @@ -423,4 +385,47 @@ - (NSString *)title {
}


- (void *)getProperByteStream:(NSString *)relativePath currentByteStream:(void *)currentByteStream isRangeRequest:(BOOL)isRangeRequest {
if (relativePath == nil || relativePath.length == 0) {
return nil;
}

NSRange range = [relativePath rangeOfString:@"#"];

if (range.location != NSNotFound) {
relativePath = [relativePath substringToIndex:range.location];
}
ePub3::string s = ePub3::string(relativePath.UTF8String);

ePub3::ConstManifestItemPtr manifestItem = m_package->ManifestItemAtRelativePath(s);
if (manifestItem == nullptr) {
NSLog(@"Relative path '%@' does not have a manifest item!", relativePath);
return nil;
}
ePub3::ManifestItemPtr m = std::const_pointer_cast<ePub3::ManifestItem>(manifestItem);

size_t numFilters = m_package->GetFilterChainSize(m);
ePub3::ByteStream *byteStream = nullptr;
ePub3::SeekableByteStream *rawInput = dynamic_cast<ePub3::SeekableByteStream *>((ePub3::ByteStream *)currentByteStream);

if (numFilters <= 0)
{
byteStream = (ePub3::ByteStream *) currentByteStream;
}
else if (numFilters == 1 && isRangeRequest)
{
byteStream = m_package->GetFilterChainByteStreamRange(m, rawInput).release();
if (byteStream == nullptr)
{
byteStream = m_package->GetFilterChainByteStream(m, rawInput).release();
}
}
else
{
byteStream = m_package->GetFilterChainByteStream(m, rawInput).release();
}

return byteStream;
}

@end
2 changes: 1 addition & 1 deletion Platform/Apple/RDServices/Main/RDPackageResource.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
@property (nonatomic, readonly) NSData *data;
@property (nonatomic, copy) NSString *mimeType;
@property (nonatomic, readonly) RDPackage *package;
@property (nonatomic) BOOL isRangeRequest;

// The relative path associated with this resource.
@property (nonatomic, readonly) NSString *relativePath;
Expand All @@ -57,6 +58,5 @@

- (NSData *)readDataOfLength:(NSUInteger)length;
- (void)setOffset:(UInt64)offset;
- (BOOL)isByteRangeResource;

@end
98 changes: 43 additions & 55 deletions Platform/Apple/RDServices/Main/RDPackageResource.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,15 @@

@interface RDPackageResource() {
@private UInt8 m_buffer[4096];
@private ePub3::ByteStream *m_byteStream;
@private std::unique_ptr<ePub3::ByteStream> m_byteStream;
@private NSUInteger m_contentLength;
@private UInt64 m_offset;
@private NSData *m_data;
@private __weak id <RDPackageResourceDelegate> m_delegate;
@private RDPackage *m_package;
@private NSString *m_relativePath;
@private BOOL m_isRangeRequest;
@private BOOL m_hasProperStream;
}

@end
Expand All @@ -55,60 +57,38 @@ @interface RDPackageResource() {
@implementation RDPackageResource


@synthesize byteStream = m_byteStream;
@synthesize contentLength = m_contentLength;
@synthesize package = m_package;
@synthesize relativePath = m_relativePath;
@synthesize isRangeRequest = m_isRangeRequest;

- (void *)byteStream {
return m_byteStream.get();
}


- (NSData *)data {
if (m_data == nil) {
NSMutableData *md = [[NSMutableData alloc] initWithCapacity:
m_contentLength == 0 ? 1 : m_contentLength];

ePub3::FilterChainByteStreamRange *filterStream = dynamic_cast<ePub3::FilterChainByteStreamRange *>(m_byteStream);
if (filterStream != nullptr) {

ePub3::ByteRange range;
range.Location(0);
range.Length(sizeof(m_buffer));

NSUInteger totalRead = 0;

std::size_t count = 1;
while (count > 0) {

count = filterStream->ReadBytes(m_buffer, sizeof(m_buffer), range);

if (count <= 0) break;

[md appendBytes:m_buffer length:count];

totalRead += count;

range.Location(range.Location() + count);
}

//TODO: comment/remove these debug messages
if (totalRead != m_contentLength) {
NSLog(@"2) length difference between filtered and raw bytes: (%lu %lu - %@)", (unsigned long)totalRead, (unsigned long)m_contentLength, m_relativePath);
}
else
{
NSLog(@"2) Correct: (%lu %lu - %@)", (unsigned long)totalRead, (unsigned long)m_contentLength, m_relativePath);
}
NSMutableData *md = [[NSMutableData alloc] initWithCapacity:m_contentLength == 0 ? 1 : m_contentLength];

if (!m_hasProperStream)

This comment has been minimized.

Sign in to view

Copy link
@danielweck

danielweck Nov 16, 2014

Member

I suggest we extract this routine into its own method "ensureProperByteStream" (it is used twice in this class), and we can also update m_contentLength immediately after m_byteStream.reset()
(see my other comment about premature computation of raw bytes vs. decrypted resource size)
{
ePub3::ByteStream *byteStream = m_byteStream.release();
m_byteStream.reset((ePub3::ByteStream *)[m_package getProperByteStream:m_relativePath currentByteStream:byteStream isRangeRequest:m_isRangeRequest]);
m_hasProperStream = YES;
}
else {
while (YES) {
std::size_t count = m_byteStream->ReadBytes(m_buffer, sizeof(m_buffer));

if (count == 0) {
break;
}

[md appendBytes:m_buffer length:count];

while (YES)
{
std::size_t count = m_byteStream->ReadBytes(m_buffer, sizeof(m_buffer));
if (count <= 0)
{
break;
}

[md appendBytes:m_buffer length:count];
}

m_data = md;
}

Expand All @@ -117,7 +97,6 @@ - (NSData *)data {


- (void)dealloc {
[m_delegate packageResourceWillDeallocate:self];

This comment has been minimized.

Sign in to view

Copy link
@danielweck

danielweck Nov 16, 2014

Member

I think that the class initializer (see below) can be renamed from initWithDelegate to initWithByteStream, and the parameter (id )delegate can be removed. Obviously, the m_delegate class member can be removed too.
}


Expand All @@ -132,11 +111,13 @@ - (void)dealloc {
}

if (self = [super init]) {
m_byteStream = (ePub3::ByteStream *)byteStream;
m_byteStream.reset((ePub3::ByteStream *)byteStream);
m_contentLength = m_byteStream->BytesAvailable();

This comment has been minimized.

Sign in to view

Copy link
@danielweck

danielweck Nov 16, 2014

Member

m_contentLength is computed prematurely (raw bytes length), because of encrypted file sizes that are greater (or smaller) than the original zip resource. See this change log:
2363be1#diff-a91f9a3904c74f736c10bfcc809845ecL46
In other words, the m_hasProperStream has to be YES (getProperByteStream already invoked), which in turn implies that m_isRangeRequest has already been initialized...which means that the first query for m_contentLength should be preceded by setOffset (not the method in this class, but the same-name function on the calling side RDPackageResourceResponse which invokes the .isRangeRequest setter).
m_delegate = delegate;
m_package = package;
m_relativePath = relativePath;
m_isRangeRequest = NO;
m_hasProperStream = NO;

if (m_contentLength == 0) {
NSLog(@"The resource content length is zero! %@", m_relativePath);
Expand All @@ -149,9 +130,22 @@ - (void)dealloc {

- (NSData *)readDataOfLength:(NSUInteger)length {
NSMutableData *md = [[NSMutableData alloc] initWithCapacity:length];

if (!m_hasProperStream)
{
ePub3::ByteStream *byteStream = m_byteStream.release();
m_byteStream.reset((ePub3::ByteStream *)[m_package getProperByteStream:m_relativePath currentByteStream:byteStream isRangeRequest:m_isRangeRequest]);
m_hasProperStream = YES;
}

if (!m_isRangeRequest)

This comment has been minimized.

Sign in to view

Copy link
@danielweck

danielweck Nov 16, 2014

Member

BUG:
length may be an arbitrary HTTP header length, greater than the available bytes from the "data" prefetch method.

BUGFIX:

if (!m_isRangeRequest)
    {
        NSData* prefetchedData = [self data];
        NSUInteger prefetchedDataLength = [prefetchedData length];
        NSUInteger adjustedLength = prefetchedDataLength < length ? prefetchedDataLength : length;
        NSMutableData *md = [[NSMutableData alloc] initWithCapacity:adjustedLength];
        [md appendBytes:prefetchedData.bytes length:adjustedLength];
        return md;
    }
{
[md appendBytes:[self data].bytes length:length];
return md;
}

ePub3::FilterChainByteStreamRange *filterStream =
dynamic_cast<ePub3::FilterChainByteStreamRange *>(m_byteStream);
dynamic_cast<ePub3::FilterChainByteStreamRange *>(m_byteStream.get());

if (filterStream == nullptr) {

This comment has been minimized.

Sign in to view

Copy link
@danielweck

danielweck Nov 16, 2014

Member

I think there is a bug (to be verified): are we covering the case whereby getProperByteStream returns a plain old SeekableByteStream (i.e. no ContentFilter), in which case readDataOfLength must use seek() ?
The reason why we do not catch this right now is because PassThrough ContentFilter captures most audio/video resources, so the number of applied filter is not zero! (which skews our test results).
I am checking with SDKLauncher-OSX...

This comment has been minimized.

Sign in to view

Copy link
@danielweck

danielweck Nov 16, 2014

Member

Bug confirmed in OSX and iOS.
Fixed in OSX:
readium/SDKLauncher-OSX@4f643c7#diff-0bff146e47b0e84a122cb3a843ac94a7L176

iOS TODO!
NSLog(@"The byte stream is not a FilterChainSyncStream!");
Expand All @@ -166,6 +160,7 @@ - (NSData *)readDataOfLength:(NSUInteger)length {
std::size_t count = filterStream->ReadBytes(m_buffer, sizeof(m_buffer), range);
[md appendBytes:m_buffer length:count];
totalRead += count;
m_offset += count;
range.Location(range.Location() + count);

if (count != range.Length()) {
Expand All @@ -186,11 +181,4 @@ - (void)setOffset:(UInt64)offset {
m_offset = offset;
}

- (BOOL)isByteRangeResource
{
ePub3::FilterChainByteStreamRange *filterStream = dynamic_cast<ePub3::FilterChainByteStreamRange *>(m_byteStream);
return (filterStream != nullptr);
}


@end
30 changes: 5 additions & 25 deletions Platform/Apple/RDServices/Main/RDPackageResourceConnection.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ - (NSString *)htmlFromData:(NSData *)data {
return html;
}

- (NSObject <HTTPResponse> *)httpResponseForMethod:(NSString *)method URI:(NSString *)path isRangeRequest:(BOOL)isRangeRequest {
- (NSObject <HTTPResponse> *)httpResponseForMethod:(NSString *)method URI:(NSString *)path {
if (m_packageResourceServer == nil ||
method == nil ||
![method isEqualToString:@"GET"] ||
Expand Down Expand Up @@ -171,7 +171,7 @@ - (NSString *)htmlFromData:(NSData *)data {
// resource byte stream, which may lead to instability.

@synchronized ([RDPackageResourceServer resourceLock]) {
RDPackageResource *resource = [m_packageResourceServer.package resourceAtRelativePath:path isRangeRequest:isRangeRequest];
RDPackageResource *resource = [m_packageResourceServer.package resourceAtRelativePath:path];

if (resource == nil) {
NSLog(@"No resource found! (%@)", path);
Expand Down Expand Up @@ -252,29 +252,9 @@ - (NSString *)htmlFromData:(NSData *)data {
}
}
}

if (!resource.isByteRangeResource) {
// This resource is not one that can be fetched by byte ranges,
// so just put the whole thing in memory.

NSData *data = resource.data;

if (data != nil) {
RDPackageResourceDataResponse *dataResponse = [[RDPackageResourceDataResponse alloc]
initWithData:data];

if (resource.mimeType) {
dataResponse.contentType = resource.mimeType;
}

response = dataResponse;
}
}
else {
RDPackageResourceResponse *resourceResponse = [[RDPackageResourceResponse alloc]
initWithResource:resource];
response = resourceResponse;
}

RDPackageResourceResponse *resourceResponse = [[RDPackageResourceResponse alloc] initWithResource:resource];
response = resourceResponse;
}
}

Expand Down
1 change: 1 addition & 0 deletions Platform/Apple/RDServices/Main/RDPackageResourceResponse.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ - (void)setOffset:(UInt64)offset {

@synchronized ([RDPackageResourceServer resourceLock]) {
[m_resource setOffset:offset];
m_resource.isRangeRequest = YES;
}
}

Expand Down
Loading

0 comments on commit cb8869b

Please sign in to comment.