google / ExoPlayer

This project is deprecated and stale. The latest ExoPlayer code is available in https://github.com/androidx/media
https://developer.android.com/media/media3/exoplayer
Apache License 2.0
21.72k stars 6.03k forks source link

Not able to implement caching using 2.12.3 #8494

Closed tfrysinger closed 3 years ago

tfrysinger commented 3 years ago

Using exoplayer2 2.12.13 Using Android Studio 4.1.2 Using Samsung S7, Android 8.0.0

I have documented this issue in depth in a stack overflow entry here: https://stackoverflow.com/questions/64949205

I have also used the instructions here in my app: https://exoplayer.dev/customization.html to cache video streamed from Dropbox, and I get the same result - a 0 byte file created in the designated cache file on the device.

The video plays, but is not cached.

ojw28 commented 3 years ago

I can't see anything obviously wrong with your code. Have you tried building against ExoPlayer as source code, and attaching a debugger (e.g., to step through CacheDataSource in order to understand what's going on)? I think that would be the best next step.

If you'd like further assistance with this then you'll probably have to provide a small sample app, as source code, that reproduces the issue. The best way to do this is to fork this repository and make some minor changes to the main ExoPlayer demo app in order to reproduce your issue.

tfrysinger commented 3 years ago

I pulled the code and started to do what you say - but it is so intertwined with other classes that it is a bit hard for me to know where to set breakpoints to walk through things.

Can you offer any guidance? Maybe ensure that the TeeDataSource in the constructor is getting initialized, then put breakpoints within that class on its read method (where the dataSink.write() occurs)?

ojw28 commented 3 years ago

A good place to start would be CacheDataSource.openNextSource.

Each call to that method decides whether to ignore the cache, read from the cache, or write to the cache. There are some inline comments that to some extent explain what each code path within that method correspond to.

For example, if stepping through that method ends up with this branch being executed, that would be indicative of the cache being ignored for some reason. You can then work backwards, by working out which method is being called to set nextSpan, and then stepping through that method call to work out why it's setting it to null.

If that doesn't help, I'd also try setting some break points in SimpleCache. For example here, to see if files are being committed to the cache. And here, to see if files are being removed from the cache.

tfrysinger commented 3 years ago

OK so I walked through openNextSource(). I verified it is using the cacheWriteDataSource (TeeDataSource). I also see that the "resolvedLength" is -1, what ramifications might this have?

isWritingToCache() returns true throughout.

nextSpan is being set via this:

} else if (blockOnCache) { try { nextSpan = cache.startReadWrite(key, readPosition, bytesRemaining); } catch (InterruptedException e) { Thread.currentThread().interrupt(); throw new InterruptedIOException(); }

and it is NOT returning as null! Because I also see this getting called:

    } else {
        // Data is not cached, and data is not locked, read from upstream with cache backing.
        long length;
        if (nextSpan.isOpenEnded()) {
            length = bytesRemaining;
        } else {
            length = nextSpan.length;
            if (bytesRemaining != C.LENGTH_UNSET) {
                length = min(length, bytesRemaining);
            }
        }
        nextDataSpec =
                requestDataSpec.buildUpon().setPosition(readPosition).setLength(length).build();
        if (cacheWriteDataSource != null) {
            nextDataSource = cacheWriteDataSource;
        } else {
            nextDataSource = upstreamDataSource;
            cache.releaseHoleSpan(nextSpan);
            nextSpan = null;
        }
    }

bytesRemaining (and thus length) is being set to -1.

currentHoldSpan is being set to nextSpan, which shows in the debugger as "[0,-1]"

This continues throughout the buffering process.

I'll walk through SimpleCache next.

tfrysinger commented 3 years ago

OK so here is a little more info:

I put a breakpoint in CacheDataSource.open(). At that point, this.ignoreCacheForUnsetLengthRequests returns false, however the dataSpec passed in returns true from dataSpec.isFlagSet(FLAG_DONT_CACHE_IF_LENGTH_UNKNOWN), and this ultimately is the problem I think.

It looks like that CacheDataSource.open() is called from StatsDataSource.open(), which in turn was called from ProgressiveMediaPeriod$ExtractingLoadable. The dataSpec is instanced there by buildDataSpec(), and it looks like it defaults to setting that flag:

    private DataSpec buildDataSpec(long position) {
      // Disable caching if the content length cannot be resolved, since this is indicative of a
      // progressive live stream.
      return new DataSpec.Builder()
          .setUri(uri)
          .setPosition(position)
          .setKey(customCacheKey)
          .setFlags(
              DataSpec.FLAG_DONT_CACHE_IF_LENGTH_UNKNOWN | DataSpec.FLAG_ALLOW_CACHE_FRAGMENTATION)
          .setHttpRequestHeaders(ICY_METADATA_HEADERS)
          .build();
    }

So it seems my problem is that I'm using a ProgressiveMediaSource.Factory to wrap my cache data source?

What is the suggested approach for needing to cache media from the Internet where the length is not known? Is this simply not a supported use case for caching at all? If so, then what I'd like to see is a way to allow this, and if the cache fills up and the evictor can't succeed, the cache simply stops at that point and the stream is used instead.

Thanks

ojw28 commented 3 years ago

What type of content are you streaming? If these are just regular media files, then it's unusual for the server to not be including a Content-Length response header, which is what would normally prevent the length from being resolved. Note that this omission would also prevent client features such as calculating the percentage of a large file that's been downloaded (unless the file size were provided in some other way).

One other possibility is that the server is including a Content-Length header, but is also using gzip during transport, which would also prevent the length from being resolved. We actively disable this by telling the server that we only support identity encoding when using DefaultHttpDataSource, which you're using according to your StackOverflow post, so I doubt this is the problem.

The reason we don't cache progressive streams if the length isn't resolved, is that this nearly always implies that the stream is an unseekable live stream, which isn't something you should cache.

It sounds to me like whatever DropBox servers you're using, they're for some reason designed to not support this use case properly. If you want to try and cache anyway, what you could do is add another very simple DataSource into your chain of DataSources, which would just pass all of its methods through to the CacheDataSource, except that in open it would clear the FLAG_DONT_CACHE_IF_LENGTH_UNKNOWN flag. Something like (untested):

public long open(DataSpec dataSpec) throws IOException {
    return cacheDataSource.open(
        dataSpec
            .buildUpon()
            .setFlags(dataSpec.flags & ~DataSpec.FLAG_DONT_CACHE_IF_LENGTH_UNKNOWN)
            .build());
}

If it's easier, you could test by just editing ProgressiveMediaPeriod$ExtractingLoadable directly to remove the flag.

If being able to remove the flag is helpful, we could consider adding it as an option on ProgressiveMediaSource.Factory for convenience.

tfrysinger commented 3 years ago

Oliver - 

I'm streaming a standard mp4 file.

I think the issue may be that I'm using a custom Storage Access Framework DocumentsProvider to handle the content Uri being given to Exoplayer. This SAF provider uses a Dropbox token with the Dropbox APIs to stream content back when the SAF API openDocument() gets called within Android's content provider/resolver framework. Here is the relevant code from that openDocument() API within the DocumentsProvider:

            DbxClientV2 mDbxClient;
            FileMetadata fileMetadata;
            try {
                mDbxClient = DropboxClientFactory.getClient();
                fileMetadata = (FileMetadata) mDbxClient.files().getMetadata(documentId);
            } catch (DbxException e) {
                Timber.d("Got IDbxException when getting file metadata for document with ID: %s, message was: %s", documentId, e.getMessage());
                return null;
            }
            if ( InTouchUtils.isVideoFile(fileMetadata.getName()) ) {

                // Return a descriptor that will stream the file
                //Timber.d("Streaming from video source");
                ParcelFileDescriptor[] pipe;
                try {
                    pipe = ParcelFileDescriptor.createPipe();

                    // Get input stream for the pipe
                    DbxDownloader downloader = mDbxClient.files().download(fileMetadata.getPathLower(), fileMetadata.getRev());
                    new TransferThread(downloader.getInputStream(), new ParcelFileDescriptor.AutoCloseOutputStream(pipe[1]), signal, fileMetadata.getSize()).start();

                    return pipe[0];
                } catch (DbxException dbe) {
                    Timber.d("Got IDbxException when streaming content: %s", dbe.getMessage());
                } catch (IOException ioe) {
                    Timber.d("Got IOException when streaming content: %s", ioe.getMessage());
                } catch (Exception e) {
                    Timber.d("Got Exception when streaming content: %s", e.getMessage());
                }
                return null;

So the descriptor being returned is one that streams it - I'm not even sure if or where any http headers would get involved?

I do think my suggestions for allowing an optional way to override the DataSpec setting would be good - again, assuming that if the streaming file filled up the cache that Exoplayer would do the right thing and switch to just streaming at that point.

ojw28 commented 3 years ago

Oh right. I found https://github.com/google/ExoPlayer/issues/4551#, and it seems like this is the same problem, and that my response there still applies.

tfrysinger commented 3 years ago

Can you confirm that if I remove that flag and the cache fills up, that Exoplayer will switch to just streaming and won't cause a problem with the cache (other than it would be full)?

tfrysinger commented 3 years ago

I just made the changes in my InTouchCacheDataSourceFactory class that you suggested (at least - I think I did) and it worked well - is this how I should be doing it? Or is there a more streamlined way to "chain" DataSources?```

@Override
public DataSource createDataSource() {
    CacheDataSource dataSource = new CacheDataSource(InTouchUtils.getMediaCache(),
            defaultDatasourceFactory.createDataSource(),
            new FileDataSource(),
            new CacheDataSink(InTouchUtils.getMediaCache(), maxCacheFragmentSize),
            CacheDataSource.FLAG_BLOCK_ON_CACHE | CacheDataSource.FLAG_IGNORE_CACHE_ON_ERROR,
            listener);
    return new InTouchCacheDataSource(dataSource);
}

/**
 * Class specifically to turn off the flag that would not cache streamed docs.
 */
private class InTouchCacheDataSource implements DataSource {
    private CacheDataSource cacheDataSource;
    InTouchCacheDataSource(CacheDataSource cacheDataSource) {
        this.cacheDataSource = cacheDataSource;
    }

    @Override
    public void addTransferListener(TransferListener transferListener) {
        cacheDataSource.addTransferListener(transferListener);
    }

    @Override
    public long open(DataSpec dataSpec) throws IOException {
        return cacheDataSource.open(dataSpec
                .buildUpon()
                .setFlags(dataSpec.flags & ~DataSpec.FLAG_DONT_CACHE_IF_LENGTH_UNKNOWN)
                .build());
    }

    @Nullable
    @Override
    public Uri getUri() {
        return cacheDataSource.getUri();
    }

    @Override
    public void close() throws IOException {
        cacheDataSource.close();
    }

    @Override
    public int read(byte[] target, int offset, int length) throws IOException {
        return cacheDataSource.read(target, offset, length);
    }
}
ojw28 commented 3 years ago

ExoPlayer allows caching of unknown length requests in some other use cases, since the reasoning for disabling this in ProgressiveMediaPeriod is specific to ProgressiveMediaPeriod. So the caching components (e.g., CacheDataSource / SimpleCache) do support this, and you should find that it works fine if you simply remove the flag. The approach you've posted above looks like the right way to do this.

I still think that implementing a DataSource that wraps the DropBox API and provides ExoPlayer with what it wants would be a much better solution, but if the approach above solves your problem then that's probably all you need.

It seems we can close this. We haven't received any additional questions of requests for this since https://github.com/google/ExoPlayer/issues/4551 was filed 2.5 years ago, and the workaround for clearing the flag isn't too horrible, if that's really what you want to do :).

tfrysinger commented 3 years ago

Vis a vis 4551 - which comment (I totally forgot we previously had this conversation!) are you referring to? I notice you did say this:

"I don't really follow the logic here. You have the content:// URI from SAF, and you also have FileMetadata including the content length. There are options for exposing the content to ExoPlayer other than just directly passing the content:// URI. For example, you could implement your own DataSource that integrates directly with SAF (e.g. SafDataSource). You could use the ContentDataSource implementation as a starting point, and change it so that it can get the length from SAF FileMetadata."

Can you comment just a bit more on the idea of implementing a DataSource that integrates in with the SAF? I'm not sure I quite follow, but sounds like that is what I should be doing for my use case.

Thanks!

ojw28 commented 3 years ago

I assume that the DropBox API, one way or another, provides:

  1. A mechanism for reading data.
  2. The length of the data, which seems like it's available here.

Those are the two things that a DataSource implementation needs to do, so you just need to implement a DataSource that wraps the DropBox API. So, for example, you could:

  1. Invent a URI schema like dropbox://documentId
  2. In your DataSource.open, get the documentId out of the URI and use the DropBox API to query the metadata and get hold of an InputStream for reading the data. By querying the metadata, you will be able to return the correct value from open, which means the length will be resolved properly.
  3. In your DataSource.read, read from the InputStream.

As per my response on the previous issue, ContentDataSource should be a reasonably good starting point, if it's easier than starting from scratch. Two things to to pay particular attention to are:

  1. The DataSpec passed to open may be requesting data from a non-zero position, particularly after a seek. So make sure not to ignore DataSpec.position. If you do this, or if you use it incorrectly, you will see that playback fails when you seek (or possibly that playback will resume from the start, rather than the requested seek position).
  2. Read the @return Javadoc for open carefully, and make sure to return exactly what it says. If the request is unbounded and DataSpec.position is non-zero, you should return the number of bytes in the file from that position, not the full length of the file.

Implementations like ContentDataSource are again a good guide for getting these details right.

tfrysinger commented 3 years ago

Appreciate the input! I will give that a shot and see how it goes...