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

ContentDataSource size determination issue #3426

Closed
axet opened this issue Nov 5, 2017 · 8 comments
Closed

ContentDataSource size determination issue #3426

axet opened this issue Nov 5, 2017 · 8 comments
Labels

Comments

@axet
Copy link

axet commented Nov 5, 2017

DefaultDataSource uses ContentDataSource to open urls. But ContentDataSource.open not following android documentations exactly and cause issues when opening url's in "r" mode only.

You can read ContentProvider.openFile documentation:

 * If opened with the exclusive "r" or "w" modes, the returned
 * ParcelFileDescriptor can be a pipe or socket pair to enable streaming
 * of data. Opening with the "rw" or "rwt" modes implies a file on disk that
 * supports seeking.

Since ContentDataSource uses "r" mode, ContentProvider may return Pipe instead of file and resulting bytes (total content length) will be miss detected:

      bytesRemaining = inputStream.available();

Problem is, if ContentProvider retuns Pipe, ExoPlayer will stop playing audio after few seconds after fist seek action and close pipe before it fully readed. To fix this we may use open "rw" mode or wait until we reach EOF.

@ojw28 ojw28 changed the title DefaultDataSource issue ContentDataSource size determination issue Nov 5, 2017
@ojw28 ojw28 added the bug label Nov 5, 2017
@ojw28
Copy link
Contributor

ojw28 commented Nov 5, 2017

I put some pending comments on the proposed fix.

@ojw28
Copy link
Contributor

ojw28 commented Nov 6, 2017

I played around with this a little by making a ContentProvider that returns a pipe. It looks like:

  • If the content provider sets the length of the AssetFileDescriptor then things just work, since assetFileDescriptor.getLength() is set.
  • If the content provider sets the length to AssetFileDescriptor.UNKNOWN_LENGTH then inputStream.available() doesn't give us the right thing. It doesn't look like inputStream.getChannel().size() gives the correct size either though. I get 0 back when I query this with my test implementation. I think we should just set bytesRemaining directly to C.LENGTH_UNSET in the case that assetFileDescriptor.getLength() doesn't provide a value.

@axet
Copy link
Author

axet commented Nov 6, 2017

  1. correct
  2. inputStream.getChannel().size() - only gives right size when FD is a file. So, it is safe to call for booth pipe/socket/file - it will return zero or correct size, inputStream.available() - only return correct size when FD is a file. So. channel size - is safe to use.

@ojw28
Copy link
Contributor

ojw28 commented Nov 6, 2017

Are you certain size() will return 0 or the correct size? It's documented as returning a "current" size, which suggests the value may change.

@axet
Copy link
Author

axet commented Nov 6, 2017

I read it some where, can not find anymore, I think by "current" they mean size can be changed due to write operations.

@ojw28
Copy link
Contributor

ojw28 commented Nov 6, 2017

Ok, thanks. Can you provide some sample code showing how to create a ContentProvider that results in available() returning the incorrect value and size() returning non-zero? I can reproduce the problem where available() returns the incorrect value, but I can't get size() to return a non-zero value at the same time.

@axet
Copy link
Author

axet commented Nov 6, 2017

Simple. I use ContentProvider urls to extract audio files from torrent/rar/zip archives for my torrent application.

https://gitlab.com/axet/android-torrent-client/blob/master/app/src/main/java/com/github/axet/torrentclient/services/TorrentContentProvider.java

I patched it already. So, it returns AssetFileDescriptor.length, but if you drop such code it ExoPlayer fails again.

In short 1) create pipe 2) create thread and start copying file to output steram. And second option, when you return file from file system (if content is torrent without archive):

                    ParcelFileDescriptor[] ff = ParcelFileDescriptor.createPipe();
                    final ParcelFileDescriptor r = ff[0];
                    final ParcelFileDescriptor w = ff[1];
                    Thread thread = new Thread(new Runnable() {
                        @Override
                        public void run() {
                            OutputStream os = new ParcelFileDescriptor.AutoCloseOutputStream(w);
                            try {
                                file.file.copy(os);
                            } catch (RuntimeException e) {
                                Log.d(TAG, "Error reading archive", e);
                            } finally {
                                try {
                                    os.close();
                                } catch (IOException e) {
                                    Log.d(TAG, "copy close error", e);
                                }
                            }
                        }
                    });
                    thread.start();
                    return r;

EDIT: if available() returning the incorrect value then size() will be zero allways.

ojw28 added a commit that referenced this issue Nov 7, 2017
Issue: #3426

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=174700804
@ojw28
Copy link
Contributor

ojw28 commented Nov 7, 2017

We pushed a change that should fix this (see above). Please double check. Thanks!

@ojw28 ojw28 closed this as completed Nov 7, 2017
ojw28 added a commit that referenced this issue Nov 13, 2017
Issue: #3426

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=174700804
@google google locked and limited conversation to collaborators Mar 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants