make ffmpeg demuxer observe pmt changes #1757

Merged
5 commits merged into from May 4, 2013

4 participants

@FernetMenta
Team Kodi member

@elupus
I have borrowed the idea of registering a callback for pmt changes from mythtv. Streams which are not in the current pmt are set inactive in order to be ignored by OpenDefaultStreams.
I tested with vdr recordings which have a pat/pmt before every GOP. pvr clients which use this demuxer need to send pat/pmt at least when there are changes.

@elupus
Team Kodi member
@elupus
Team Kodi member
@FernetMenta
Team Kodi member

I don't like the buffered packet either, this is why I though putting this info somehow in the inputstream and the player queries this before processing the packet.

It would be much better to output a packet from ffmpeg on a separate data stream containing the stream change

This indeed would be nice but I have no idea how to do this. The functions inside ffmpeg which detect pat and pmt are callbacks itself. I have no clue how to stop reading after a pmt and put this packet into a separate stream.

@elupus
Team Kodi member
@FernetMenta
Team Kodi member

Thanks for the hint, will look into it.

@FernetMenta
Team Kodi member

@elupus I am facing the following problem: at the time ffmpeg hits a new pmt, potentially new streams have not been parsed. Firing a DMX_SPECIALIZED_STREAMCHANGE at this time does only work when streams are dropped.
I think we need to fire the event also after CDVDDemuxFFmpeg::AddStream was called like in this pr. That again raises the problem with the buffered packet.

I could defer the event until e.g. all stream have reset AVSTREAM_PARSE_FULL but this is going to get ugly in ffmpeg. Wouldn't it be better to fire the stream change on every change: streams added/dropped AND any change of a existing stream?

@FernetMenta
Team Kodi member

next round. There's an error with the change signal in input stream, player should continue and process packet.

@elupus elupus and 1 other commented on an outdated diff Nov 11, 2012
lib/ffmpeg/libavformat/mpegts.c
@@ -1474,8 +1512,7 @@ static void pmt_cb(MpegTSFilter *filter, const uint8_t *section, int section_len
goto out;
// stop parsing after pmt, we found header
- if (!ts->stream->nb_streams)
- ts->stop_parse = 2;
+ ts->stop_parse = 2;
@elupus
Team Kodi member
elupus added a line comment Nov 11, 2012

Hmm.. wouldn't this always stop?

@FernetMenta
Team Kodi member
FernetMenta added a line comment Nov 11, 2012

Yep, this flushes the out the last PES packet. Right after this the stream change is sent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elupus elupus and 1 other commented on an outdated diff Nov 11, 2012
xbmc/cores/dvdplayer/DVDInputStreams/DVDInputStream.h
protected:
DVDStreamType m_streamType;
std::string m_strFileName;
BitstreamStats m_stats;
std::string m_content;
CFileItem m_item;
+ bool m_streamChange;
};
@elupus
Team Kodi member
elupus added a line comment Nov 11, 2012

Eh why. Should be signaled same way as for pvr.

@FernetMenta
Team Kodi member
FernetMenta added a line comment Nov 11, 2012

Just an idea to prevent from the buffered packet. We should send streamchange also when a new stream was created, means completely parsed and added to the streams in demuxer. This can't be done with the demux packet.

EDIT: without buffing the old packet

@elupus
Team Kodi member
elupus added a line comment Nov 11, 2012

Why?.. when av_find_stream_info ends. We have found the streams that should be available. If things change after that we need to push it out as a packet. The buffered packet is a better approach than this.

@FernetMenta
Team Kodi member
FernetMenta added a line comment Nov 11, 2012

We have to distinguish between adding a stream and dropping a steam. Right after a pmt change we can check if a stream needs to be dropped. But a new added stream (while already playing) is not ready right after the pmt signal. demuxer would add it to the streams with first packet belonging to the stream coming out of ffmpeg. This is the time to signal a stream change to player. Unfortunately we have a packet at this time.

@elupus
Team Kodi member
elupus added a line comment Nov 11, 2012

I prefer we signal it when found.. if it changes later, we should detect that.

@FernetMenta
Team Kodi member
FernetMenta added a line comment Nov 11, 2012

How do we detect later? Assume a new audio track gets added with preferred language. Right after pmt change the stream is not ready and would be ignored or not found by OpenDefaultStreams.

@elupus
Team Kodi member
elupus added a line comment Nov 11, 2012

Why isn't it ready? You mean based on parsing? If it's based on parsing, it's a larger problem really. We always need to detect when extradata or info in a stream changes properly. So it has no relevance to the problem at hand really.

@FernetMenta
Team Kodi member
FernetMenta added a line comment Nov 11, 2012

also respond to your post on mpegts.c

ffmpeg never removes a stream, it just adds new streams. Hence I think a pmt change which removes pids from a program is not a problem and we don't need to reopen demuxer (this takes quite a while to complete). pmt_cb adds new pids to the program and the stream to the context if it does not already exist. Right after we get the pmt change packet the new stream is in the list in FormatContext but not in the list of streams in ffmpeg demuxer (parsed or not).
ffmpeg demuxer already adds new streams or updates existing ones. This works quite well, just the notification to player is missing. I think whenever AddStream (in Read) is called we should signal a stream change.

I have been testing with a mpegts recording which has an original language audio track (set to preferred). When I seek past the show where the audio track is gone, I get the other audio track opened. Seeking back into the movie re-opens the right audio track again. It does not create a new stream, it just reactivates the old one.
This is exactly the behavior I would expect.

@FernetMenta
Team Kodi member
FernetMenta added a line comment Nov 11, 2012

Sure, we can add the ffmpeg streams to the demuxer streams right after the pmt signal. Language should be set by pmt, right? Will give this a try.

My synapses are running hot with that ffmpeg stuff :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elupus elupus and 1 other commented on an outdated diff Nov 11, 2012
lib/ffmpeg/libavformat/mpegts.c
@@ -1542,6 +1579,9 @@ static void pmt_cb(MpegTSFilter *filter, const uint8_t *section, int section_len
p = desc_list_end;
}
+ ts->cur_programid = h->id;
+ ts->new_pmt = 1;
@elupus
Team Kodi member
elupus added a line comment Nov 11, 2012

You can create the packet here and just set stop_parse directly.

@FernetMenta
Team Kodi member
FernetMenta added a line comment Nov 11, 2012

see above, the last PES needs to be flushed.

@elupus
Team Kodi member
elupus added a line comment Nov 11, 2012

well, the problem is that if this is the first thing that occur, it will not stop parsing and it will output a pes packet belonging to the new pmt before the new pmt is output. Ie the wrong order.

If we get a new pmt in the middle of a pes packet, i suspect we can flush the old pes. So setting stop_parse here would make sense i think.

This is really tricky code thou. And we do want something we can get upstream, so we need to get this nice.

@FernetMenta
Team Kodi member
FernetMenta added a line comment Nov 11, 2012

I don't claim that I have full understood this all. But it's not a bad idea playing around with the bits before asking the ffmpeg devs.

@elupus
Team Kodi member
elupus added a line comment Nov 11, 2012
@elupus
Team Kodi member
elupus added a line comment Nov 12, 2012

I still think we should generated the pmt change packet right here. Any in progress PES will very likely belong to a stream that will continue after the change. So it is not a problem for it to be output after the PMT packet.

@FernetMenta
Team Kodi member
FernetMenta added a line comment Nov 12, 2012

Do you mean constructing the packet here and setting stop_parse to 1? Will try this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@FernetMenta
Team Kodi member

@elupus I have update the pr. Now ffmpeg does not stop after a pmt. Potentially late packets from a previous pmt get filtered out in demuxer. Are you ok with this? It prevents inactivated streams from being activated again.

As you suggested ProcessStreamChange adds new streams and inactivates those which are not in the pmt.
The concept seems to work though the code needs to get some clean-up.

  • What do you suggest for the stream id of the data channel? Zero or a negative value.
  • How should the coding of pids into the data section of the packet happen? Should we add some preceding characters like 'p' 'm' 't' followed by 16bit values?
@FernetMenta
Team Kodi member

updated. You are right, looks much cleaner now.

@elupus
Team Kodi member

i think we should output the whole PMT packet out of the demuxer. With it's packet id set to the correct PID. This would be more useful for other uses than some special packet.

I don't think we actually need to parse it in our demuxer. Only detect that there now was some type of change. Any stream that is no longer available should then be marked with discard by ffmpeg or similar that they have ended.

Ps. don't spend too much time on the xbmc side for now. We'll get there, but this is looking quite nice from the ffmpeg side.

@FernetMenta
Team Kodi member

Any stream that is no longer available should then be marked with discard by ffmpeg or similar that they have ended

That works for the case when a stream ends but what to for the case when a previously ended stream becomes available again. AV_DISCARD may have been set by the user, hence we can't reset it on a pmt change.
Of course, the user application can set the flag again as desired after the the pmt change event. This approach makes sense to but it changes (API) behavior slightly. Should we go for this?

@elupus
Team Kodi member

Hmm.. you are right.. it's probably a bad idea to re-use discard for this. We really need a stream flag that indicates that it has ended and is not available anymore (which we subsequently can clear if it comes back)

@FernetMenta
Team Kodi member

Having such a flag would be great but requires an API change.
How do you think about keeping the stream indices in AVProgram consistent with the internal Program structure in mpegts? Keeping indices of inactive streams in AVProgram looks wrong to me.

@elupus
Team Kodi member
@FernetMenta
Team Kodi member

updated.

@FernetMenta
Team Kodi member

@elupus is this something you consider for Frodo?

@elupus
Team Kodi member
@FernetMenta
Team Kodi member

Updating AVProgram was accepted upstream: FFmpeg/FFmpeg@4c41fc8

Opening a data stream for pmt changes would break ffmpeg encoding. We could just check AVProgram for a change before reading.

@elupus
Team Kodi member
@ghost

status?

@FernetMenta
Team Kodi member

This is currently tested by MediaPortal users. Together with FernetMenta@bf753eb it enables fast channel switches. So far it looks ok. Needs review by elupus.

@elupus
Team Kodi member

Are the ffmpeg changes what eventually got accepted upstream?

@FernetMenta
Team Kodi member

Both ffmpeg commits are taken from upstream and are obsolete after we got the update. One of them was submitted by us, the other one is a patch for something which got broken by the first one.

@elupus elupus commented on an outdated diff Apr 7, 2013
xbmc/cores/dvdplayer/DVDDemuxers/DVDDemuxFFmpeg.cpp
@@ -504,6 +496,12 @@ void CDVDDemuxFFmpeg::Dispose()
{
g_demuxer.set(this);
+ if(m_AVPacket.result >= 0)
@elupus
Team Kodi member
elupus added a line comment Apr 7, 2013

Not needed. av_free_packet should be null safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elupus elupus commented on an outdated diff Apr 7, 2013
xbmc/cores/dvdplayer/DVDDemuxers/DVDDemuxFFmpeg.cpp
@@ -647,10 +645,33 @@ DemuxPacket* CDVDDemuxFFmpeg::Read()
pkt.data = NULL;
pkt.stream_index = MAX_STREAMS;
- // timeout reads after 100ms
- m_timeout.Set(20000);
- int result = m_dllAvFormat.av_read_frame(m_pFormatContext, &pkt);
- m_timeout.SetInfinite();
+ int result = -1;
+ // check for saved packet after a program change
+ if(m_AVPacket.result >= 0)
+ {
+ // in case we did not move by seek or demuxer was flushed,
+ // take the packet of last read
+ if(m_AVPacket.pts == m_iCurrentPts)
@elupus
Team Kodi member
elupus added a line comment Apr 7, 2013

Drop this. Make sure a flush or seek free's the packet instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elupus elupus and 1 other commented on an outdated diff Apr 7, 2013
xbmc/cores/dvdplayer/DVDDemuxers/DVDDemuxFFmpeg.cpp
m_streams[pPacket->iStreamId]->pPrivate != m_pFormatContext->streams[pPacket->iStreamId] ||
m_streams[pPacket->iStreamId]->codec != m_pFormatContext->streams[pPacket->iStreamId]->codec->codec_id)
{
// content has changed, or stream did not yet exist
AddStream(pPacket->iStreamId);
+ m_streams[pPacket->iStreamId]->changes++;
@elupus
Team Kodi member
elupus added a line comment Apr 7, 2013

why can't AddStream do this increment?

@FernetMenta
Team Kodi member
FernetMenta added a line comment Apr 7, 2013

just noticed that this is not needed. AddStream deletes and recreated the stream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elupus elupus commented on an outdated diff Apr 7, 2013
xbmc/cores/dvdplayer/DVDDemuxers/DVDDemuxFFmpeg.cpp
@@ -1170,6 +1214,11 @@ void CDVDDemuxFFmpeg::AddStream(int iId)
else
m_streams[iId]->iPhysicalId = pStream->id;
}
+ if (!IsActiveStream(iId))
+ {
+ m_streams[iId]->type = STREAM_NONE;
+ m_streams[iId]->codec = CODEC_ID_NONE;
+ }
@elupus
Team Kodi member
elupus added a line comment Apr 7, 2013

Ah okey.. this is what solves the addition of all streams. I think i would prefer setting some other flag. Something like the discard value to skip the streams.

@elupus
Team Kodi member
elupus added a line comment Apr 7, 2013

We can change that later thou..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elupus elupus and 1 other commented on an outdated diff Apr 7, 2013
xbmc/cores/dvdplayer/DVDDemuxers/DVDDemuxFFmpeg.cpp
+ return false;
+}
+
+bool CDVDDemuxFFmpeg::IsProgramChange()
+{
+ if (m_program == UINT_MAX)
+ return false;
+
+ bool change(false);
+ int noOfStreams = GetNrOfStreams();
+ for (int i = 0; i < noOfStreams; i++)
+ {
+ if ((m_streams[i]->type == STREAM_NONE && IsActiveStream(i)) ||
+ (m_streams[i]->type != STREAM_NONE && !IsActiveStream(i)))
+ change = true;
+ }
@elupus
Team Kodi member
elupus added a line comment Apr 7, 2013

This is a really heavy operation to do for each packet read. Is there no other way to detect it?

@FernetMenta
Team Kodi member
FernetMenta added a line comment Apr 7, 2013

We had a solution (ffmpeg event) but unfortunately this part was not accepted upstream. Michael said that it would break some other functionality. Comparing new and old state is the only idea I have so far.

@elupus
Team Kodi member
elupus added a line comment Apr 7, 2013

If we only keep the active streams for the program just as we did before in the same order they are mentioned in the avprogram structure, we would only need to iterate over the list once.

@elupus
Team Kodi member
elupus added a line comment Apr 7, 2013

Something along the following lines:

if(m_pFormatContext->programs[m_program]->nb_stream_indexes != m_stream_count)
  return true;

for (unsigned int i = 0; i < m_pFormatContext->programs[m_program]->nb_stream_indexes; i++) {
  int idx = m_pFormatContext->programs[m_program]->stream_index[i];
  if(idx != m_streams[i].id)
     return true;
  if(m_pFormatContext->streams[idx]->codec->codec_type != m_streams[i].type)
     return true;
}
@FernetMenta
Team Kodi member
FernetMenta added a line comment Apr 7, 2013

Yep, doing the compare for number of streams first is a good idea.

@elupus
Team Kodi member
elupus added a line comment Apr 7, 2013
@FernetMenta
Team Kodi member
FernetMenta added a line comment Apr 7, 2013

I already copied your solution :)
Still need the check for type unknown. This ok?

if(m_pFormatContext->streams[idx]->codec->codec_type == AVMEDIA_TYPE_UNKNOWN)
continue;

@FernetMenta
Team Kodi member
FernetMenta added a line comment Apr 7, 2013

hmm, this does not match because we use our own types like STREAM_VIDEO
if(m_pFormatContext->streams[idx]->codec->codec_type != m_streams[i]->type)

We could add a paramter origType to the m_streams structure? This would make the check for unknown obsolete.

@elupus
Team Kodi member
elupus added a line comment Apr 7, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elupus elupus and 1 other commented on an outdated diff Apr 7, 2013
xbmc/cores/dvdplayer/DVDDemuxers/DVDDemuxFFmpeg.cpp
@@ -1327,3 +1376,37 @@ void CDVDDemuxFFmpeg::GetStreamCodecName(int iStreamId, CStdString &strName)
strName = codec->name;
}
}
+
+bool CDVDDemuxFFmpeg::IsActiveStream(int idx)
+{
+ if (m_program == UINT_MAX)
+ return true;
+
+ for (unsigned int i = 0; i < m_pFormatContext->programs[m_program]->nb_stream_indexes; i++)
+ {
+ if (idx == m_pFormatContext->programs[m_program]->stream_index[i] &&
+ m_pFormatContext->streams[idx]->codec->codec_type != AVMEDIA_TYPE_UNKNOWN)
@elupus
Team Kodi member
elupus added a line comment Apr 7, 2013

I would skip that unknown check here. If ffmpeg says it's active we should consider it active even if we don't know what to do with it.

@FernetMenta
Team Kodi member
FernetMenta added a line comment Apr 7, 2013

I put this in after a problem report. I got a sample with a unknown stream. AddStream sets this type to STREAM_NONE. As a consequence IsProgramChange detects changes all the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@FernetMenta
Team Kodi member

@elupus I push an update. Please check if I interpreted your comments correctly. It's only compile tested. I will run more tests once you are ok with it in general.

@FernetMenta
Team Kodi member

I have tested it with a couple of ts files with pmt changes and also a file with an unknown stream.

@elupus elupus commented on the diff Apr 8, 2013
xbmc/cores/dvdplayer/DVDDemuxers/DVDDemuxFFmpeg.cpp
+ }
+
+ return false;
+}
+
+bool CDVDDemuxFFmpeg::IsProgramChange()
+{
+ if (m_program == UINT_MAX)
+ return false;
+
+ if(m_pFormatContext->programs[m_program]->nb_stream_indexes != m_program_streams)
+ return true;
+
+ for (unsigned int i = 0; i < m_pFormatContext->programs[m_program]->nb_stream_indexes; i++)
+ {
+ int idx = m_pFormatContext->programs[m_program]->stream_index[i];
@elupus
Team Kodi member
elupus added a line comment Apr 8, 2013

Since you didn't go fully the way i proposed (ie only adding the program's streams to m_streams, not all). This need a check that the idx is in range of m_stream.

I would still prefer that you only add the program stream's to the structure just like it was before.

@FernetMenta
Team Kodi member
FernetMenta added a line comment Apr 8, 2013

consider a program with pids 1,2,3, and 4. ffmpeg puts those to index 0,1,2,and 3 (m_pFormatContext->streams[idx])
After a pmt change we have pids 5 and 6. ffmpeg puts those to indices 4 and 5. Means we have 2 active streams and a structure with 6 streams.
After a while it is pointless if we only added program streams to the structure. Am I wrong?

@elupus
Team Kodi member
elupus added a line comment Apr 8, 2013

Why only add streams? We should re-create our streams structure from scratch to always match the selected program. So after the pmt change we only have 2 streams in our structure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elupus elupus commented on the diff Apr 8, 2013
xbmc/cores/dvdplayer/DVDDemuxers/DVDDemuxFFmpeg.cpp
if(i != m_program)
m_pFormatContext->programs[i]->discard = AVDISCARD_ALL;
}
- if(m_program != UINT_MAX)
- {
- // add streams from selected program
- for (unsigned int i = 0; i < m_pFormatContext->programs[m_program]->nb_stream_indexes; i++)
- AddStream(m_pFormatContext->programs[m_program]->stream_index[i]);
- }
- }
- // if there were no programs or they were all empty, add all streams
- if (m_program == UINT_MAX)
- {
- for (unsigned int i = 0; i < m_pFormatContext->nb_streams; i++)
- AddStream(i);
@elupus
Team Kodi member
elupus added a line comment Apr 8, 2013

What i meant below is that this hunk should remain, potentially factored out to a separate function since you need to redo it on program change.

@FernetMenta
Team Kodi member
FernetMenta added a line comment Apr 8, 2013

Then we need to set all stream in m_streams to STREAM_NONE on a pmt change, then call AddStream only for the streams in the program (by using this hunk) ?

@FernetMenta
Team Kodi member
FernetMenta added a line comment Apr 8, 2013

If we only added e.g. index 3 and 4, GetNrOfStreams would fail.

@elupus
Team Kodi member
elupus added a line comment Apr 8, 2013

why? it would return 2

@FernetMenta
Team Kodi member
FernetMenta added a line comment Apr 9, 2013

It was you who told me that the structure doesn't like gaps. If we don't have a stream at index 0, it returns 0.


int CDVDDemuxFFmpeg::GetNrOfStreams()
{
  int i = 0;
  while (i < MAX_STREAMS && m_streams[i]) i++;
  return i;
}

The structure m_streams uses index given by m_pFormatContext->streams[iId].
The program m_pFormatContext->programs[m_program]->stream_index[i] is only a subset of the streams in m_pFormatContext->streams[iId].
Consider the case that we have e.g. 10 streams in m_pFormatContext->streams[iId] but the program only references stream 5 and 10.
pFormatContext->programs[m_program]->stream_index[0] ----> 5
pFormatContext->programs[m_program]->stream_index[1] ----> 10
This would result in a structure with gaps.

@elupus
Team Kodi member
elupus added a line comment Apr 9, 2013
@elupus
Team Kodi member
elupus added a line comment Apr 9, 2013
@FernetMenta
Team Kodi member
FernetMenta added a line comment Apr 9, 2013

For the scope of this pr, should we map ffmpeg indices to our ones?
our_index = m_index_map[ffmpeg_idx]

@elupus
Team Kodi member
elupus added a line comment Apr 9, 2013
@jmarshallnz
Team Kodi member
jmarshallnz added a line comment Apr 9, 2013

Don't let the end of the merge window hold things up unnecessarily. It's clearly ready to go once ya'll come to an agreement on how it works, so just slot it in as soon as you're done, regardless of dates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elupus elupus commented on an outdated diff Apr 8, 2013
xbmc/cores/dvdplayer/DVDDemuxers/DVDDemuxFFmpeg.h
XbmcThreads::EndTime m_timeout;
+ // Due to limitations of ffmpeg, we only can detect a program change
+ // with a packet. This struct saves the packet for the next read and
+ // signals STREAMCHANGE to player
+ struct
+ {
+ AVPacket pkt; // packet ffmpeg returned
+ int result; // result from av_read_packet
+ }m_AVPacket;
@elupus
Team Kodi member
elupus added a line comment Apr 8, 2013

call this m_pkt instead of m_AVPacket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@FernetMenta
Team Kodi member

Updated. Now m_streams is a map with ffmpeg idx as key. On a pmt change all streams are disposed and re-created.

@elupus
Team Kodi member

Incorporate my changes, test, rebase. Then i think we ar ready to go.

@elupus
Team Kodi member

Nice work on this btw. This has been sorely missing for quite some time.

@FernetMenta
Team Kodi member

Pulled your changes, rebased, and tested. I squashed 2 little changes to one of your commits: 58f9472280f78b8be4bc55a9953c7a2bc24a47c6

  • use of GetStreamInternal in Read(), was GetStream
  • typo in comment: breif -> brief
@ghost

good to go?

@FernetMenta
Team Kodi member

depends on elupus. I am running this for quite a while and didn't observe issues.

@elupus
Team Kodi member
@ghost ghost merged commit be6e169 into xbmc:master May 4, 2013
@Voyager1
Team Kodi member

bad news, this PR breaks playback of MKVs with VobSub subtitles (general segfault - "xbmc stopped working"). Haven't had time to investigate why, but at least reverting this fixes the issue.

@FernetMenta
Team Kodi member

can you point me to a sample?

@Voyager1
Team Kodi member

any movie with .idx/.sub vobsub really...

for what it's worth, here's the call stack upon crash:

    XBMC.exe!operator delete(void * pUserData)  Line 52 + 0x51 bytes    C++
    XBMC.exe!operator delete[](void * p)  Line 21 + 0x9 bytes   C++
    XBMC.exe!CDemuxStream::~CDemuxStream()  Line 110 + 0x12 bytes   C++
    XBMC.exe!CDemuxStreamSubtitle::~CDemuxStreamSubtitle()  + 0x41 bytes    C++
    XBMC.exe!CDVDDemuxVobsub::CStream::~CStream()  + 0x41 bytes C++
    XBMC.exe!CDVDDemuxVobsub::CStream::`scalar deleting destructor'()  + 0x14 bytes C++
    XBMC.exe!CDVDDemuxVobsub::~CDVDDemuxVobsub()  Line 45 + 0x2e bytes  C++
>   XBMC.exe!CDVDPlayer::AddSubtitleFile(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & filename, const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & subfilename, CDemuxStream::EFlags flags)  Line 3887 + 0x1b bytes   C++
    XBMC.exe!CDVDPlayer::OpenInputStream()  Line 630    C++
    XBMC.exe!CDVDPlayer::Process()  Line 941 + 0xb bytes    C++
    XBMC.exe!CThread::Action()  Line 219    C++
    XBMC.exe!CThread::staticThread(void * data)  Line 131   C++
@Voyager1
Team Kodi member

I think I found and fixed the issue. I guess your change just revealed a hidden bug in the CDVDDemuxVobSub destructor, which (incorrectly) uses free() instead of letting the CDemuxStream destructor take care of it the correct way. I commented out the free() statement and problem is fixed:

CDVDDemuxVobsub::~CDVDDemuxVobsub()
{
  for(unsigned i=0;i<m_Streams.size();i++)
  {
    //if(m_Streams[i]->ExtraData)
    //  free(m_Streams[i]->ExtraData);
    delete m_Streams[i];
  }
  m_Streams.clear();
}
...
  virtual ~CDemuxStream()
  {
    delete [] ExtraData;
  }
@FernetMenta
Team Kodi member

great! we missed this one, after having moved deletion of extradata to the destructor: aa279ff

Are you going to send a fix?

@Voyager1
Team Kodi member

I'll be able to do it only later this today (evening). by all means go ahead if you have time sooner...

@Voyager1
Team Kodi member

@FernetMenta - bad news, the crash may be resolved, but vobsubs are still a mess... cycling through the subtitles doesn't work well (seems like the order is incorrect and most of them (if any) don't display... I'm looking into it but let me know if you have any thoughts.

@Voyager1
Team Kodi member

i have reverted the commits one by one and came to the conclusion it's the very first one (FernetMenta@e41959c) that causes this. Still not clear why...

@FernetMenta
Team Kodi member

@Voyager1 could you dropbox me a sample or point me to one. I do hard finding anything for testing.

@Voyager1

I think that this change is wrong (it's actually testing something different than before: whether there's a non-null stream is already created in the map, VS the original test which was just checking bounds). Looking at the code further down (around line 785) confirms the suspicion as we may be working with a stream that yet needs to be added...
This may be the cause of my issue with vobsubs, will test tonight and report back.

Team Kodi member

hard to tell from this line because its not the current code but you are digging in the right place:
b55f479

Team Kodi member

in the current code it is line 649. I'm starting to feel strongly that the second half of the if expression needs to go. It's incorrect, changes logic from prior to this PR, calls Flush and av_free_packet whereas before it did not.

Team Kodi member

I think line 649 is correct. We deal with stream ids we get from ffmpeg and those we preset to player. if we can't find GetStreamInternal something has gone wrong. any changes of ffmpeg streams should be detected by line 639 (IsProgramChange())

Team Kodi member

did you see one of the two log messages of this block in your log?

Team Kodi member

I don't have access to a test PC right now, but will get back to you tonight. Two things
a) my problem is with external vobsubs (idx/sub files) that are added. Stream ids can appear that aren't yet added to the demuxer (see line 771 in new code). and
b) I deducted this by sequentially reverting the 5 commits of the PR, one by one, in reverse order. The problem remained, until I reverted this one. Then I looked at ALL changes in this commit and they all make sense, they perform the same action as before, on a map instead of an array. Except for this one. This one forces you in a block when a stream hasn't been added to the map, whereas before it only did that if the stream ID was greater than MAX_STREAMS (100).
So let's wait for my test.

Team Kodi member

the single commits of this pr do not work standalone. player needs monotonic indices which are not given by this commit we are commenting on. MAX_STREAMS is depreciated, hence we moved to the map. m_stream_index holds the indices presented to player.

Stream ids can appear that aren't yet added to the demuxer

that's all this pr was about. we don't want streams to silently appear/disappear, instead notifying player with a stream change message so it can act upon the change. Maybe we have missed a case in IsProgramChange.

I have found a sample on the web but can't duplicate this issue.

Team Kodi member

agree with the commits needing to work together. I just wanted to highlight how I pinpointed the one that makes the difference. The idx/sub examples I'm having trouble with have several subtitles embedded, and only one or two (as I recall only the first and last one) actually work. All subtitle language names appear in the GUI, but no subtitle overlays appear while playing video (with the exception of the one or two). As I said, I'll do more work on this tonight to come with a definitive analysis.

Team Kodi member

I think I see what you mean: de4a06f

Team Kodi member

Yes, that is what I meant indeed... thanks!

@vicbitter

When I try to play a BluRay ISO, XBMC (on Linux) segfaults with:

0x086716fb in CDVDDemuxFFmpeg::AddStream (this=this@entry=0xaf903928, iId=0, stream=stream@entry=0x9f94f220) at DVDDemuxFFmpeg.cpp:1243
1243 stream->iId = res.first->second->iId;

@FernetMenta
Team Kodi member

thanks, fixed in #2701

@vicbitter

BluRay ISOs are playing again! Many thanks

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment