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 app to turn off printing errors to logcat #4665

Closed
noamtamim opened this issue Aug 15, 2018 · 6 comments
Closed

Allow app to turn off printing errors to logcat #4665

noamtamim opened this issue Aug 15, 2018 · 6 comments
Assignees

Comments

@noamtamim
Copy link
Contributor

Issue description

When there are playback errors, ExoPlayer sends them to logcat in addition to reporting via listeners:

    // Snippet from ExoPlayerImplInternal
    } catch (ExoPlaybackException e) {
      Log.e(TAG, "Playback error.", e);
      stopInternal(/* reset= */ false, /* acknowledgeStop= */ false);
      eventHandler.obtainMessage(MSG_ERROR, e).sendToTarget();
      maybeNotifyPlaybackInfoChanged();
    } catch (IOException e) {
      Log.e(TAG, "Source error.", e);
      stopInternal(/* reset= */ false, /* acknowledgeStop= */ false);
      eventHandler.obtainMessage(MSG_ERROR, ExoPlaybackException.createForSource(e)).sendToTarget();
      maybeNotifyPlaybackInfoChanged();
    } catch (RuntimeException e) {
      Log.e(TAG, "Internal runtime error.", e);
      stopInternal(/* reset= */ false, /* acknowledgeStop= */ false);
      eventHandler.obtainMessage(MSG_ERROR, ExoPlaybackException.createForUnexpected(e))
          .sendToTarget();
      maybeNotifyPlaybackInfoChanged();

I believe the player should not print to logcat. The app listens to onPlayerError() and it can decide to log these errors according to its own logic.

Version of ExoPlayer being used

2.8.3

@ojw28
Copy link
Contributor

ojw28 commented Aug 16, 2018

This is a valid point, but on the flip side it's helpful for us to have some logging enabled by default, so that when less advanced developers file "playback doesn't work" type issues, there's something in the logs to go on.

We could leave the logging on by default and add a way of disabling it. Is it really important or worth it though? Is the logging causing any non-trivial issues for you? Thanks!

@noamtamim
Copy link
Contributor Author

Upon startup, we run a test (see #3835) that always causes ExoPlayer to fail (the failure error is what matters for the purpose of the test). We don't want this to be printed to the logcat because it looks like something bad has happened.
Further, it seems that some (all?) of the crash-reporting services also report logcat errors. This causes a lot of false positives in the reports.

So yes, an ability to turn it off will be great. And I agree that leaving it on by default is probably a good thing.

@noamtamim
Copy link
Contributor Author

noamtamim commented Aug 22, 2018

Hi @ojw28, appreciate your response here.

@ojw28
Copy link
Contributor

ojw28 commented Aug 22, 2018

Ack. We'll probably need to redirect all Log.* calls through our own class to do this in a sensible way.

ojw28 pushed a commit that referenced this issue Sep 20, 2018
Currently there is no way to disable (or reduce) the logcat output generated
by ExoPlayer.

Issue:#4665

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=213421072
@noamtamim
Copy link
Contributor Author

@ojw28 I believe this can be closed with the release of 2.9.0, right?

@ojw28 ojw28 assigned tonihei and unassigned ojw28 Oct 1, 2018
@ojw28
Copy link
Contributor

ojw28 commented Oct 1, 2018

Believe so; thanks!

@ojw28 ojw28 closed this as completed Oct 1, 2018
@google google locked and limited conversation to collaborators Jan 31, 2019
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

3 participants