-
Notifications
You must be signed in to change notification settings - Fork 468
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
IMA Extension fails to report post-prepared errors to the VideoAdPlayer.VideoAdPlayerCallback.onError()
#1334
Comments
Thanks for reporting, I can reproduce the issue. Will submit a fix that postpones the listener deregistration in |
When the AdTagLoader is deactivated because of a player error, the error callback is already pending on the app's main thread, but not yet executed. This means the VideoAdPlayerCallback instances registered in AdTagLoader won't receive this error event if the Player.Listener is immediately removed from AdTagLoader. This can be fixed by postponing the deregistration until after already pending messages have been handled. As this means other callbacks can be triggered now with player==null, this check needs to be added to other callbacks to avoid handling stale events. Issue: #1334 PiperOrigin-RevId: 630068222
Yes, definitely the best solution is in the As I understand the contract,
The issue occurs because an playback error:
It looked to me like having two As a quick and dirty fix (our situation does not have multiple private void maybeUpdateCurrentAdTagLoader() {
@Nullable AdTagLoader oldAdTagLoader = currentAdTagLoader;
@Nullable AdTagLoader newAdTagLoader = getCurrentAdTagLoader();
if (!Util.areEqual(oldAdTagLoader, newAdTagLoader)) {
if (oldAdTagLoader != null && newAdTagLoader != null) {
oldAdTagLoader.deactivate();
}
... I'm sure the correct fix is more complicated than this, but it does lead to exactly the correct sequence of events:
Thanks so much for taking this on, I'm happy to test any change you propose. |
I just saw your commit, after I wrote the long comment ¯_(ツ)_/¯. Will cherry-pick and test the fix |
Fix works perfectly, very clever to just defer the unregister. Thanks @tonihei BTW, the playback issue was with a particular vp8/Vorbis content that failed on an SEI/AmLogic platform that is likely missing some patches from AmLogic to the VP8 decoder (Google Chromecast plays the same content just fine). Again thanks, closing the bug. |
Version
Media3 main branch
More version details
No response
Devices that reproduce the issue
Any device
Devices that do not reproduce the issue
none
Reproducible in the demo app?
Yes
Reproduction steps
I'll need to work on a sample for you and post it, probably an playlist with segments that fail deep enough into playback to miss prepare (or just chunkless prepare will succeed)
Expected result
Error is reported to IMA so the ERROR Pixel pings and code watching the call backs gets a valid IMA event.
Actual result
Error is invisible to the IMA API, last event reported is "Ad Progress". Logging looks like this:
At this point the
resetInternal()
call has posted a call toAdsLoader.stop()
so IMA Extension removes the player listener before it gets a chance to get the playback error callback, which comes later:So this just looks like a pause to the AdsTagLoader:
I've got some ideas how to fix this, but there are so many ways I'd like to get input from the team first.
Media
I'll try and set something up.. and send it to android-media..
Bug Report
adb bugreport
to android-media-github@google.com after filing this issue.The text was updated successfully, but these errors were encountered: