Improve stream handling

[Jalle19]

Changes:

• don't clear the stream vector except when completely stopping playback (ie. when the demux is closed).
• only compare the stream codec when determining if a stream still exists or not

With these changes the streams don't get shuffled around during channel switches. I don't see the point in treating general stream changes (such as a new audio stream appearing suddenly) with stream changes that happen due to a channel switch. In many cases two different channels have the exact same type of streams (e.g. one MPEG-2 video and one MP2 audio stream).

Coupled with https://github.com/Jalle19/xbmc/compare/faster-channel-switching (which I'm currently testing locally, feel free to check it out) this means neither the demuxer nor the audio sink have to be reset when switching channels which considerable speeds up channel switching. With this PR it can be so fast that it's hard to measure accurately with a stopwatch (due to reaction time).

@opdenkamp and @FernetMenta I'd appreciate your input as usual.

[elupus]

Why is this needed? Why can't we clear streams? What is it in dvdplayer that causes it to be slow in opening streams

[elupus]

Your other branch contain too much unrelated changes to review. And stuff that breaks dvd playback (demuxer reset).

[elupus]

If two streams have the same channel type, the (audio/video) players should detect this and avoid the re-open internally.

[Jalle19]

Why is this needed? Why can't we clear streams? What is it in dvdplayer that causes it to be slow in opening streams

Why should we clear the streams?

Your other branch contain too much unrelated changes to review. And stuff that breaks dvd playback (demuxer reset).

I could split the unrelated stuff into a separate PR, I just added them as I went while browsing through the code. Which part breaks DVD playback (naturally I haven't tested that)?

Basically, what currently happens when switching channels is something like this:

1. The demuxer is re-opened, which calls RequestStreams(). Here we get the streams for the new channel and so the various decoders start to open.
2. Moments later the DMX_SPECIALID_STREAMCHANGE packet comes in which resets the demuxer and step 1 is performed all over again.
3. Caching state is set to CACHESTATE_PVR which buffers data for a while before starting playback.
4. OpenDefaultStreams() is called at least once which sets the default audio and subtitle language, something which is done anyway when LoadCurrentChannelSettings() is called.

The new approach basically removes all these steps so that the player never stops playing, only thing that happens on a channel change is that the streams are switched inplace, then playback continues.

Obviously I'm not very familiar with the code, I've analyzed the logs during a channel switch and added a bunch of logging to determine what paths the code take.

[elupus]

What i mean is why is it a problem that we re-do a OpenDefaultStreams()?

If we end up resetting the demuxer twice that does indeed some bad. Why do we end up doing that?

[Jalle19]

What i mean is why is it a problem that we re-do a OpenDefaultStreams()?

I can't answer for sure cause I haven't looked into it that much. It could turn out that it is needed after all, if so the necessary parts could probably be factored out.

If we end up resetting the demuxer twice that does indeed some bad. Why do we end up doing that?

The demuxer is always reset in CDVDPlayer::OpenDemuxStream(). It's also destructed in a bunch of other places (see Jalle19/xbmc@dbbdbe0#diff-14ed9651a2eb656ff75aea52d0912a00L739). I'm not familiar enough with the code to know why that is, most of it is 5+ years old.

[FernetMenta]

I don't see that the demuxer is destructed after DMX_SPECIALID_STREAMCHANGE. demuxer is detructed on channel change which is fine.

[Jalle19]

@FernetMenta it doesn't happen directly but sooner or later it will be destructed, usually in OpenDemuxStream().

Why do we want to close it if it's not necessary? As with all other things it takes time, plus it's not necessary.

[opdenkamp]

where's that called exactly? it shouldn't destruct things when receiving DMX_SPECIALID_STREAMCHANGE unless needed.

[amet]

just tested this, works like a charm… zapping is easy and seamless.

not sure about changes needed in DVDPlayer but from user perspective its a very nice improvement.gets my +100

[FernetMenta]

You can't feed new stream into an existing decoder. This is wrong and would break some scenarios. Further, we have up to 8 seconds buffered. Your changes let the buffer play out, then continue with the new stream.

[Jalle19]

where's that called exactly? it shouldn't destruct things when receiving DMX_SPECIALID_STREAMCHANGE unless needed.

The demuxer (m_pDemuxer) is destroyed all over the place. This causes all streams/decoders to be reopened.

You can't feed new stream into an existing decoder. This is wrong and would break some scenarios.

Could you elaborate? The stream parameters are checked in CDVDDemuxPVRClient::RequestStreams (which is called on DMX_SPECIALID_STREAMCHANGE) and the stream is disposed if the parameters (like width/height, codec etc.) don't match.

Further, we have up to 8 seconds buffered. Your changes let the buffer play out, then continue with the new stream.

Which change affects this? I haven't seen this behavior at all.

[FernetMenta]

A stream has more parameters coded into extradata. For new streams we initialize a parser and split extradata. Reusing this data for a different stream is wrong.

[Jalle19]

From what I can tell the extradata is always reset in RequestStreams(), even when the stream is reused.

I'll remove the unrelated changes from the other branch and put it up for review so we can continue the discussion there. There's obviously some things that I've missed but I think it will be easier for everyone to comment and refer to parts of the code if it's in PR form.

[Jalle19]

xbmc/xbmc#3964

[Jalle19]

Any updates on this? Any reason why it couldn't be merged?

[FernetMenta]

this is wrong and would break things. streams are not equal if codec id matches.

[Jalle19]

@FernetMenta apart from that, is the rest valid?

[FernetMenta]

nope, it is wrong trying to reuse a configured decoder for a different stream.

[Jalle19]

The stream is supposed to be disposed in DVDDemuxPVRClient if it differs in any way.

[FernetMenta]

no, I already explained this to you. demux client would miss any difference in extradata. Reuse is wrong and there is no reason to postpone cleanup of old streams.

[Jalle19]

Okay, I guess there's no point in arguing over this anymore. It seems to work, that's why I've been trying to push it. Could you finally comment on Jalle19@a762427? It kind of goes together with Jalle19/xbmc@5b5bb5e