Include timestamp of frame onRenderedFirstFrame #2986
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I signed it! |
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
|
I signed it! Again! |
|
Or maybe I didn't. Hmm. I signed the CLA with both possible email addresses. |
botondbutuza
changed the title
| @@ -285,8 +285,8 @@ public void onVideoSizeChanged(int width, int height, int unappliedRotationDegre | |||
| } | |||
|
|
|||
| @Override | |||
| public void onRenderedFirstFrame(Surface surface) { | |||
| Log.d(TAG, "renderedFirstFrame [" + surface + "]"); | |||
| public void onRenderedFirstFrame(Surface surface, long framePosition) { | |||
There was a problem hiding this comment.
Can we call it framePositionMs?
| @@ -378,7 +378,7 @@ private void setSurface(Surface surface) throws ExoPlaybackException { | |||
| // The surface is set and unchanged. If we know the video size and/or have already rendered to | |||
| // the surface, report these again immediately. | |||
| maybeRenotifyVideoSizeChanged(); | |||
| maybeRenotifyRenderedFirstFrame(); | |||
| maybeRenotifyRenderedFirstFrame(0); | |||
There was a problem hiding this comment.
You should probably pass the timestamp of the last frame that was rendered here, since that's the frame that's currently displayed on the surface.
| @@ -438,7 +438,7 @@ protected void onInputFormatChanged(Format newFormat) throws ExoPlaybackExceptio | |||
| @Override | |||
| protected void onQueueInputBuffer(DecoderInputBuffer buffer) { | |||
| if (Util.SDK_INT < 23 && tunneling) { | |||
| maybeNotifyRenderedFirstFrame(); | |||
| maybeNotifyRenderedFirstFrame(0); | |||
There was a problem hiding this comment.
This needs to be correct also. I think you probably need buffer.timeUs - streamOffsetUs, where streamOffsetUs is somehow obtained from BaseRenderer. Would need to check this though.
| @@ -679,16 +679,16 @@ private void clearRenderedFirstFrame() { | |||
| } | |||
| } | |||
|
|
|||
| /* package */ void maybeNotifyRenderedFirstFrame() { | |||
| /* package */ void maybeNotifyRenderedFirstFrame(long framePosition) { | |||
There was a problem hiding this comment.
It's probably going to less error prone if this method and the one below receive framePositionUs and divide by 1000 internally (rather than having the callers need to divide).
| */ | ||
| void onRenderedFirstFrame(Surface surface); | ||
| void onRenderedFirstFrame(Surface surface, long framePosition); |
There was a problem hiding this comment.
Please put Ms explicitly on the end of the variable name, for clarity. Ditto in a few places.
| @@ -314,7 +314,7 @@ private void renderBuffer() { | |||
| outputBuffer = null; | |||
| consecutiveDroppedFrameCount = 0; | |||
| decoderCounters.renderedOutputBufferCount++; | |||
| maybeNotifyRenderedFirstFrame(); | |||
| maybeNotifyRenderedFirstFrame(0); | |||
There was a problem hiding this comment.
Comments in MediaCodecVideoRenderer apply to this file too (but I haven't duplicated them all :)).
|
Closing due to inactivity. If you're still interested in getting this submitted, feel free to open a new pull request that addresses the pending comments. Thanks! |
As per #2533, the presentation timestamp of the first frame is passed on to onRenderedFirstFrame. This is helpful because the first available keyframe is rendered instantly, and is then followed by a visible delay before playback catches up with rendered frame.