Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 130bbc45 authored by Ian Baker's avatar Ian Baker
Browse files

Fix vorbis-specific workaround to only compare audio timestamps 

This workaround was introduced in 2014 [1]. At the time it was correctly
only comparing audio timestamps because mAnchorTimeMediaUs only held
audio timestamps then. In 2017 [2], mAnchorTimeMediaUs started to hold
video timestamps too - at which point this comparison became invalid and
a bug was introduced that can lead to control-flow exiting too early
when handling an audio buffer with the same timestamp as the preceding
video buffer. If this happens at the start of playback, then the
**second** audio buffer timestamp is passed to
MediaClock::setStartingMediaTime, which can lead to incorrect values
being returned from NuPlayerRenderer::getCurrentPosition (when paused
at the start of playback).

[1] https://cs.android.com/android/_/android/platform/frameworks/av/+/eecb7805bbbb712925d4372c505f8c7f5c4fb5ed
[2] https://cs.android.com/android/_/android/platform/frameworks/av/+/17648f365e5914fa772bbfebaf9f5c6a37ddfb99

Test: atest MediaPlayerTest
Bug: 283882552
Bug: 282472894
Bug: 263063118
Change-Id: I7fbe6bdaff27c93f15d9d85902306a3f6e00b978
parent de76fc18

media/libmediaplayerservice/nuplayer/NuPlayerRenderer.cpp

+4 −1
Original line number Diff line number Diff line
@@ -132,6 +132,7 @@ NuPlayer::Renderer::Renderer( 
      mMediaClock(mediaClock),

      mPlaybackSettings(AUDIO_PLAYBACK_RATE_DEFAULT),

      mAudioFirstAnchorTimeMediaUs(-1),

      mAudioAnchorTimeMediaUs(-1),

      mAnchorTimeMediaUs(-1),

      mAnchorNumFramesWritten(-1),

      mVideoLateByUs(0LL),
@@ -433,6 +434,7 @@ void NuPlayer::Renderer::setAudioFirstAnchorTimeIfNeeded_l(int64_t mediaUs) { 
// Called on renderer looper.

void NuPlayer::Renderer::clearAnchorTime() {

    mMediaClock->clearAnchor();

    mAudioAnchorTimeMediaUs = -1;

    mAnchorTimeMediaUs = -1;

    mAnchorNumFramesWritten = -1;

}
@@ -1261,7 +1263,7 @@ void NuPlayer::Renderer::onNewAudioMediaTime(int64_t mediaTimeUs) { 
    Mutex::Autolock autoLock(mLock);

    // TRICKY: vorbis decoder generates multiple frames with the same

    // timestamp, so only update on the first frame with a given timestamp

    if (mediaTimeUs == mAnchorTimeMediaUs) {

    if (mediaTimeUs == mAudioAnchorTimeMediaUs) {

        return;

    }

    setAudioFirstAnchorTimeIfNeeded_l(mediaTimeUs);
@@ -1299,6 +1301,7 @@ void NuPlayer::Renderer::onNewAudioMediaTime(int64_t mediaTimeUs) { 
        }

    }

    mAnchorNumFramesWritten = mNumFramesWritten;

    mAudioAnchorTimeMediaUs = mediaTimeUs;

    mAnchorTimeMediaUs = mediaTimeUs;

}

media/libmediaplayerservice/nuplayer/include/nuplayer/NuPlayerRenderer.h

+3 −0
Original line number Diff line number Diff line
@@ -177,6 +177,9 @@ private: 
    float mVideoFpsHint;



    int64_t mAudioFirstAnchorTimeMediaUs;

    // previous audio anchor timestamp, in media time base.

    int64_t mAudioAnchorTimeMediaUs;

    // previous anchor timestamp (audio or video), in media time base.

    int64_t mAnchorTimeMediaUs;

    int64_t mAnchorNumFramesWritten;

    int64_t mVideoLateByUs;