diff --git a/src/Session.cpp b/src/Session.cpp index 5188dc545..4b0750505 100644 --- a/src/Session.cpp +++ b/src/Session.cpp @@ -713,7 +713,7 @@ void CSession::EnableStream(CStream* stream, bool enable) if (!m_timingStream || stream->m_info.GetStreamType() == INPUTSTREAM_TYPE_VIDEO) m_timingStream = stream; - stream->m_isEnabled = true; + stream->SetIsEnabled(true); } else { @@ -821,7 +821,7 @@ bool SESSION::CSession::GetNextSample(ISampleReader*& sampleReader) if (!streamReader) continue; - if (stream->m_isEnabled) + if (stream->IsEnabled()) { // Advice is that VP does not want to wait longer than 10ms for a return from // DemuxRead() - here we ask to not wait at all and if ReadSample has not yet @@ -914,7 +914,7 @@ bool SESSION::CSession::SeekTime(double seekTime, unsigned int streamId, bool pr uint64_t maxTime{0}; for (auto& stream : m_streams) { - if (stream->m_isEnabled && (curTime = stream->m_adStream.getMaxTimeMs()) && curTime > maxTime) + if (stream->IsEnabled() && (curTime = stream->m_adStream.getMaxTimeMs()) && curTime > maxTime) { maxTime = curTime; } @@ -961,7 +961,7 @@ bool SESSION::CSession::SeekTime(double seekTime, unsigned int streamId, bool pr continue; streamReader->WaitReadSampleAsyncComplete(); - if (stream->m_isEnabled && (streamId == 0 || stream->m_info.GetPhysicalIndex() == streamId)) + if (stream->IsEnabled() && (streamId == 0 || stream->m_info.GetPhysicalIndex() == streamId)) { bool reset{true}; // all streams must be started before seeking to ensure cross chapter seeks @@ -1043,7 +1043,7 @@ void SESSION::CSession::OnStreamChange(adaptive::AdaptiveStream* adStream) { for (auto& stream : m_streams) { - if (stream->m_isEnabled && &stream->m_adStream == adStream) + if (stream->IsEnabled() && &stream->m_adStream == adStream) { UpdateStream(*stream); m_changed = true; diff --git a/src/Stream.h b/src/Stream.h index 763ffa934..da82d0452 100644 --- a/src/Stream.h +++ b/src/Stream.h @@ -25,12 +25,24 @@ class ATTR_DLL_LOCAL CStream CStream(adaptive::AdaptiveTree* tree, PLAYLIST::CAdaptationSet* adp, PLAYLIST::CRepresentation* initialRepr) - : m_isEnabled{false}, m_adStream{tree, adp, initialRepr}, m_isValid{true} + : m_adStream{tree, adp, initialRepr}, m_isValid{true} { } ~CStream() { Disable(); } + /*! + * \brief Determines whether the stream is enabled for playback + * \return True if the stream is enabled, otherwise false + */ + bool IsEnabled() const { return m_isEnabled; } + + /*! + * \brief Set if the stream is enabled for playback + * \param isEnabled Set to true to enable the stream + */ + void SetIsEnabled(bool isEnabled) { m_isEnabled = isEnabled; }; + /*! * \brief Stop/disable the AdaptiveStream and reset */ @@ -80,13 +92,13 @@ class ATTR_DLL_LOCAL CStream m_adByteStream = std::move(adByteStream); } - bool m_isEnabled; std::optional m_mainStreamIndex; // Used when this stream is "included" to video (main stream) adaptive::AdaptiveStream m_adStream; kodi::addon::InputstreamInfo m_info; bool m_isValid; private: + bool m_isEnabled{false}; std::unique_ptr m_streamReader; std::unique_ptr m_adByteStream; std::unique_ptr m_streamFile; diff --git a/src/decrypters/DrmEngine.cpp b/src/decrypters/DrmEngine.cpp index efec20507..7f84d29ef 100644 --- a/src/decrypters/DrmEngine.cpp +++ b/src/decrypters/DrmEngine.cpp @@ -662,7 +662,7 @@ void DRM::CDRMEngine::ExtractStreamProtectionData(PLAYLIST::CRepresentation* rep SESSION::CStream stream{ const_cast(&CSrvBroker::GetResources().GetTree()), adp, repr}; - stream.m_isEnabled = true; + stream.SetIsEnabled(true); stream.m_adStream.start_stream(); stream.SetAdByteStream(std::make_unique(&stream.m_adStream)); stream.SetStreamFile(std::make_unique(*stream.GetAdByteStream(), diff --git a/src/main.cpp b/src/main.cpp index 94179a4b9..cdff404cb 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -165,26 +165,31 @@ void CInputStreamAdaptive::EnableStream(int streamid, bool enable) CStream* stream{m_session->GetStream(m_session->GetStreamIndexFromId(streamid))}; - if (!enable && stream && stream->m_isEnabled) + if (!enable && stream && stream->IsEnabled()) { UnlinkIncludedStreams(stream); m_session->EnableStream(stream, false); } } -// If we change some Kodi stream property we must to return true -// to allow Kodi demuxer to reset with our changes the stream properties. +// OpenStream method notes: +// - This method is called: +// - At playback start +// - At chapter/period change (DEMUX_SPECIALID_STREAMCHANGE) +// - At stream quality change (DEMUX_SPECIALID_STREAMCHANGE) by "adaptive" streaming or from Kodi OSD +// - Due to CDVDDemuxClient::ParsePacket (Kodi core) while in playback, see "Kodi core is attempting to reopen the stream" below +// - The "streamid" requested can be influenced from preferences set in Kodi settings (e.g. language). +// - If the requested "streamid" fails to open on the Kodi core side (after OpenStream callback, e.g. for missing extradata) +// Kodi core will try to (fallback) open another video "streamid", this will happen recursively until success. +// - The OpenStream method not only opens the stream, but also implicitly enables it +// so don't exists a EnableStream callback to explicitly enable the stream after the opening, +// EnableStream method is used by VP only to disable the stream +// which can happen immediately after opening (e.g. subtitles disabled on playback startup). +// - If a stream info property has been changed, you need to return "true" on OpenStream +// to allow Kodi core to update its internal properties with our changes. bool CInputStreamAdaptive::OpenStream(int streamid) { LOG::Log(LOGDEBUG, "OpenStream(%d)", streamid); - // This method can be called when: - // - Stream first start - // - Chapter/period change - // - Automatic stream (representation) quality change (such as adaptive) - // - Manual stream (representation) quality change (from OSD) - // streamid behaviour: - // - The streamid can be influenced by Kodi core based on preferences set in Kodi settings (e.g. language) - // - Fallback calls, e.g. if the opened video streamid fails, Kodi core will try to open another video streamid if (!m_session) return false; @@ -194,28 +199,51 @@ bool CInputStreamAdaptive::OpenStream(int streamid) if (!stream) return false; - if (stream->m_isEnabled) + if (stream->IsEnabled()) { + // Stream quality changed if (stream->m_adStream.StreamChanged()) { UnlinkIncludedStreams(stream); stream->Reset(); stream->m_adStream.Reset(); } - else + else // Kodi core is attempting to reopen the stream { - LOG::Log(LOGDEBUG, "OpenStream(%d): The stream has already been opened", streamid); - return false; + // If a stream was already opened means that Kodi core is attempting to reopen + // ALL streams just for his internal purposes and this callback must be avoided. + + //! @todo: This issue appears to have been introduced with PR https://github.com/xbmc/xbmc/pull/10097 + //! when "changes" var differs from the stored value, it force to reopen all streams, + //! where in the past was reopening only the associated stream. + //! + //! The request to reopen a stream is unclear, + //! there is no reason to open a stream already opened, because the EnableStream(false) callback was never sent + //! so the state of the addon is still unchanged. + //! + //! Looks like that the "changes" var is modified by CDVDDemuxClient::ParsePacket while in playback + //! https://github.com/xbmc/xbmc/blob/d1a1d48c3cb3722d39264ffdd8132f755ffecd27/xbmc/cores/VideoPlayer/DVDDemuxers/DVDDemuxClient.cpp#L119 + //! maybe this is required for some other CDVDInputStream interface(?) but not for CInputStreamAddon, this should be at least optional + //! since can easily leads to playback problems such as stuttering because reopening all streams can be an heavy task to do while playback + //! moreover for subtitles case this is even more messed up, since they can be disabled... (EnableStream callback) + + // NOTE: you cannot rely on stream->IsEnabled() for all stream types because a playback can start with subtitles disabled + // so this leads to a mess between OpenStream and EnableStream callbacks causing abnormal behaviors in the addon components. + // As workaround we detect the first stream opened twice times (OpenStream for subtitles type is always last), + // then we set m_checkCoreReopen to true in order to skip all the following callbacks to OpenStream, + // and when we receive the first DemuxRead callback means that Kodi core has finished all openings, + // then we reset m_checkCoreReopen for the next round of OpenStream callbacks + m_checkCoreReopen = true; } } - stream->m_isEnabled = true; + if (m_checkCoreReopen) + { + LOG::Log(LOGDEBUG, "OpenStream(%d): The stream has already been opened", streamid); + return false; + } - //! @todo: for live with multiple periods (like HLS DISCONTINUITIES) for subtitle case - //! when the subtitle has been disabled from video start, and happens a period change, - //! kodi call OpenStream to the subtitle stream as if it were enabled, and disable it with EnableStream - //! just after it, this lead to log error "GenerateSidxSegments: [AS-x] Cannot generate segments from SIDX on repr..." - //! need to find a solution to avoid open the stream when previously disabled from previous period + stream->SetIsEnabled(true); CRepresentation* rep = stream->m_adStream.getRepresentation(); @@ -228,7 +256,7 @@ bool CInputStreamAdaptive::OpenStream(int streamid) while ((mainStream = m_session->GetStream(mainStreamIndex++))) { - if (mainStream->m_info.GetStreamType() == INPUTSTREAM_TYPE_VIDEO && mainStream->m_isEnabled) + if (mainStream->m_info.GetStreamType() == INPUTSTREAM_TYPE_VIDEO && mainStream->IsEnabled()) break; } @@ -298,6 +326,10 @@ DEMUX_PACKET* CInputStreamAdaptive::DemuxRead(void) m_session->OnDemuxRead(); + // On Kodi core CDVDDemuxClient::ParsePacket occurs after the DemuxRead callback + // since it can cause to reopen all streams, reset the check just before + m_checkCoreReopen = false; + if (~m_failedSeekTime) { LOG::Log(LOGDEBUG, "Seeking to last failed seek position (%d)", m_failedSeekTime); diff --git a/src/main.h b/src/main.h index 9944cec28..b0d0ca7c4 100644 --- a/src/main.h +++ b/src/main.h @@ -65,6 +65,8 @@ class ATTR_DLL_LOCAL CInputStreamAdaptive : public kodi::addon::CInstanceInputSt std::atomic m_lastPts{PLAYLIST::NO_PTS_VALUE}; void UnlinkIncludedStreams(SESSION::CStream* stream); + + bool m_checkCoreReopen{false}; // Check if Kodi core will reopen all streams }; /*******************************************************/