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

Add method to reset ad group to be played again. #9615

Closed
chladto1 opened this issue Oct 27, 2021 · 17 comments
Closed

Add method to reset ad group to be played again. #9615

chladto1 opened this issue Oct 27, 2021 · 17 comments
Assignees

Comments

@chladto1
Copy link

We are updating ExoPlayer in our project from version 2.13.2 to 2.15.1. We are using modified IMA extension because of feature described in issue 8810. We had to change adPlaybackState.adGroups[i] = AdPlaybackState.AdGroup().withAdCount(1).withAdState(AdPlaybackState.AD_STATE_PLAYED, 0) to adPlaybackState = adPlaybackState.withNewAdGroup(i, timeUs).withAdCount(i, 1).withPlayedAd(i, 0) and adPlaybackState.adGroups[i] = AdPlaybackState.AdGroup() to adPlaybackState = adPlaybackState.withNewAdGroup(i, adTimeUs). But it looks like there are some internal changes and our application is crashing in newly added check on line 285 in AdsMediaSource that was not in 2.13.2 version.

Could you please help us how to set AdGroup directly into AdPlaybackState.adGroups and avoid increasing the AdPlaybackState.adGroupCount? Our code is in Kotlin language and we created extension AdPlaybackState.setAdGroup(index: Int, adGroup: AdPlaybackState.AdGroup) that is using reflection to set an AdGroup into AdPlaybackState.adGroups. But using reflection is not the best solution. Thanks!

@tonihei
Copy link
Collaborator

tonihei commented Nov 15, 2021

I'm not aware of any removed methods that no longer work, so I think whatever you are planning to do in the old code should still work the same in the new code. A general note in that regard: It's unclear what you are trying to achieve exactly and usually we don't expect apps to update AdPlaybackState themselves unless they have a customized AdsLoder.

void increasing the AdPlaybackState.adGroupCount

Using withNewAdGroup adds a new ad group to the list, so this is by design.

@chladto1
Copy link
Author

There is no ability to set AdGroup at specific index in the new version. In 2.13.2 we can call adPlaybackState.adGroups[i] = *. In 2.15.1 we must call adPlaybackState.withNewAdGroup(i, timeUs).* that increase property adGroupCount and causes a crash on newly added line 285 in AdsMediaSource. Could you please just add a method to set AdGroup at index?

@tonihei
Copy link
Collaborator

tonihei commented Nov 18, 2021

AdPlaybackState is supposed to be an immutable class and changing the existing array members was never a valid usage. You should be able to modify an existing ad with the various helper methods in AdPlaybackState, for example withAdCount, withAdUri, withPlayedAd etc. If there is something specific you need to do that isn't possible via these methods, please describe your use case in more detail and we might be able to add additional helper methods.

@chladto1
Copy link
Author

Use case is described in issue 8810 and I shared you our code. I can share you our current code that we are using in 2.15.1. Is that what would help?

@tonihei
Copy link
Collaborator

tonihei commented Nov 18, 2021

Issue #8810 is about marking an ad as skipped, which can be achieved with AdPlaybackState.withSkippedAd.

@chladto1
Copy link
Author

Yes, we are doing that, but to mark them available, we were call adPlaybackState.adGroups[i] = AdPlaybackState.AdGroup() which is not possible in the new version. Call adPlaybackState = adPlaybackState.withNewAdGroup(i, adTimeUs) causes crash on line 285 in AdsMediaSource.

@tonihei
Copy link
Collaborator

tonihei commented Nov 18, 2021

I'm not sure I understand which part is missing or not working.

  • withNewAdGroup adds a new group, and this is not what you want to do if I understand correctly. Instead you want to change an existing ad group.

  • Changing an existing ad group can be done by calling AdPlaybackState.withAdCount/withPlayedAd/withSkippedAd etc, which you are using already per your last comment.

  • but to mark them available, we were call adPlaybackState.adGroups[i] = AdPlaybackState.AdGroup()

    To mark an ad as available, you need to provide an URI via withAdUri.

@chladto1
Copy link
Author

URI of an ad is not available at the moment. We need to set AdPlaybackState.AdGroup() at index witch probably corresponds to withNewAdGroup. There is no any migration guide, so I am just guessing.

@tonihei
Copy link
Collaborator

tonihei commented Nov 18, 2021

If the URI of the ad is not yet available, then you can't mark the ad as available because it would violate state assumption we have about AdGroup. Assigning an empty AdGroup after it has previously been non-empty is also not supported because the player assumes the number of ads can only grow, not shrink.

Other modifications are allowed: You can add new ads in the group, you can set the ad durations, you can set the state to played/skipped/error, or provide an URI to set the state back to available.
So I still don't see which method is missing exactly - which state of the existing ad group do you need to modify that can't be modified at the moment?

@chladto1
Copy link
Author

If the URI of the ad is not yet available, then you can't mark the ad as available because it would violate state assumption we have about AdGroup.

URIs of all AdGroups are not available until they are loaded. We want to make exactly that state. AdGroup is available to load. We mark AdGroup as skipped and later we mark AdGroup as "ready to load".

Assigning an empty AdGroup after it has previously been non-empty is also not supported because the player assumes the number of ads can only grow, not shrink.

This was available in previous versions of ExoPlayer with standard API. There was not needed reflection until version 2.15.1.

@tonihei
Copy link
Collaborator

tonihei commented Nov 19, 2021

URIs of all AdGroups are not available until they are loaded. We want to make exactly that state. AdGroup is available to load. We mark AdGroup as skipped and later we mark AdGroup as "ready to load".

There is no "ready to load" state I think because you'd need to either reset it to UNAVAILABLE or AVAILABLE, depending on whether the ad URI has already been set.

So to clarify, you need a method to reset the final ad states (SKIPPED, ERROR, PLAYED) back to (UNAVAILABLE or AVAILABLE), allowing the player to pick up the ad again for playback the next time it plays at this position. The method would look something like AdPlaybackState.withResetAdGroup(int adGroupIndex). Would that help to solve your problem?

This was available in previous versions of ExoPlayer with standard API.

That was never a valid API we intended to be used. It's more of an oversight that let you modify array elements that should have unmodifiable.

@chladto1
Copy link
Author

Yes, method like AdPlaybackState.withResetAdGroup(int adGroupIndex) that set everything to the same state as constructor would be great!

@tonihei
Copy link
Collaborator

tonihei commented Nov 19, 2021

that set everything to the same state as constructor

It wouldn't :). The method would keep ad count, ad URIs, the ad group time and known ad durations. But if I understand the requirements correctly, this is also what you need to reset the skipped state.

@tonihei tonihei changed the title Crash in onAdPlaybackState Add method to reset ad group to be played again. Nov 19, 2021
@chladto1
Copy link
Author

If I remember correctly, there was some Exo/IMA code that requires exactly that state. Maybe empty states or uris array or count as C.LENGTH_UNSET. For that reason, we are setting AdPlaybackState.AdGroup(adTimeUs) at index. Other solutions causes an errors. I can send you our actual code and you can try it. Maybe you will find a better solution :)

@tonihei
Copy link
Collaborator

tonihei commented Nov 19, 2021

I tested setting all ads to SKIPPED initially (to prevent playing them although they are loaded) and then after a while I reset just their state back to AVAILABLE (keeping all other values). After this point they started playing again as expected, so I think such a method would solve the general problem of resetting ads.

Depending on your customization of the ads loader, it could be that you need to update the states in more places or make sure IMA receives the expected positions etc., but that's down to your app customization and we can't provide detailed advise on how to customize the components correctly. The enhancement tracked by #8810 is also still not solved, so you might see short rebufferings when you change the ad state while the previous content piece is already playing.

@chladto1
Copy link
Author

After this point they started playing again as expected, so I think such a method would solve the general problem of resetting ads.

Glad to hear it, thanks.

you need to update the states in more places or make sure IMA receives the expected positions etc

Yes, I know. We are keeping track with the original IMA extension, but our version has a lot of modifications to "solve" save instance state, our smart seek feature, delayed responses, UI customizations, ...

tonihei added a commit that referenced this issue Nov 19, 2021
The player will not play ads in final states (played, skipped, error)
again. To allow ads loader customizations to play ads again, we can
add a method that resets the state back to available or unavailable
(depending on whether we have the URI for the ad).

Issue: #9615
PiperOrigin-RevId: 411042842
@tonihei
Copy link
Collaborator

tonihei commented Nov 19, 2021

Added the method in the commit above. Closing this issue accordingly.

@tonihei tonihei closed this as completed Nov 19, 2021
icbaker pushed a commit to androidx/media that referenced this issue Nov 19, 2021
The player will not play ads in final states (played, skipped, error)
again. To allow ads loader customizations to play ads again, we can
add a method that resets the state back to available or unavailable
(depending on whether we have the URI for the ad).

Issue: google/ExoPlayer#9615
PiperOrigin-RevId: 411042842
@google google locked and limited conversation to collaborators Jan 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants