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

Allow specifying per-item request headers (for playback and downloads) #6166

Open
chanbbong opened this issue Jul 11, 2019 · 15 comments
Open
Assignees

Comments

@chanbbong
Copy link

I have different cookies for each video.
so each time new video is loaded, I set DownloadManager with new cookie.
defaultHttpDataSourceFactory.getDefaultRequestProperties().set("Cookie", cookieValue);
however, I can only download video for the first time.
I think DownloadService seems to call DownloadManager only when DownloadService is started for the first time.
How do I download video with each different cookie?

@ojw28
Copy link
Contributor

ojw28 commented Dec 18, 2019

I don't think we currently support this (you can of course implement your own download functionality, rather than relying on the out-of-the-box components that we provide). Marking this as an enhancement.

@ojw28
Copy link
Contributor

ojw28 commented Dec 18, 2019

@marcbaechinger - We should consider whether we want to allow request headers to be specified in MediaItem, and (eventually) whether DownloadRequest should include a MediaItem rather than some of its current fields.

@ojw28 ojw28 changed the title How do I download video with each different cookie? Allow specifying request headers for downloads (e.g. in DownloadRequest) Dec 18, 2019
@msesma
Copy link

msesma commented Apr 17, 2020

Same problem here. Even if I don't allow the user to download two videos al the same time, the DownloadService only request DownloadManager once, so the second video will use the DownloadManger of the first one and fail because of wrong cookie.

@BarryFruitman
Copy link

+1 for specifying headers in a DownloadRequest or MediaItem. Headers are used pretty commonly in authentication schemas and it would be useful to be able to specify them on a per-request basis.

@NathanSass
Copy link

+1 I would also like this feature

@kschults
Copy link

Hi @ojw28, I'm about to start looking into contributing a fix for this, but wanted to check in first with a couple of questions.

  1. Are you already working on this? Or do you already have thoughts about what will be involved in fixing it?

  2. Is there anything I need to know about developing on this project, aside from what's in CONTRIBUTING.md?

@ojw28
Copy link
Contributor

ojw28 commented Aug 11, 2020

Are you already working on this?

No

Do you already have thoughts about what will be involved in fixing it?

Although not directly what you're trying to achieve, I think a good first step would be to add requestHeaders to MediaItem.PlaybackProperties, and use them in the various MediaSource implementations for playback. It doesn't really make logical sense to support something for downloads and not for playback, so I think this should be done first even if you don't need it yourself.

Then for downloads I think you'll need to:

  1. Add a requestHeaders field to DownloadRequest
  2. Upgrade the database logic in DefaultDownloadIndex to support persisting the headers
  3. Use the headers in the various Downloader implementations

Note that I haven't actually tried any of the above, so I can't really guarantee it's correct or accurate. It's a best guess as to what would be needed.

Is there anything I need to know about developing on this project, aside from what's in CONTRIBUTING.md?

Small changes are preferred to larger ones (i.e., the playback and download pieces suggested above should definitely be separate pull requests, rather than one big one). Other stuff that's probably obvious:

  • Try and adhere to the style of existing code
  • If you're working on something larger where there might be multiple possible solutions, it's best to ask questions about the design early

@kschults
Copy link

Thanks for the info.

For future questions, is there a better way to ask, or is leaving them here the best place?

@ojw28
Copy link
Contributor

ojw28 commented Aug 11, 2020

Here is fine; thanks!

@google google deleted a comment from Mavrick102 Aug 11, 2020
@google google deleted a comment from Mavrick102 Aug 11, 2020
@kschults
Copy link

kschults commented Sep 3, 2020

@ojw28 Perhaps there's another way to do this. I had initially tried to solve this problem in my client by using HttpDataSourceFactory with defaultRequestProperties set (as did @chanbbong), but it seemed like the broken link in that chain is that DefaultDownloaderFactory uses the same CacheDataSourceFactory for every download request.

I'm having trouble tracking how the DataSourceFactory that gets passed to a DownloadHelper ends up in the DownloadManager, but it seems like it could be more straightforward to allow the DefaultDownloaderFactory to use different sources?

I was able to hack a solution into place on the client side by extending DefaultDownloaderFactory and rewriting the downloadConstructorHelper via reflection during createDownloader. My thinking here is that there may be a way to formalize this into something that isn't an awful hack.

@RemiHin
Copy link

RemiHin commented Jul 23, 2021

Is there any update on this? I'm running into the problem that I can't download files that require an authentication header.

@Mkurbanov
Copy link

Mkurbanov commented Sep 20, 2021

it's working. just add DefaultHttpDataSource.Factory to DownloadManager. like this code:

            // Create a factory for reading the data from the network.
            Executor downloadExecutor = Runnable::run;
            DefaultHttpDataSource.Factory defaultHttpDataSourceFac = new DefaultHttpDataSource.Factory();
            HashMap<String, String> hashMap = new HashMap<>();
            hashMap.put("header_param", "header_value");
            defaultHttpDataSourceFac.setDefaultRequestProperties(hashMap);
            // Create the download manager.
            downloadManager = new DownloadManager(
                    context,
                    VideoCache.getDatabaseProvider(context),
                    VideoCache.getCache(context),
                    defaultHttpDataSourceFac,
                    downloadExecutor);

@kschults
Copy link

@Mkurbanov That solution is fine if you only need a single header that's the same for all downloads. If you need a different header per-download, this won't work.

@ojw28 ojw28 changed the title Allow specifying request headers for downloads (e.g. in DownloadRequest) Allow specifying per-MediaItem headers (MediaItem for playback, DownloadRequest for downloads) Apr 19, 2022
@ojw28 ojw28 changed the title Allow specifying per-MediaItem headers (MediaItem for playback, DownloadRequest for downloads) Allow specifying per-item request headers (MediaItem for playback, DownloadRequest for downloads) Apr 19, 2022
@ojw28 ojw28 changed the title Allow specifying per-item request headers (MediaItem for playback, DownloadRequest for downloads) Allow specifying per-item request headers (for playback and downloads) Apr 19, 2022
@ojw28
Copy link
Contributor

ojw28 commented Apr 19, 2022

I've generalized the title to cover the playback case as well as the download case. Note that there is a pending pull request that looks to add this (#7852), although I was concerned at the time that:

  1. For adaptive content, it's unclear to which requests headers should be attached during the playback.
  2. It doesn't handle the need to refresh header content (e.g., authorization tokens that may have expired).

@kschults
Copy link

Thanks for the update @ojw28. I'm going to be offline for the next six+ months, so I won't be able to move forward on #7852 until at least late fall, if someone wants to pick it up from me. (Will comment over there as well)

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

No branches or pull requests

9 participants