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.

Sign up for GitHub

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

Update ContentDataSource.java #3428

Closed
wants to merge 1 commit into from
Closed

Update ContentDataSource.java #3428

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion library/core/src/main/java/com/google/android/exoplayer2/upstream/ContentDataSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ public long open(DataSpec dataSpec) throws ContentDataSourceException {
long assetFileDescriptorLength = assetFileDescriptor.getLength();
if (assetFileDescriptorLength == AssetFileDescriptor.UNKNOWN_LENGTH) {
// The asset must extend to the end of the file.
bytesRemaining = inputStream.available();
bytesRemaining = ((FileInputStream) inputStream).getChannel().size();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's possible for multiple assets to be in a single file. In which case is size() the size of just this one, or of all of them? I think you might want to subtract position() of the FileChannel to get correct behavior?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what do you mean, FileChannel one per file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the uri for which the source is being opened necessarily corresponds to the whole file though. Else this code wouldn't need to be using assetFileDescriptor.getStartOffset as above? If the uri only corresponds to a part of the file, that needs to be accounted for.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if the uri does correspond to the whole file, you'd need to subtract skipped if it's non-zero.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then correct line is:

bytesRemaining = ((FileInputStream) inputStream).getChannel().size() - assetFileDescriptor.getStartOffset

if (bytesRemaining == 0) {
// FileInputStream.available() returns 0 if the remaining length cannot be determined,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment doesn't seem to make sense given then change being made? Does the if block itself still make sense?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bytesRemaining = inputStream.available()

  • for the momen when you open Pipe this call can return 0.

bytesRemaining = ((FileInputStream) inputStream).getChannel().size();

  • FileDescription for correct file in file system, this call will return correct file size.

Again. I do not understand, why you asking this. Propbably need more testing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the previous code we actually called inputStream.available() and then did something conditionally on the value being 0. We're not making an inputStream.available() call any more, so the comment doesn't make sense. Can size() return 0? If not why is this if block still here? If so the comment needs changing.

// or if it's greater than Integer.MAX_VALUE. We don't know the true length in either
Expand Down