Opened 13 years ago

Closed 13 years ago

#2147 closed defect (fixed)

pes assembler

Reported by: Janne <janne-mythtv@…> Owned by: Stuart Auchterlonie
Priority: minor Milestone: 0.21
Component: dvb Version: head
Severity: medium Keywords:
Cc: Ticket locked: no

Description

Attached patch addresses following issues:

Ensure that _psiOffset + 3 + 1 is already read before parsing a pes packet header (mainly calling Length()). This fixes several issues e.g. EIT events with a starttime in 2038, segfault int PAT parsing or "program number not found in PAT" errors.

Fixes pesdata calculation in InitPESPacket for PESPacket::View().

Removes a useless statement from AssemblePSIP().

Attachments (7)

pes_assembler_fixes.diff (4.1 KB) - added by Janne <janne-mythtv@…> 13 years ago.
pespacket_assembler_debug.diff (762 bytes) - added by Janne <janne-mythtv@…> 13 years ago.
2147_fix.diff (2.8 KB) - added by Janne <janne-mythtv@…> 13 years ago.
drop_stuffing_tables.diff (636 bytes) - added by Janne <janne-mythtv@…> 13 years ago.
gdb.txt (187.9 KB) - added by petri@… 13 years ago.
gdb2.txt (31.6 KB) - added by petri@… 13 years ago.
myth.log (90.6 KB) - added by petri@… 13 years ago.

Download all attachments as: .zip

Change History (35)

Changed 13 years ago by Janne <janne-mythtv@…>

Attachment: pes_assembler_fixes.diff added

comment:1 Changed 13 years ago by danielk

Owner: changed from Stuart Auchterlonie to danielk

comment:2 Changed 13 years ago by anonymous

Issue not entirely solved. valgrind reports following error

==31579== Conditional jump or move depends on uninitialised value(s)
==31579==    at 0x52BF80F: NetworkInformationTable::Parse() const (dvbtables.cpp:16)
==31579==    by 0x52EF440: NetworkInformationTable::NetworkInformationTable(PSIPTable const&) (dvbtables.h:30)
==31579==    by 0x52E75AA: DVBStreamData::HandleTables(unsigned, PSIPTable const&) (dvbstreamdata.cpp:210)
==31579==    by 0x52C8581: MPEGStreamData::HandleTSTables(TSPacket const*) (mpegstreamdata.cpp:684)
==31579==    by 0x52C1C51: MPEGStreamData::ProcessTSPacket(TSPacket const&) (mpegstreamdata.cpp:722)
==31579==    by 0x52C1D6F: MPEGStreamData::ProcessData(unsigned char*, int) (mpegstreamdata.cpp:707)
==31579==    by 0x5580FB3: DVBSignalMonitor::RunTableMonitorTS() (dvbsignalmonitor.cpp:386)
==31579==    by 0x558175F: DVBSignalMonitor::RunTableMonitor() (dvbsignalmonitor.cpp:563)
==31579==    by 0x558177E: DVBSignalMonitor::TableMonitorThread(void*) (dvbsignalmonitor.cpp:173)
==31579==    by 0x88F3019: start_thread (pthread_create.c:261)
==31579==    by 0x8F1C482: clone (in /lib64/libc-2.3.6.so)

and I see sometimes a bunch of "AddTSPacket: Out of sync!!! Need to wait for next payloadStart PID: 0x12, continuity counter: 6 (expected 5)."

The continuity counter is always expected+1.

comment:3 in reply to:  2 Changed 13 years ago by Stuart Auchterlonie

Milestone: 0.20

Replying to anonymous:

and I see sometimes a bunch of "AddTSPacket: Out of sync!!! Need to wait for next payloadStart PID: 0x12, continuity counter: 6 (expected 5)."

The continuity counter is always expected+1.

This would be a symptom of a occasional signal glitch and this is the point of the CC. The CC should cycle through 0-15 and if a TS packet is corrupted it'll be dropped leading to a discontinuity in the CC. This gives you the information needed to drop that PES packet due to bad signal or intermittent error.

comment:4 Changed 13 years ago by danielk

Resolution: fixed
Status: newclosed

(In [10734]) Fixes #2147. Various PES packet assembly fixes.

This prevents printing spurious warnings with the pespacket.cpp changes, fixes an offset problem in pespacket.h affecting DVB standard PSIP, checks the PES packet size before tring to verify it in AssemblePSIP, and also checks the PES packet size when it is supposed to be complete so that one scrambled packet can't disrupt the decoding of the next packet on the same PID.

comment:5 Changed 13 years ago by Janne <janne-mythtv@…>

Resolution: fixed
Status: closedreopened
Type: patchdefect

this isn't fixed.

#2273, #2274 are duplicates of this.

Changed 13 years ago by Janne <janne-mythtv@…>

comment:6 Changed 13 years ago by Janne <janne-mythtv@…>

valgrind will only detect incomplete packets as uninitialized data if we disable the pes allocator.

So run the backend under valgrind only with pespacket_assembler_debug.diff applied.

comment:7 Changed 13 years ago by danielk

Owner: changed from danielk to Stuart Auchterlonie
Status: reopenednew

Stuart I hate to offload this on you, but if someone has a safe fix before 0.20...

BTW It's also possible that the problem is not with the packet assembler. We may not be checking the CRC on every SI table because we may be checking the PES CRC flag which isn't valid for some of the tables... Or, the users reporting problems may be using one of the cards blacklisted for CRC checking on PAT/PMT tables due to broken packet rewriting hardware.

This is not a showstopper bug. It would simply be nice to fix before the release if the fix doesn't risk breaking anything. Feel free to reassign to me if this doesn't get fixed before release.

comment:8 Changed 13 years ago by petri@…

This is really showstopper bug for me every day. Backend runs max couble hours.

comment:9 Changed 13 years ago by otto at kolsi dot fi

I've also started to have problems that crash the backend (1-2 times / day). Based on the logs, I believe it is this same problem (logs show PES packet related errors). If needed, I can get logs and backtraces and/or test patches.

comment:10 Changed 13 years ago by anonymous

Severity: mediumhigh

looks like same problem according to the logs... PES packet related error... backend usualy dead after 2-3 hours of live tv....

comment:11 Changed 13 years ago by anonymous

Severity: highmedium

comment:12 Changed 13 years ago by Colin Guthrie <mythtv@…>

Has to be said, I agree with the high severity. I'd consider myself luck if I get 2-3 hours before my backend dies as a result of this bug, certainly I'd say it's blocker for 0.20. Let me know if I can help. I have BT's but I don't think they look useful as to the root cause :(

comment:13 Changed 13 years ago by Stuart Auchterlonie

Priority: minormajor

We need valgrind logs of the backend built with Janne's pespacket_assembler_debug.diff to try to identify the cause of this.

The valgrind command line to use was recently pasted to the dev list

Changed 13 years ago by Janne <janne-mythtv@…>

Attachment: 2147_fix.diff added

comment:14 Changed 13 years ago by Janne <janne-mythtv@…>

Type: defectpatch

2147_fix.diff hopefully solves all the issues (#2271, #2273, #2274).

We know in AddTSPacket() if the PES packet is broken. With this patch we use that knowledge in AssemblePSIP() to discard broken packets.

The patch also clarifies the meaning of _pesdataSize.

Changed 13 years ago by Janne <janne-mythtv@…>

Attachment: drop_stuffing_tables.diff added

comment:15 Changed 13 years ago by Janne <janne-mythtv@…>

added patch to drop stuffing packet before the CRC check in HandleTSTables().

There is another one with -v siparser. That can't easily removed. The main problem is that we handle DVB SI tables as PES packets which is incorrect.

comment:16 Changed 13 years ago by Colin Guthrie <mythtv@…>

Tested last night - applied both patches. No crashes when recording two long movies. While I can't be sure, it's certainly seems to work for me :)

comment:17 Changed 13 years ago by Stuart Auchterlonie

(In [11072]) Refs #2147. Discard broken pes packets by providing feedback from AddTSPacket. Allows AssemblePSIP to discard broken packets.

Also clarifies the meaning of _pesdataSize.

Thanks to Janne for the patch, Mark, Colin & Otto for testing.

comment:18 Changed 13 years ago by Stuart Auchterlonie

(In [11073]) Refs #2147. Drop stuffing packets as they are not required.

Thanks to Janne for the patch, Mark, Colin & Otto for testing...

comment:19 Changed 13 years ago by otto at kolsi dot fi

Tested the patches for one night, so far everything is working fine!

comment:20 Changed 13 years ago by Stuart Auchterlonie

Milestone: 0.200.21
Priority: majorminor

There's stlll a small issue left, but it looks like the majority of problems have been fixed.

Excerpt from Mark B's valgrind logs.

==8502== Use of uninitialised value of size 4
==8502==    at 0x456BCAE: MPEGDescriptor::DescriptorLength() const (mpegdescriptors.h:145)
==8502==    by 0x45ABED2: MPEGDescriptor::Parse(unsigned char const*, unsigned)
(mpegdescriptors.cpp:18)
==8502==    by 0x47CD2ED: EITHelper::AddEIT(DVBEventInformationTable const*) (eithelper.cpp:243)
==8502==    by 0x45A09FC: DVBStreamData::HandleTables(unsigned, PSIPTable const&) (dvbstreamdata.cpp:290)
==8502==    by 0x457FE80: MPEGStreamData::HandleTSTables(TSPacket const*) (mpegstreamdata.cpp:701)
==8502==    by 0x4578BDA: MPEGStreamData::ProcessTSPacket(TSPacket const&) (mpegstreamdata.cpp:739)
==8502==    by 0x4578CF2: MPEGStreamData::ProcessData(unsigned char*, int) (mpegstreamdata.cpp:724)
==8502==    by 0x48AE23E: DVBSignalMonitor::RunTableMonitorTS() (dvbsignalmonitor.cpp:386)
==8502==    by 0x48AEACE: DVBSignalMonitor::RunTableMonitor() (dvbsignalmonitor.cpp:563)
==8502==    by 0x48AEAF8: DVBSignalMonitor::TableMonitorThread(void*) (dvbsignalmonitor.cpp:173)
==8502==    by 0x608807C: start_thread (in /lib/tls/libpthread-2.3.6.so)
==8502==    by 0x62728FD: clone (in /lib/tls/libc-2.3.6.so)
==8502==

comment:21 Changed 13 years ago by Stuart Auchterlonie

Status: newassigned
Type: patchdefect

comment:22 Changed 13 years ago by petri@…

Hi, Backend still stops :(

myth.log 2006-09-12 07:09:34.660 TVRec(1): Got good signal 2006-09-12 07:09:34.664 TVRec(1): ClearFlags?(WaitingForSignal?,) -> RunMainLoop?,AskAllowRecording?,SignalMonitorRunning?,EITScannerRunning, 2006-09-12 07:09:34.700 DVBSM(0)::AddPIDFilter(0x12): 2006-09-12 07:09:35.122 DTVSM(0)::SetNIT(): net_id = 100 2006-09-12 07:09:35.123 SM(0)::AddFlags?: Seen(NIT,) Match() Wait() 2006-09-12 07:09:35.124 DTVSM(0)::SetNIT(): net_id = 100 2006-09-12 07:09:35.126 SM(0)::AddFlags?: Seen(NIT,) Match() Wait() 2006-09-12 07:09:36.166 match[0]: 0 vs. 2006-09-12 07:09:36.618 match[0]: 0 vs. 2006-09-12 07:10:01.942 PESPacket: Failed CRC check 0x0 != 0xbc12eeb4 for StreamID = 0x0 2006-09-12 07:10:01.948 PESPacket: Failed CRC check 0x0 != 0xbc12eeb4 for StreamID = 0x0 2006-09-12 07:10:01.949 PESPacket: Failed CRC check 0x0 != 0xbc12eeb4 for StreamID = 0x0 2006-09-12 07:10:01.951 PSIP packet failed CRC check. pid(0x0) type(0x0) 2006-09-12 07:10:01.954 SM(1)::AddFlags?: Seen(PAT,) Match() Wait() 2006-09-12 07:10:01.956 SM(1)::AddFlags?: Seen() Match(PAT,) Wait() 2006-09-12 07:10:01.957 CreatePATSingleProgram() 2006-09-12 07:10:01.958 PAT in input stream

gdb.txt is attached.

Changed 13 years ago by petri@…

Attachment: gdb.txt added

comment:23 in reply to:  22 Changed 13 years ago by Stuart Auchterlonie

Replying to petri@heinala.fi:

Hi, Backend still stops :(

gdb.txt is attached.

you've managed to delete the few crucial lines where gdb tells us how and where it crashed. Please compress the gdb.txt file and re-upload it.

Changed 13 years ago by petri@…

Attachment: gdb2.txt added

Changed 13 years ago by petri@…

Attachment: myth.log added

comment:24 Changed 13 years ago by petri@…

Sorry! Hope that new file helps!

comment:25 in reply to:  24 Changed 13 years ago by Janne <janne-mythtv@…>

that error is known. you can work around it by running mythbackend with the default verbose options.

comment:26 Changed 13 years ago by Stuart Auchterlonie

(In [11334]) When we detect a discontinuity in our TS packet via the continuity counter, throw away the TS packet.

It's the next level up above the PES packets, but is similar to the pespacket mishandling we've had before.

Refs #2147 because of this.

comment:27 Changed 13 years ago by danielk

(In [12383]) Fixes #2858. Refs #2147. Reverts [11334], this threw away a possibly good TS packet if it followed a lost TS packet.

comment:28 Changed 13 years ago by Janne Grunau

Resolution: fixed
Status: assignedclosed

probably fixed by [12480]

Note: See TracTickets for help on using tickets.