Source length calculation in ContentDataSource changed

[koral--]

Fixes UnrecognizedInputFormatExceptions with valid files loaded from content:// URIs

[ojw28]

Thanks for finding this! Please find a couple of comments inlined.

[ojw28]

Rather than doing this, I think it would be preferable to modify L84 to do:

bytesRemaining = assetFileDescriptor.getLength();
if (bytesRemaining == AssetFileDescriptor.UNKNOWN_LENGTH) {
  // The asset must extend to the end of the file.
  bytesRemaining = inputStream.available();
  if (bytesRemaining == 0) {
    // FileInputStream.available() returns 0 if the remaining length cannot be determined, or
    // if it's greater than Integer.MAX_VALUE. We don't know the true length in either case,
    // so treat as unbounded.
    bytesRemaining = C.LENGTH_UNSET;
  }
}

Which makes it explicit what's going on (and also allows bytesRemaining to be greater than Integer.MAX_VALUE, at least in theory).

[ojw28]

Please split into separate AssetDataSourceTest and ContentDataSourceTest classes. Thanks.

[koral--]

Changed.
It seems that there is also one more unhandled edge case.
ContentResolver#openAssetFileDescriptor() may return null which will result in NPE in L73.
Should that be also fixed in this PR?

[ojw28]

Do you understand under what conditions null can actually be returned, and have you managed to reproduce? I see it's marked as @Nullable, but I'm finding it hard to work out when null can actually be returned (I followed the source code as far as IContentProvider). It seems plausible null isn't ever returned, and it's just a result of code going through the IContentProvider interface.

If you do understand the conditions, and/or if handling is trivial anyway (which I guess it is), then yes feel free to fix in this PR. What will you do in that case though? Throw something? What?

[koral--]

There may be a custom ContentProvider which returns null from one of its open* methods family e.g. if TestDataProvider#openAssetFile() from unit test in this PR returns null it will be propagated to ContentResolver#openAssetFileDescriptor().
It is even explicitly mentioned in the documentation: https://developer.android.com/reference/android/content/ContentProvider.html#openFile(android.net.Uri, java.lang.String).

What will you do in that case though? Throw something? What?

something like that: throw new IOException("Could not open " + uri)

[ojw28]

Maybe FileNotFoundException? IOException seems fine as well.

[koral--]

Done.

[ojw28]

Thanks!

[ojw28]

Hm, I tried to modify this test to also validate that the data being read was correct, but it appears the data read from the DataSource is not as expected. Any ideas?

[ojw28]

It is actually broken. Seems reverting back to using assetFileDescriptor.createInputStream(), as you had it originally, fixes the problem. As does adding inputStream.skip(assetFileDescriptor.getStartOffset()).

I'm working on a bit of a cleanup change, so will merge a fix as part of that.

[ojw28]

Fixed in 8bb6439