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 setting AdsLoader (and AdViewProvider) dynamically/on-demand. #6159

Open
eneim opened this issue Jul 10, 2019 · 3 comments
Open

Allow setting AdsLoader (and AdViewProvider) dynamically/on-demand. #6159

eneim opened this issue Jul 10, 2019 · 3 comments
Assignees

Comments

@eneim
Copy link
Contributor

eneim commented Jul 10, 2019

[REQUIRED] Use case description

The scenario is: I have a MediaSource that can be played in either list/background/full-screen and I would like to have ads content only in full-screen playback. The setup also require non-blocking playback, i.e I don't want to re-prepare the MediaSource at all.

Current implementation requires AdsLoader and AdViewProvider when constructing AdsMediaSource. If I use PlayerView as the AdViewProvider, the background playback may leak this instance (as the foreground service keeps playing after the Activity is destroyed). Acknowledge that I can have a custom implementation of AdViewProvider in a way that can avoid leaking, doing so is not trivial.

On the other hand, what AdsMediaSource is doing with AdsLoader are: (1) starting it (i.e kick-off the logic that involve ads content to be taken into account when playing the main content), (2) stop it, (3) recover on error. I think instead of having AdsLoader/AdViewProvider as fixed final fields, having a AdsMediaSource.setAdsStuff(nullable AdsLoader, nullable AdViewProvider) may give this class more flexibility.

Proposed solution

  • Making AdsLoader/AdViewProvider non-final.
  • Introducing setter for these 2 fields (in one or 2 methods).
  • Pre-condition: if AdsLoader is notnull then AdViewProvider must be notnull.
  • When client call setter for AdsLoader/AdViewProvider, AdsMediaSource may call AdsLoader.start() if it is already prepared, or call AdsLoader.start() when it is being prepared.
  • Other methods (stop/handle preparation error) can be kept unchanged.
  • Setting AdsLoader to null = disabling ads playback. AdsMediaSource would call AdsLoader.stop() and do other cleanup for ads content.

Alternatives considered

I plan to implement a draft following my proposal above, but if you guys think it is a reasonable request, I'm happy to wait.

@ojw28
Copy link
Contributor

ojw28 commented Jul 10, 2019

I think there are probably two things to address here:

  1. Being able to replace the views that ImaAdsLoader is using (e.g. because playback switches from one PlayerView to another). It looks like this might be possible by calling stop() and then start() on the ImaAdsLoader instance, but I'm not sure if there are any side effects of doing that.
  2. Being able to change the AdsLoader (either by replacing it, or by changing its state internally) based on arbitrary events that the application wants to affect if and when ads are played. I think this might already be possible for custom AdsLoader implementations.

@andrewlewis can probably clarify and add more detail.

@eneim
Copy link
Contributor Author

eneim commented Jul 31, 2019

Hi @andrewlewis , do you have any comment for this issue, or is there any development happening already? Thanks in advance.

@andrewlewis
Copy link
Collaborator

In the current implementation AdsMediaSource expects to know the positions of ads when it exposes its timeline, but if the AdsLoader can be set/modified/cleared during playback it seems like we'd need a way to add ad groups during playback. Also, with IMA the ad display container is used in the ads request, and I expect changing it requires a new ads request, which would clear any state (e.g., currently playing ad).

One workaround would be to implement a custom AdsLoader, and ignore AdViewProvider completely. You can pass the view you want to use directly to your custom loader. Because the positions of ad breaks can't change dynamically you could try including ad breaks everywhere you might want them, then set these to be skipped just before they play if you know you're in the background.

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

3 participants