Can someone describe me what MediaPlayerPrivateGStreamer::m_isPaused represents? It may seem like an innocent or obvious question, but after investigating the code, that's far from true. First, what is NOT represented by m_isPaused? 1) The value returned by MediaPlayerPrivateGStreamer::paused(). That function at no point reads it, in fact. 2) Whether playback is paused. Play a non-live video without MSE, wait for it to buffer, then pause it. If you add some prints, you'll see that m_paused remains false both at the end of MediaPlayerPrivateGStreamer::pause() and any MediaPlayerPrivateGStreamer::updateStates() that follows the next main thread ticks short after. Second, where is m_isPaused set? 1) On initialization, it's set to a default value of true. 2) In updateStates(), when we get GST_STATE_CHANGE_SUCCESS to GST_STATE_PLAYING it is set to false. 3) In updateStates(), when we get GST_STATE_CHANGE_SUCCESS to GST_STATE_READY or GST_STATE_NULL, it is set to true. Notably this doesn't cover GST_STATE_PAUSED. 4) In updateStates(), when we get GST_STATE_CHANGE_NO_PREROLL to GST_STATE_PAUSED, m_isPaused is set to true. 5) In updateStates(), when we get GST_STATE_CHANGE_NO_PREROLL to GST_STATE_PLAYING m_isPaused is set to false. 6) In didEnd(), when !m_player->isLooping() && !isMediaSource(), it is set to true. 7) MediaPlayerPrivateGStreamerMSE uses m_isPaused to true on pause() and to false on play(). Notably, this usage has no overlap with the usage in MediaPlayerPrivateGStreamer, as MediaPlayerPrivateGStreamerMSE has its own versions of play(), pause(} and updateStates(). Third, where is m_isPaused actually read? 1) In updateStates(), when we get GST_STATE_CHANGE_SUCCESS to GST_STATE_PAUSED. If (didBuffering && !m_isBuffering && !m_isPaused && m_playbackRate), it changes the state back to PLAYING. Why? 2) In MediaPlayerPrivateGStreamer::configureMediaStreamAudioTracks(), where it's passed to webkitMediaStreamSrcConfigureAudioTracks() 3) At the beginning of updateStates(), written to `oldIsPaused`. 4) In updateStates(), when we get GST_STATE_CHANGE_NO_PREROLL, immediately after (4) and (5) from the previous point, where a transition to PLAYING will be made if (!m_isPaused && m_playbackRate). 5) At the end of updateStates(), where if (oldIsPaused != m_isPaused) configureMediaStreamAudioTracks(); 6) In MediaPlayerPrivateGStreamer::performTaskAtMediaTime(), as part of an early return: if (!m_pipeline || m_didErrorOccur || m_isSeeking || m_isPaused || !m_playbackRate || !currentTime.isValid()) return false; 7) In MediaPlayerPrivateGStreamerMSE::updateStates(), which uses its own meaning of m_isPaused [see (7) in the previous section] to determine whether to change the pipeline state. Can we conclude a meaning for m_isPaused? A) In MSE streams, m_isPaused represents whether playback or pause has last been requested with pause() and play(), defaulting to true before playback starts. B) In live streams it represents whether at an unspecified point in time after the main thread tick in which play() or pause() are called, whether the playback was paused or resumed. C) In regular non-live streams, it represents whether playback has ended with a EOS with looping set to false since the pipeline successfully pre-rolled. These definitions are then used interchangeably (likely inadvertedly) at all the mentioned points, causing who knows what unwanted side effects. MediaPlayerPrivateGStreamer::m_isPaused should represent something useful and consistent for all the possible cases (regular playback, live playback, MSE, WebRTC) or be deleted from MediaPlayerPrivateGStreamer.
PR to add a FIXME: https://github.com/WebKit/WebKit/pull/16833
Committed 267418@main (779cc0077bb0): <https://commits.webkit.org/267418@main> Reviewed commits have been landed. Closing PR #16833 and removing active labels.