[confirm] TsReader oddities when dealing with TS files with PCR rollover (1 Viewer)

[asselin]

MediaPortal Version: 1.2.0 Beta
MediaPortal Skin: DefaultWide
Windows Version: Windows 7 + SP1
CPU Type: AMD Athlon 64 X2 4600
HDD: WD 1T + WD 1.5T
Memory: 4G
Motherboard: ASUS
Video Card: ATI Radeon HD 4550
Video Card Driver: 11.5
Sound Card: ATI Radeon HD 4550 (HDMI)
Sound Card AC3: HDMI
Sound Card Driver: 11.5
1. TV Card: HDHomerun HDHR2
1. TV Card Type: ATSC
1. TV Card Driver: 20110323
2. TV Card: HDHomerun HDHR3
2. TV Card Type: ATSC
2. TV Card Driver: 20110323
MPEG2 Video Codec: ffdshow SAF 5.01
MPEG2 Audio Codec: ffdshow SAF 5.01
h.264 Video Codec: ffdshow DXVA SAF 5.01
TV - HTPC Connection: HDMI

First, I'm fairly new to MediaPortal-- I've been using it approx 1-2 months now. Before that, I was using GBPVR. I mention this because when I migrated, I had a large backlog of TS files I recorded OTA using GBPVR that I imported into MediaPortal (by way of ForTheRecord).

Most of the files played fine, but a few exhibited 2 problems:
1. Hit OK on the remote to start playback, and MediaPortal would freeze. HDD light goes solid, and after a while (could be minutes), the file starts playing back.
2. After it starts playing back, if you try to skip forward or backward, the HDD light goes solid again, and then after a while, the recording restarts from the beginning.

Needless to say, spousal approval of MediaPortal took a hit upon running into these.

I did some investigating, and found a clue in TsReader.log that lead me to the problem:

26-06-2011 21:22:59.522 [d2c]Abnormal start PCR, endPcr 118052298, startPcr 8368037531
26-06-2011 21:22:59.544 [d2c]Abnormal start PCR, endPcr 118052298, startPcr 8368037531
26-06-2011 21:22:59.568 [d2c]Abnormal start PCR, endPcr 118052298, startPcr 8368037531
26-06-2011 21:22:59.592 [d2c]Abnormal start PCR, endPcr 118052298, startPcr 8368037531
26-06-2011 21:22:59.616 [d2c]PCR rollover normally found ! endPcr 118052298, startPcr 8368037531

The timestamp of the next entry was always a while after this one. I did some digging, and tracked this down to TsDuration.cpp, where there is a loop to read from the TS file in this situation, and it looks like it's been there forever. This leads to a question that perhaps someone more knowledgeable than I can answer-- when MediaPortal records OTA, does it massage the PCRs to make sure this situation doesn't happen (and thus this code rarely gets used), or is this in fact a problem that folks see on a regular basis?

Going further, I did some research on TS files, PCRs, etc. Since my expertise is now up to about 2 days, I can't say I've got this right, but I * think * the algorithm here is not only annoying to spouses, but also produces incorrect results. As I understand what TsDuration is trying to do, it's attempting to determine the length (duration) of the video file (hence the very appropriate name chosen). That being the case, searching the file for the max PCR and using it in a calculation like END + (MAX - START) doesn't actually yield the correct duration. The correct one, I believe is END + (MAXPCRVALUE - START) where MAXPCRVALUE is a constant -- the maximum possible PCR value (i.e. a PCR with the base and extension of all binary 1s). I argue that the maximum PCR value placed in the TS stream is a sample of the PCR at a particular point in time, but after that PCR value, the counter doesn't immediately roll over to 0-- it keeps incrementing until it hits all 1s, and THEN rolls over to 0. It's just that the all 1s value isn't a sample that gets put into the TS file.

That being the case, the whole loop is unnecessary... just replace the loop with code that sets m_maxPcr to the maximum possible PCR value:

  if (m_endPcr.PcrReferenceBase < m_startPcr.PcrReferenceBase)
  {
    m_maxPcr.PcrReferenceBase = 0x1ffffffffULL;
    m_maxPcr.PcrReferenceExtension = 0x1ffULL;
    m_maxPcr.IsValid = true;
  }

To that end, I've attached TsDuration.simple.cpp.patch and TsDuration.h.patch to implement this simple patch. This seems to entirely fix problem 1.

Making that change doesn't affect problem 2. However, quite by accident, my original attempt to fix this was a little more invasive, and it sort-of fixed problem 2 (see the TsDuration.ver2.cpp.patch). With this patch, fast forward and rewind worked just fine in my test file until about 40 minutes into it, at which point, they stopped working and exhibited a variation of problem 2-- at any point after the 40 minute mark, hitting either would make the video restart from the exact same point about 40 minutes into the video, but the time displayed in the bar would be wrong-- it would show the time that SHOULD be playing (e.g. 48 mins) instead of the time that ACTUALLY was playing (40 mins). I've got to believe this is another manifestation handling PCR rollover in other parts of TsReader, but just haven't had the time to dive deeper yet.

Any feedback on the proposed changes, and whether I'm on the right track would be appreciated. And again, since I'm new here, any pointers on how best to contribute to MediaPortal would be welcome too.

 

[tourettes]

TsReader is not that in good supporting the 3rd party created TS files. There is one main reason why the current approach has been selected (actually the duration code seems to contain some leftovers). DVB broadcasters are known to behave really badly when it comes to the timestamps. There are few kinds of discuntinuities in the stream time that could happen:

• PCR rollover - this is one of the issues you are seeing. This would be possible to be fixed on the TsReader level. This is also not a bug on broadcaster's side, but a valid case.
• Random jump in stream time (backwards or forwards). This happens in many broadcasted streams (randomly). This kind of error cannot be fixed on playback time - duration is not possible to calculate unles the whole TS file would be analysed and the stream timeline would be recostructed.
• "Splice points" (if I remember correctly, been few years since I have looked into TsWriter ). These are discontinuities in the stream timeline - a difference to the previous issue is that broadcaster knows when such is happening and it is setting a flag into the stream to indicate this. Again not possible to fix on playback

TsReader is designed to work with the TS files that are produced by the TsWriter. In that case we can always trust (unless there is a bug in TsWriter) that there aren't PCR rollovers or any discontinuities in the stream timeline. This will allow us just to calculate the duration based on the first and last PCR value. It wouldn't be nice from the performance point of view if the whole TS file needs to be scanned before the playback can be started.

 

[asselin]

tourettes, thanks for the background info; this makes a lot more sense now.

I found the seek problem late last night (didn't have a chance to produce a patch for it since it was late, but I can do that later). The basic problem is in TsFileSeek.cpp, line 299-- the logic in the if() statement is reversed. Going beyond that though, this file has 2 algorithms for seek-- one for when there is PCR rollover and one for when there isn't (binary search). That first algorithm actually isn't needed though; binary search works just fine for both cases (once the if statement is fixed). And of course, the binary search is faster too.

Since it's possible for folks to have TS files in their video library that they haven't gotten from MediaPortal, and PCR rollover is a valid case, and there's already code in there to handle it (albeit buggy code), is it OK if we go ahead and fix it? As a bonus, I can submit the patches to simplify the existing code and actually reduce the number of lines of code.

If so, should I post the patches here, or start a thread in the patches forum (learned about that late last night too )? Would it be better to post 1 "big" patch with the bug fixes plus the cleanup, or is the preference to break it up into bite-size patches for the specific bug fix, removal of unnecessary code, and other cleanup, etc.?

Thanks...

 

[asselin]

Moving this to patch forum

Since this is really a patch submission, I started a thread in the patch forum: [Pending] Fix for TsReader problems with TS files with PCR rollover - MediaPortal Forum. Followup should probably go there...