Better validation is always a good plan.

As I recall, at least some of the code may have originated from libdvbv5. Looking at that upstream repo I see a number of commits about improving robustness over the years.

Does it make any sense to either resync with, or perhaps just depend on, the upstream library (yes, some refactor may be needed)?

By and large I do agree with doing more checks and especially I like optimizing performance by improving buffer allocations.
However, I have a few remarks on this.

1. Part of the motivation mentioned is issue mythfrontend crash when recording is very noisy #596 but this is a divide by zero in new code created by David himself. I think the real motivation is to have zero warning messages by any tool you can think of. That is not necessarily wrong but one should be clear about the reasoning.

2. I really want a separation between non-functional code improvements such as buffer allocations, index checking etc etc and what are functional changes. Functional changes should NOT be hidden in a sea of non-functional changes.
   Putting an incidental functional change in a sea of non-functional code improvement changes has happened before quite often and there is also an example of this in pull request More robust parsing for ATSC PSIP tables (proposal) #624.

Line 626 in mpegtables.cpp, commit fe6d17e, the additional check on DescriptorID, is a functional change. Is this really checked against the relevant standards? Is the IsVideo() check that preceeds it really not good enough to check that this is a video stream? If the IsVideo() not good enough is that not something that also should be fixed? Which stream created the problem for which this change is a fix? Really sure that this does not break anything?

For a functional change I would appreciate a separate commit, with a clear description of what problem is fixed by this commit, possible a stream that shows the problem so that it can be reproduced.

Klaas,

Yes, my motivation is to eliminate bugs in MythTV to make it a better program. We run three well know static analysis tools (clang-tidy, Coverity, and cppcheck) and one lesser known static analysis tool (clazy) in order to point out problems in the code or point out places where the code could be improved. I have taken on the task of trying to fix these warnings (or at least analyze and notate why they shouldn't be fixed) because no-one else seems to have the time. As you pointed out I occasionally make mistakes, but my track record over the last five years is pretty damn good. When I started working on MythTV even a simple "clean" compilation threw several hundred warning messages. That was long before I started looking at the output from the static analysis tools.

My original comment should have referenced issue #589 (not 596), which was due to MPEGStreamData::ProcessTSPacket not properly handling the adaptation field. Yes, I know you blame my C to C++ array changes for causing that issue. They did not. The missing check for an adaptation field and the potential to read past the end of the TS packet appear to have always been present in that function. My change simply prevented reading past the end of the TS packet, and thereby shone a great big glaring spotlight on a preexisting undiscovered bug. I have mostly avoided the video processing code in the past, but that adaptation field problem and the number of Coverity bugs in the mpeg/mheg directories are why I decided to spend time looking at the mpeg and mheg code.

I had already created a separate commit for the functional change in ProgramMapTable::IsStillPicture. To address your questions about that change... I have spent quite a bit of time reading ISO/IEC 13818-1 and ATSC A/65; not quite as much reading SCTE 35 and the DVD Blue Book. (I also have copies of ATSC A/90, ETSI TR 101, and ETSI EN 301, which seem to be the references necessary for eventually investigating MHEG.) I discovered the problem addressed by fe6d17e while creating test cases using PMT packets captured from multiple services. I am using the TSDuck tools to parse these captured streams and produce the 'actual' values used in those test cases. The test case using a PMT packet transmitted by ESPN consistently failed, and I tracked the problem to ProgramMapTable::IsStillPicture. The IsVideo() check in that function is fine. The problem is that the next line assumes that a video stream entry only contains a single descriptor and that that descriptor is always of type video_stream_descriptor(0x02). Nowhere in ISO-IEC 13818-1 do I see that assertion stated. The ESPN PMT packet used to generate the test case has a video stream containing a single descriptor in the user private range (0x86), and I have a trace (but not a test case) from a New Zealand station showing a video stream that contains only a single maximum_bitrate_descriptor(0x14). Yes, I should have written a better message for that individual commit, but I am nowhere close to considering this set of changes ready to commit. I was interested in getting the concept of additional table validation in front of you and a wider audience. That's why the pull request was labeled as a "proposal".

I specifically wanted you to look at these proposed changes because you spend far more time in this part of the code than I do, and probably understand it better than I ever will. I see you're in basic agreement that checking is good, as is optimizing buffer allocations. What about the idea of discarding a malformed table vs processing only the parts of the table that fit within the packet? I think that if any part of a table is determined as corrupt, we shouldn't process any part of that table. I've come to believe that the constructor throwing an error is the correct response to an invalid packet, but there is not a lot of error processing in MythTV that is designed using throw/catch (although there is more than I expected to find when I checked). The other approach would be to set a flag that says this newly allocated data structure is corrupt, shouldn't be used, and should be immediately deleted. There's no memory management when throwing an error because the object is never created. I don't think adding a throw to the code causes any extra processing to the non-throwing case. If the throw is executed you're already into error handling code, and a few extra cycles to find the landing spot for the throw (which should always be the first landing spot) shouldn't cause too much slowdown. Its been a long time since I looked at compiler internals though. What about the idea of splitting out the ATSC/SCTE/DVB tests into their own separate test files, as there are going to be significantly more test cases than before? Or should they all remain in a single file? These are the sorts of questions I was hoping to address by filing this issue.

Gary, thanks for the pointer to libdvbv5. I looked at it and and it doesn't seem to bear that much resemblance to our code.