[FFmpeg-devel] New patch for mpegts.c

[JULIAN GARDNER]

----- Original Message -----
> From: Michael Niedermayer <michaelni at gmx.at>
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Cc: 
> Sent: Tuesday, 16 October 2012, 15:05
> Subject: Re: [FFmpeg-devel] New patch for mpegts.c
> 
> On Tue, Oct 16, 2012 at 01:31:35PM +0100, JULIAN GARDNER wrote:
>> 
>> 
>>  Whilst working on some new additions to MPEGTS support i found that the 
> current code did not take into account the "Table Version Number", 
> which caused FFMPEG to process every table that was in the TS. After adding the 
> new code and validating against a 3 minute TS the counts on tables processed 
> went down from 1360 to 3.
> 
> can you provide a testcase that shows the improvment?

Remove the check against the version number and run the code with debug and you will see hundreds of messags saying it has processed the table, but with the same version number, put the check back in and rerun and you will 3 messages, well that is it the version number does not change

> 
> [...]
>>  diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
>>  index 33a0eaf..de06a68 100644
>>  --- a/libavformat/mpegts.c
>>  +++ b/libavformat/mpegts.c
>>  @@ -58,7 +58,18 @@ typedef struct MpegTSPESFilter {
>>       void *opaque;
>>   } MpegTSPESFilter;
>>   
>>  -typedef void SectionCallback(MpegTSFilter *f, const uint8_t *buf, int 
> len);
>>  +typedef struct SectionHeader {
>>  +    uint8_t tid;
>>  +    uint16_t id;
>>  +    uint8_t version;
>>  +    uint8_t sec_num;
>>  +    uint8_t last_sec_num;
>>  +} SectionHeader;
> 
> moving of code if its needed, belongs in a seperate patch
> 
Ok so i need to send in multiple patches, as this move is NEEDED
 
>>  +
>>  +static int parse_section_header(SectionHeader *h,
>>  +                                const uint8_t **pp, const uint8_t *p_end);
>>  +
>>  +typedef void SectionCallback(MpegTSFilter *f, const uint8_t *buf, const 
> uint8_t *buf_end, SectionHeader *h);
>>   
>>   typedef void SetServiceCallback(void *opaque, int ret);
>>   
>>  @@ -76,11 +87,14 @@ struct MpegTSFilter {
>>       int pid;
>>       int es_id;
>>       int last_cc; /* last cc code (-1 if first packet) */
>>  +    uint8_t last_version;
>>       enum MpegTSFilterType type;
>>       union {
>>           MpegTSPESFilter pes_filter;
>>           MpegTSSectionFilter section_filter;
>>       } u;
>>  +    const char *name;
>>  +    uint8_t tid;
>>   };
>>   
>>   #define MAX_PIDS_PER_PROGRAM 64
>>  @@ -318,18 +332,45 @@ static void write_section_data(AVFormatContext *s, 
> MpegTSFilter *tss1,
>>               }else
>>                   crc_valid = 2;
>>           }
>>  -        if (crc_valid)
>>  -            tss->section_cb(tss1, tss->section_buf, 
> tss->section_h_size);
>>  +        if (crc_valid) {
>>  +            const uint8_t *p_end;
>>  +            const uint8_t *p;
>>  +            SectionHeader h;
>>  +
>>  +            av_dlog(ts->stream, "%s: len %i\n", 
> tss1->name, tss->section_h_size);
>>  +            hex_dump_debug(ts->stream, tss->section_buf, 
> tss->section_h_size);
>>  +
>>  +            p = tss->section_buf;
>>  +            p_end = p + tss->section_h_size - 4;
>>  +
>>  +            if (parse_section_header(&h, &p, p_end) < 0)
>>  +                return;
>>  +
>>  +            if (h.tid != tss1->tid)
>>  +                return;
> 
> changing tid and parse_section_header handling belongs to 2 seperate
> patches (and why are these changes needed at all?)
> 
I have moved similar code that is used in all callbacks so it is done in this routine, if we go one to add more tables this will function for them as well.

> 
>>  +
>>  +            // Check Version Numbers
>>  +            if (tss1->last_version==h.version)
>>  +                return;
>>  +            tss1->last_version = h.version;
> 
> the last_version should only be updated after successfull parsing of
> the data

There is no error return from the section callback, so there is no way to know if it is a valid decode!

> also how does this interact when things are split into multiple
> sections?
> 

Well as the original code DOES NOT do anything about multiple sections, this and the unpatched code will FAIL. I will look into adding the extra code to process multiple tables. In my view the PAT/PMT will really only evey have 1 section, but it is possible that the SDT could be split over multiple ones. I will find a test stream with this and add code to process this, but this will change this decoder fundamentally.

joolz