Skip to content

[pvr] open default streams after demuxer changed streams #1564

Merged
merged 1 commit into from Oct 10, 2012

5 participants

@xhaggi
Team Kodi member
xhaggi commented Oct 7, 2012

This PR adds an additional call of OpenDefaultStreams to ReadPacket if demuxer sends the special stream id DMX_SPECIALID_STREAMCHANGE.

Currently the call of OpenDefaultStreams within CDVDPlayer::Process() has no effect for pvr streams, because at this time no stream information is available.

@ghost
ghost commented Oct 7, 2012

If this fixes the subtitles

+1

@xhaggi
Team Kodi member
xhaggi commented Oct 7, 2012

it takes care of audio and subtitle streams

@MartijnKaijser
Team Kodi member

@ocram
No need to comment on PRs without having tested anything

@elupus
Team Kodi member
elupus commented Oct 7, 2012
@elupus
Team Kodi member
elupus commented Oct 7, 2012
@opdenkamp
Team Kodi member

@elupus that doesn't take guisettings like prefered subtitle language into account. we don't want just any stream. not saying that this hack is correct, but it works around the issue.

@elupus
Team Kodi member
elupus commented Oct 8, 2012
@opdenkamp
Team Kodi member

both when using an add-on with it's own demuxer, as the method is called before the stream info is available. we could call it after a stream change packet and when receiving the first packet in CDVDPlayer::ReadPacket(), similar to what this patch is proposing

@elupus
Team Kodi member
elupus commented Oct 8, 2012
@opdenkamp
Team Kodi member

hmm, that will add an extra requirement to add-ons, that they always send a stream change packet at the start, to work around a problem in XBMC. that's nasty

@elupus
Team Kodi member
elupus commented Oct 8, 2012
@opdenkamp
Team Kodi member

it's not new for PVR ;-) but yeah, i agree, putting a stream change in the wrapper and calling OpenDefaultStreams when receiving that one would fix it

@xhaggi
Team Kodi member
xhaggi commented Oct 8, 2012

@opdenkamp what do you think about calling a CDVDDemuxPVRClient::Read() in CDVDDemuxPVRClient::Open() like it's implemented in CDVDDemuxHTSP::Open(). This would fix it too.

@opdenkamp
Team Kodi member

that would block in the Open() call though. i'm not sure what kind of effect this has without testing it.

@opdenkamp
Team Kodi member

and the solution in the wrapper looks like a better one, as that will also work for normal stream changes.

@xhaggi
Team Kodi member
xhaggi commented Oct 8, 2012

i will do some tests later and report if it's working .. what do you mean with "putting a stream change in the wrapper and calling OpenDefaultStreams when receiving that one"

@opdenkamp
Team Kodi member

there's a special demux packet to notify the player about changes in the streams from an add-on (dmxid_specialid or something like that, don't have the source checked out atm). you also need to reopen the correct stream after such a change. if you make sure that the wrapper always send that special packet as first packet when reading, then you don't need to hack around in dvdplayer, but just call OpenDefaultStreams when receiving such a packet. which is valid behaviour anyway.

@xhaggi
Team Kodi member
xhaggi commented Oct 8, 2012

you mean DMX_SPECIALID_STREAMCHANGE ... thanks for the explanation

@FernetMenta
Team Kodi member

As far as I can see is that OpenDefaultStreams() triggers general reset of video player. A DMX_SPECIALID_STREAMCHANGE event is fired e.g. when there is a new subtitle stream. I don't think we want to reset video player in this case.

@opdenkamp
Team Kodi member

hmm no we'd only want it to reset the player if needed, but the other stuff in that method, like selecting the correct subtitle is needed then

@xhaggi
Team Kodi member
xhaggi commented Oct 8, 2012

i think we should ensure, that we hold the required stream information after opening the demuxer, so there is no need to change anything in DVDPlayer.

@opdenkamp
Team Kodi member

there is no demuxer with the stream info yet when OpenDefaultStreams is called now

@xhaggi
Team Kodi member
xhaggi commented Oct 8, 2012

i know .. so we should read the first packet in CDVDDemuxPVRClient::Open() to get the stream info right after opening the demuxer. this would solve the issue.

@opdenkamp
Team Kodi member

and what about DMX_SPECIALID_STREAMCHANGE then? we need to re-check the streams again then. so just sending such a packet as first one in the wrapper and responding to that still seems like a better option to me

@xhaggi
Team Kodi member
xhaggi commented Oct 8, 2012

@opdenkamp if you prefer this, i will try it .. i don't want unnecessary changes to DVDPlayer, because CDVDDemuxHTSP::Open integrates it on the same way

@opdenkamp
Team Kodi member

well that option looks like the least code changing in dvdplayer to me. dvdplayer is @elupus territory, not mine, so it should be whatever he prefers

@xhaggi
Team Kodi member
xhaggi commented Oct 8, 2012

it is now ensured that stream information are available when opening the PVR demuxer, so that the call of OpenDefaultStreams in DVDPlayer works like expected.

we can add the additional calls of OpenDefaultStreams in DVDPlayer, if it receives the special packet later.

@opdenkamp can you take a look at this.

@elupus no changes made to DVDPlayer

@FernetMenta
Team Kodi member

@xhaggi
This is wrong for several reasons.

1)
As @opdenkamp already pointed out, you can't block in Open
It might take some time for the backend to tune to a channel. Normal demuxer read can be aborted but I guess you are stuck in Open if the backend does not manage it to tune the channel.

2)
Still no solution for changing streams (e.g. subtitle stream is added during playback)
There is no guarantee that all streams are ready on first Read()

@xhaggi
Team Kodi member
xhaggi commented Oct 8, 2012

i thought this could be fixed within the demuxer :/ .. will change this tomorrow or so.

@xhaggi
Team Kodi member
xhaggi commented Oct 8, 2012

updated the PR: now it checks the special stream id DMX_SPECIALID_STREAMCHANGE, update the selection streams and open the default streams. it would be great to know, if i'm on the right way.

@elupus
Team Kodi member
elupus commented Oct 8, 2012

@opdenkamp @FernetMenta your choice on this one. it's alright by me.

@opdenkamp
Team Kodi member

yup, fixes the issue, thanks

@opdenkamp opdenkamp and 1 other commented on an outdated diff Oct 9, 2012
xbmc/cores/dvdplayer/DVDPlayer.cpp
@@ -804,6 +807,18 @@ bool CDVDPlayer::ReadPacket(DemuxPacket*& packet, CDemuxStream*& stream)
if(packet)
{
+ // stream changed, update and open defaults
+ if(packet->iStreamId == DMX_SPECIALID_STREAMCHANGE)
+ {
+ // reset the caching state for pvr streams
+ if (m_pInputStream->IsStreamType(DVDSTREAM_TYPE_PVRMANAGER))
@opdenkamp
Team Kodi member
opdenkamp added a note Oct 9, 2012

hmm, why do we need this one?

@opdenkamp
Team Kodi member
opdenkamp added a note Oct 9, 2012

and it looks like this leads to an unneeded pause+resume when receiving an update subtitle stream for instance.

@xhaggi
Team Kodi member
xhaggi added a note Oct 9, 2012

if you think we don't need a cache reset, i will remove this .. in my tests this had no negative effects

@opdenkamp
Team Kodi member
opdenkamp added a note Oct 9, 2012

it's even unwanted if it's not strictly needed, as it can temporarily pause playback when it's not needed

@xhaggi
Team Kodi member
xhaggi added a note Oct 9, 2012

i think we don't need this .. but why do you use it on the subtitle part?

@opdenkamp
Team Kodi member
opdenkamp added a note Oct 9, 2012

what do you mean exactly?

@xhaggi
Team Kodi member
xhaggi added a note Oct 9, 2012

line 777 at the subtitle part it's used too. i copied it from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@opdenkamp opdenkamp and 1 other commented on an outdated diff Oct 9, 2012
xbmc/cores/dvdplayer/DVDPlayer.cpp
if(packet->iStreamId == DMX_SPECIALID_STREAMCHANGE)
{
// reset the caching state for pvr streams
if (m_pInputStream->IsStreamType(DVDSTREAM_TYPE_PVRMANAGER))
SetCaching(CACHESTATE_PVR);
- CDVDDemuxUtils::FreeDemuxPacket(packet);
+ m_SelectionStreams.Clear(STREAM_NONE, STREAM_SOURCE_DEMUX_SUB);
@opdenkamp
Team Kodi member
opdenkamp added a note Oct 9, 2012

is this one needed too?

@xhaggi
Team Kodi member
xhaggi added a note Oct 9, 2012

i will check if it's needed

@xhaggi
Team Kodi member
xhaggi added a note Oct 9, 2012

@opdenkamp If no subtitle streams disappear, then we do not need this.

@opdenkamp
Team Kodi member
opdenkamp added a note Oct 9, 2012

and if they disapear, then it should be fixed in OpenDefaultStreams() I think, and it will also happen for other stream types if that's the case.

@xhaggi
Team Kodi member
xhaggi added a note Oct 9, 2012

I think that this functionality should not be placed into OpenDefaultStreams .. it open streams, not clear or modify the stream list. If we can not ensure that no streams disappear, then we leave it the way it is.

Clear the stream list and update them with the new ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@opdenkamp opdenkamp and 1 other commented on an outdated diff Oct 9, 2012
xbmc/cores/dvdplayer/DVDPlayer.cpp
if(packet->iStreamId == DMX_SPECIALID_STREAMCHANGE)
{
// reset the caching state for pvr streams
if (m_pInputStream->IsStreamType(DVDSTREAM_TYPE_PVRMANAGER))
SetCaching(CACHESTATE_PVR);
- CDVDDemuxUtils::FreeDemuxPacket(packet);
@opdenkamp
Team Kodi member
opdenkamp added a note Oct 9, 2012

aren't we leaking now? doesn't seem to be freed anymore when returning?

@elupus
Team Kodi member
elupus added a note Oct 9, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@opdenkamp opdenkamp was assigned Oct 9, 2012
@FernetMenta FernetMenta and 2 others commented on an outdated diff Oct 9, 2012
xbmc/cores/dvdplayer/DVDPlayer.cpp
if(packet->iStreamId == DMX_SPECIALID_STREAMCHANGE)
{
// reset the caching state for pvr streams
if (m_pInputStream->IsStreamType(DVDSTREAM_TYPE_PVRMANAGER))
SetCaching(CACHESTATE_PVR);
- CDVDDemuxUtils::FreeDemuxPacket(packet);
+ m_SelectionStreams.Clear(STREAM_NONE, STREAM_SOURCE_DEMUX_SUB);
+ m_SelectionStreams.Update(NULL, m_pSubtitleDemuxer);
+ OpenDefaultStreams();
return true;
@FernetMenta
Team Kodi member
FernetMenta added a note Oct 9, 2012

This needs special handling for this case. We don't want to reset players in case just a subtitle stream was added. Maybe add a parameter to OpenDefaultStreams()

@xhaggi
Team Kodi member
xhaggi added a note Oct 9, 2012

will add a parameter to handle subtitle only on OpenDefaultStreams

@elupus
Team Kodi member
elupus added a note Oct 9, 2012

xhaggi, add a parameter that contains a mask of what players to open streams for. I think there is such a flag based define somewhere already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@xhaggi
Team Kodi member
xhaggi commented Oct 9, 2012

@opdenkamp i will do some more tests on the subtitle part to check if the additional stuff is needed.

@xhaggi
Team Kodi member
xhaggi commented Oct 9, 2012

I removed the unneeded cache reset and added a parameter to handle only opening a defined stream type in OpenDefaultStreams.

@xhaggi
Team Kodi member
xhaggi commented Oct 9, 2012

@elupus currently i use the enum stream type STREAM_NONE to determine if all streams should have to be opened. i would prefer to add a new enum value STREAM_ALL and use this instead on STREAM_NONE .. what do you think about it?

@xhaggi
Team Kodi member
xhaggi commented Oct 10, 2012

@opdenkamp i moved clear + update selectionstreams into OpenDefaultStreams

@elupus
Team Kodi member
elupus commented Oct 10, 2012

@xhaggi I don't like that move for the same reasons you initialy said. We do it extra times on open + it's wrong for dvd's. I think you missunderstood (i think anyway) what @opdenkamp mean about there about only being one call to OpenDefaultStreams(). I think he was talking about the fact that there was one extra for subtitle demuxer which as far as I know isn't even used for PVR...

@elupus
Team Kodi member
elupus commented Oct 10, 2012

Imho, revert that last commit and the change to OpenDefaultStreams and only keep the DMX_STREAM_CHANGE check for the standard demuxer.

@xhaggi
Team Kodi member
xhaggi commented Oct 10, 2012

@elupus changed + squashed commits .. thank you for clarifying this ;)

@elupus
Team Kodi member
@xhaggi
Team Kodi member
xhaggi commented Oct 10, 2012

@elupus done

@opdenkamp opdenkamp merged commit bcc0bb6 into xbmc:master Oct 10, 2012
@FernetMenta
Team Kodi member

@opdenkamp , @elupus

Now this 72e64a3 causes immediate reopen of video codec. At the time DMX_SPECIALID_STREAMCHANGE is received, the new PVR parser had no time to parse level and profile. Those are not even in the PVR stream properties.

Team Kodi member
Team Kodi member

Maybe we should exaclty do this. PVR needs to fill the buffers anyway before it can start playback. If we added buffers to PVR demuxer, we could get rid of special buffer handling in dvdplayer.

Team Kodi member
Team Kodi member

I meant special handling for PVR. The input speed for PVR is real time and playback can't be started until buffers have filled to a certain level. With recent changes those messages are back:

WARNING: CDVDMessageQueue(audio)::Get - asked for new data packet, with nothing available
Team Kodi member
Team Kodi member

as long as we do have this special handling for pvr, i think a call to setcaching() when video or audio streams changed will fix that message popping up and possible stuttering. i asked @xhaggi to remove a call to that one earlier, as i couldn't see the need, but if we switch to another stream and flush, then we need to buffer up a bit again. or we need to fix that sync, but i don't know enough about dvdplayer's code to see how and where to fix it.

Team Kodi member
Team Kodi member

dvdplayer calls general reset on players which flushes the codec

Team Kodi member
Team Kodi member

It does not flush the queue but the codec reset does flush ffmpeg. As a result quite a few packets get lost and decoder requests packets from queue.

Team Kodi member
Team Kodi member

Yes, I think re-caching is required in this case but there are other issues maybe introduced with PVR timeshift. Sometimes I get a non disappearing seeking dialog when switching channels.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.