Report non-fatal errors through callbacks and not just as logcat warning

[fguinet]

Hi,

in case of an Audio discontinuity detected (cf. DefaultAudioSink line 644 : Log.e(TAG, "Discontinuity detected [expected ...) nothing is raised to the AnalyticsListener (such as for onPositionDiscontinuity).
Is it a bug, a new feature to be explored or simply as designed ?
I would be very happy to have this on a coming version to better track the audio discontinuity of some live streams.

Best regards,

Fabien

[ojw28]

@tonihei - Seems worth propagating to AudioRendererEventListener and AnalyticsListener?

[tonihei]

As far as I can see there are 3 reasons why such a discontinuity in DefaultAudioSink can happen:

1. A regular, expected discontinuity because something changed in the track selection, the media queue, or the media period reports a discontinuity in the source. This gets all handled in ExoPlayerImplInternal and gets reported as onPositionDiscontinuity. The DefaultAudioSink ends up with the START_NEED_SYNC flag because the audio renderer position is reset and needs syncing.
2. Buffers in the audio renderer are skipped on purpose because they are marked as decode-only. Given that this is likely only happening after a seek or other expected discontinuities, there is no reason to report that again.
3. Unexpected discrepancies between the expected media timestamp and the actual media timestamp. That's the warning on L644 in DefaultAudioSink you mentioned. If I remember correctly, you only end up in there if you have malformed media. For all other media issues, we tend to just log a warning as long as we can continue playback as in this case. So I'm not sure we should report it.

@andrewlewis may know more details about the cases for which this particular warning can occur.

I would be very happy to have this on a coming version to better track the audio discontinuity of some live streams.

Can you explain in more detail what you are seeing and what you expect to see? Might be that you are actually looking for audible audio underruns, which are already reported via onAudioUnderrun.

[fguinet]

In think we are in the third case. We are playing a Live stream media and there is sometimes a PCR shift at the packager level. Indeed this is an issue from the packager we are using, but we would like to track that kind of QoS issue and report them when they occur (analytics). Does that make sense ? Le mer. 4 sept. 2019 à 18:00, tonihei <notifications@github.com> a écrit :

…

As far as I can see there are 3 reasons why such a discontinuity in DefaultAudioSink can happen: 1. A regular, expected discontinuity because something changed in the track selection, the media queue, or the media period reports a discontinuity in the source. This gets all handled in ExoPlayerImplInternal and gets reported as onPositionDiscontinuity. The DefaultAudioSink ends up with the START_NEED_SYNC flag because the audio renderer position is reset and needs syncing. 2. Buffers in the audio renderer are skipped on purpose because they are marked as decode-only. Given that this is likely only happening after a seek or other expected discontinuities, there is no reason to report that again. 3. Unexpected discrepancies between the expected media timestamp and the actual media timestamp. That's the warning on L644 in DefaultAudioSink you mentioned. If I remember correctly, you only end up in there if you have malformed media. For all other media issues, we tend to just log a warning as long as we can continue playback as in this case. So I'm not sure we should report it. @andrewlewis <https://github.com/andrewlewis> may know more details about the cases for which this particular warning can occur. I would be very happy to have this on a coming version to better track the audio discontinuity of some live streams. Can you explain in more detail what you are seeing and what you expect to see? Might be that you are actually looking for audible audio underruns, which are already reported via onAudioUnderrun. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#6384?email_source=notifications&email_token=AD3DKZOYJCFCZH6JVJE63STQH7LTLA5CNFSM4ITQ6GN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD54CMSA#issuecomment-527967816>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD3DKZKWH2KHNK5KT4RSPEDQH7LTLANCNFSM4ITQ6GNQ> .

[tonihei]

That makes complete sense.

I just wonder if that case is any different from many other non-fatal warnings we have across our code base. Whenever we log something with the warning level, it means we recognized a problem but the problem is most likely non-fatal and we can just continue playback. But in all cases, it would we be good to report this to analytics systems if possible to know that an unexpected situation occurred. So far, the only non-fatal errors reported through the listeners are load errors that can be retried. I think if we report this kind of non-fatal error, we should think about a way to report all of them and not just this particular one.

Marking as an enhancement again to track reporting generic non-fatal errors.

[tonihei]

We decided to report non-fatal errors with more targeted callbacks corresponding to the area in which they occurred (for example onAudioSinkError, onAudioCodecError or onVideoCodecError. So no need for the generic non-fatal callback anymore.

The initially requested reporting of the non-fatal but unexpected audio timestamp discontinuities will be reported as AudioSink.UnexpectedDiscontinuityException through onAudioSinkError in the future.