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

MediaCodecVideoRenderer without Surface never ends #2582

Closed
ghost opened this issue Mar 20, 2017 · 6 comments
Closed

MediaCodecVideoRenderer without Surface never ends #2582

ghost opened this issue Mar 20, 2017 · 6 comments
Labels

Comments

@ghost
Copy link

ghost commented Mar 20, 2017

Issue description

When instantiating a MediaCodecVideoRenderer and never call setSurface (or set it to null) the render will never return true for isEnded. This will cause the player to play the video forever (after the audio track finishes the player will switch to the stand alone clock and keeps playing)

Reproduction steps

Create MediaCodecVideoRenderer and a MediaCodecAudioRenderer use them in a ExoPlayer and never call setSurface

Link to test content

http://html5demos.com/assets/dizzy.mp4

Version of ExoPlayer being used

v2.3.0

Device(s) and version(s) of Android being used

Nexus 5X emulator, Pixel XL, OnePlus 2

A full bug report captured from the device

N/A

@rustyshelf
Copy link

We have this same issue, I thought I'd comment because for us this happens when the user leaves our app, we remove the video from the surface it was playing on but we still keep playing in the background. Is there a recommended way to handle switching from rendering the video to just playing the audio seamlessly @ojw28?

@ghost
Copy link
Author

ghost commented Mar 27, 2017

@ojw28 mentioned one workaround in the issue referenced above (#2575), which is that you can enable or disable the renderer

We implemented a solution where we extend the MediaCodecVideoRenderer and overwrite handleMessage to check for C.MSG_SET_SURFACE. When that messages is received we updated an internal enabled state which we use in isEnded

public boolean isEnded() {
  return !enabled || super.isEnded();
}

That solution is rather hacky and we have not tested all edge cases, but it works for our video files.

@ojw28
Copy link
Contributor

ojw28 commented Mar 27, 2017

If you have demuxed audio and video then disabling the renderer is actually the right thing to do. We'll stop requesting the video stream in that case, so there will be a big bandwidth saving. We'll seamlessly rejoin the stream when the renderer is enabled again.

If your audio and video are demuxed then you can do the above, but the rejoin wont be seamless. The right thing for seamless is to detach the surface as you are doing, and for us to fix this bug.

@ojw28
Copy link
Contributor

ojw28 commented Mar 29, 2017

There are actually a bunch of things slightly wrong with surface attach/detach whilst the renderer is enabled. We'll use this issue to track these problems in general.

ojw28 added a commit that referenced this issue Mar 31, 2017
The only case where we should pass false is if we want to know
whether the passed time is within the buffered media. This is
relevant within seekTo implementations only.

This is related to (but does not fix) issue #2582

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=151315676
ojw28 added a commit that referenced this issue Mar 31, 2017
This change also ensures that format changes are read whilst the
renderer is enabled but without a codec. This is necessary to
ensure the drm session is updated (or replaced).

Updating the format is also needed so that the up-to-date format is
used in the case that the codec is initialized later due to the
surface being set. Previously, if an ABR change occurred between
the format being read and the surface being attached, we would
instantiate the codec and then immediately have to reconfigure it
when as a result of reading the up-to-date format. For a non-adaptive
codec this resulted in the codec being immediately released and
instantiated again!

Issue: #2582

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=151608096
ojw28 added a commit that referenced this issue Apr 11, 2017
@ojw28
Copy link
Contributor

ojw28 commented Apr 25, 2017

We still have some further changes to come around making surface switches more seamless, but the initial issue as reported here should now be fixed.

@ojw28 ojw28 closed this as completed Apr 25, 2017
@rustyshelf
Copy link

Thanks so much, we've been waiting for this and another issue to be fixed and both have. Wow! Of course we're demanding users so now we want to know when it will appear in a release ;) (sorry)

@google google locked and limited conversation to collaborators Aug 24, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants