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

Playback without Surface causes dropped frames and delay in video playback when a Surface is added #2575

Closed
ghost opened this issue Mar 19, 2017 · 5 comments
Labels

Comments

@ghost
Copy link

ghost commented Mar 19, 2017

Issue description

When removing the surface from the SimpleExoPlayer and after some time re-adding it a lot of "droppedFrames" messages are triggered. This cause quite some delay before the video playback is continued (while sound is normally playing)

This is cause by a wrong translation of timestamps:
When the MediaCodecRenderer doesn't have a codec it calls skipToKeyframeBefore with positionUs. This will call skipToKeyframeBefore on the stream. The stream expects the periodPositionUs but the used position the rendererPositionUs. In simple cases this can be adjusted using RENDERER_TIMESTAMP_OFFSET_US, but in other cases the offset can be a more variable value and it is not possible to adjust it without MediaPeriodHolder (which is not accessible in the MediaCodecRenderer. Because the seek fails, the renderer will try to decode all the frames as soon as an output is available again (Surface). As the delay is too high (>30 ms) the frames will be dropped.

Reproduction steps

Instantiate a SimpleExoPlayer (ExoPlayerFactory.newInstance). Clear surface but let video playback continue. Re-add a surface.

Link to test content

http://html5demos.com/assets/dizzy.mp4 (it's easier with longer videos to get some time for a larger time gap -> frames that are dropped)

Version of ExoPlayer being used

r2.3.0

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

Tested on PixelXL (7.1.1) and OnePlus 2 (6.0.1)

A full bug report captured from the device

    D/EventLogger: videoDecoderInitialized [0.72, OMX.qcom.video.decoder.avc]
    D/EventLogger: videoFormatChanged [0.72, id=2, mimeType=video/avc, res=480x360]
    D/EventLogger: videoDecoderInitialized [15.12, OMX.qcom.video.decoder.avc]
    D/EventLogger: droppedFrames [15.42, 50]
    D/EventLogger: droppedFrames [15.61, 50]
    D/EventLogger: droppedFrames [15.84, 50]
    D/EventLogger: droppedFrames [16.08, 50]
    D/EventLogger: droppedFrames [16.30, 50]
    D/EventLogger: droppedFrames [16.52, 50]
    D/EventLogger: droppedFrames [16.76, 50]
    D/EventLogger: droppedFrames [16.99, 50]
    D/EventLogger: droppedFrames [26.28, 48]
@ojw28 ojw28 added the bug label Mar 19, 2017
@ojw28
Copy link
Contributor

ojw28 commented Mar 19, 2017

Thanks for reporting this. It looks like we should subtract streamOffsetUs in BaseRenderer.skipToKeyframeBefore, so that it becomes:

protected void skipToKeyframeBefore(long timeUs) {
  stream.skipToKeyframeBefore(timeUs - streamOffsetUs);
}

If you could confirm whether that fixes the issue, that would be great. Thanks!

@ghost
Copy link
Author

ghost commented Mar 19, 2017

hmm not that easy to do that as stramOffsetUs is private. But it should fix it as my current quick workaround is to extend the MediaCodecVideoRenderer and override skipToKeyframeBefore with (Kotlin code:)

    override fun skipToKeyframeBefore(timeUs: Long) {
        super.skipToKeyframeBefore(timeUs - RENDERER_TIMESTAMP_OFFSET_US)
    }

where RENDERER_TIMESTAMP_OFFSET_US is the default value of streamOffsetUs (60000000)

@ghost
Copy link
Author

ghost commented Mar 20, 2017

Another issue that I encountered in this szenario is that the video never end as long as no surface is set on the video renderer. This is because the isEnded of the video track renderer checks if the output is done, but as we have no surface, we have no codec and therefore the output is never rendered. Right now I have overwritten isEnded and check if I have a surface/ codec.

I have not tested if the above fix also fixes this issue, I will try that again later if you want. Also should I create a seperate issue for this?

@ojw28
Copy link
Contributor

ojw28 commented Mar 20, 2017

Yes, we're aware that's a problem. Feel free to file a separate issue for tracking purposes.

We're currently trying to figure out exactly how this kind of case should behave. Note that you can disable the renderer (rather than just not having a Surface attached to it) as one way to avoid this problem, although you may find that enabling it again is not seamless. We may change it to be seamless in the future and at that point require that the renderer be disabled, although this is still under discussion.

@ghost
Copy link
Author

ghost commented Mar 20, 2017

I created #2582 just to keep track of it :)

ojw28 added a commit that referenced this issue Mar 22, 2017
Issue: #2575

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=150622487
@ojw28 ojw28 closed this as completed Mar 22, 2017
ojw28 added a commit that referenced this issue Mar 23, 2017
@google google locked and limited conversation to collaborators Aug 3, 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

1 participant