Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace NanoHTTPd with AndroidAsync (fixes media playback issues) #50

Merged
merged 16 commits into from
Jan 28, 2015

Conversation

jccr
Copy link
Member

@jccr jccr commented Jan 22, 2015

Fixes issues:

Potentially fixes:

Juan Corona and others added 15 commits December 22, 2014 08:57
… helps make the android media clients work better.
…ll Request diff only includes actually modified code)
…final Pull Request diff only includes actually modified code)"

This reverts commit 78d9b83.
…acter (as per the original file) ... sorry for the messy commit history
…TION_RESET while it is still being sent over the socket.
…rator) note: made sure that object calling .equals() is not null
@danielweck
Copy link
Member

Hi @jccr I reviewed your latest commit and I fixed the tab vs. 4-spaces indentation inconsistency.

I also noticed your addition in InputStream skip() and reset() in the non-isRange case. Note that with encrypted media, this is actually likely to result in incorrect stream positioning, as the "Content Filter Chain" mechanism is designed only to handle read() operations. See here:

https://github.com/readium/readium-sdk/blob/develop/ePub3/ePub/filter_chain_byte_stream.cpp#L71

https://github.com/readium/readium-sdk/blob/develop/ePub3/ePub/filter_chain_byte_stream_range.cpp#L69

However, I do not think that this is a ship-stopper, because as far as I can tell the AndroidAsync HTTP server does not invoke these methods anyway.

Ultimately, perhaps we could mimic what I did with NanoHTTPD server: implement a subset of InputStream to restrict usage to only Readium's required stream-access methods (I'm not sure about AndroidAsync's internals, but perhaps this would allow getting rid of the no-parameter read() method as well). Alternatively, perhaps we could just throw NotImplementedException or something along these lines to make it clear that the "Content Filter Chain" backend does not support arbitrary seeking, only a well-defined read-based protocol.

@nleme am I getting this right? It would be great to hear your thoughts before we merge, just to make sure we're not missing anything glaringly obvious :)

So, as far as I am concerned, we're ready to merge to develop. I am logging a separate issue to record the above comments.

@danielweck
Copy link
Member

PS: regarding InputStream skip() usage in AndroidAsync, note that the returned value does not necessarily reflect the position in the raw underlying ByteStream in the case of encrypted media. However we can easily work around that without modifying AndroidAsync's source code:

if (start != inputStream.skip(start))
    throw new StreamSkipException("skip failed to skip requested amount");

https://github.com/koush/AndroidAsync/blob/master/AndroidAsync/src/com/koushikdutta/async/http/server/AsyncHttpServerResponseImpl.java#L278

As for Util.pump(): only this read() method signature is used:

int read = is.read(b.array(), 0, (int)toRead);

https://github.com/koush/AndroidAsync/blob/master/AndroidAsync/src/com/koushikdutta/async/Util.java#L86

Conclusion: at a later date we will definitely be able to easily restrict the InputStream API that AndroidAsync consumes. For now, let's just merge to develop (pending Nelson's approval).

@rkwright
Copy link
Member

I agree that I would like to sets of eyes, i.e. I would like to have Nelson take a look if he can be spared.

@nleme
Copy link
Contributor

nleme commented Jan 26, 2015

I'm looking at this now. I'll send my comments a little later.

@nleme
Copy link
Contributor

nleme commented Jan 26, 2015

I took a look and this looks good to me too. It is ready to merge. :-)

I didn't see any big hole on this, so I think we are good to go. We can always make improvements, but that should be out of develop, we don't need to keep this feature branch anymore.

As I commented in private e-mail as well, I think it's a good change to move away from NanoHTTP. I thought that NanoHTTP was too small, and based on what I looked about AndroidAsync, it seems to be a little bit more beefed up. The only thing I'll miss is the fact that NanoHTTP, because it was so small (just a single file!) could have its source code included in our project, instead of the JAR file. That is not a big deal, though, because we can always take a look at the source code for AndroidAsync right here on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants