[FFmpeg-devel] [PATCH 1/2] avformat/mpegts: parse sections with multiple tables

[James Almer]

On 5/9/2018 6:35 PM, Aman Gupta wrote:
> From: Aman Gupta <aman at tmm1.net>
> 
> Fixes PMT parsing in some mpegts streams which contain
> multiple tables within the PMT pid. Previously, the parser
> assumed only one table was present in each packet, and discarded
> the rest of the section data after attempting to parse the first
> table.
> 
> A similar issue was documented in the BeyondTV software[1], which
> helped me diagnose the same bug in the ffmpeg mpegts demuxer. I also
> tried DVBInspector, libdvbpsi's dvbinfo, and tstools' tsinfo to
> help debug. The former two properly read PMTs with multiple tables,
> whereas the last has the same bug as ffmpeg.
> 
> I've created a minimal sample[2] which contains the combined PMT.
> Here's what ffmpeg probe shows before and after this patch:
> 
> Before:
> 
>     Input #0, mpegts, from 'combined-pmt-tids.ts':
>       Duration: 00:00:01.08, start: 4932.966167, bitrate: 741 kb/s
>       Program 1
>       No Program
>         Stream #0:0[0xf9d]: Audio: ac3, 48000 Hz, mono, fltp, 96 kb/s
>         Stream #0:1[0xf9b]: Audio: mp3, 0 channels, fltp
>         Stream #0:2[0xf9c]: Unknown: none
> 
> After:
> 
>     Input #0, mpegts, from 'combined-pmt-tids.ts':
>       Duration: 00:00:01.11, start: 4932.966167, bitrate: 718 kb/s
>       Program 1
>         Stream #0:0[0xf9b]: Video: mpeg2video ([2][0][0][0] / 0x0002), none(tv, top first), 29.97 fps, 29.97 tbr, 90k tbn, 90k tbc
>         Stream #0:1[0xf9c](eng): Audio: ac3 (AC-3 / 0x332D4341), 48000 Hz, 5.1(side), fltp, 384 kb/s
>         Stream #0:2[0xf9d](spa): Audio: ac3 (AC-3 / 0x332D4341), 48000 Hz, mono, fltp, 96 kb/s
> 
> With the patch, the PMT is parsed correctly so the streams are
> created in the correct order, are associated with "Program 1",
> and their codecs are set correctly.
> 
> [1] http://forums.snapstream.com/vb/showpost.php?p=343816&postcount=201
> [2] https://s3.amazonaws.com/tmm1/combined-pmt-tids.ts
> 
> Signed-off-by: Aman Gupta <aman at tmm1.net>
> ---
>  libavformat/mpegts.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> index 0d1dda1c36..3c9f9421cb 100644
> --- a/libavformat/mpegts.c
> +++ b/libavformat/mpegts.c
> @@ -391,7 +391,8 @@ static void write_section_data(MpegTSContext *ts, MpegTSFilter *tss1,
>                                 const uint8_t *buf, int buf_size, int is_start)
>  {
>      MpegTSSectionFilter *tss = &tss1->u.section_filter;
> -    int len;
> +    uint8_t *cur_section_buf = NULL;
> +    int len, offset;
>  
>      if (is_start) {
>          memcpy(tss->section_buf, buf, buf_size);
> @@ -408,23 +409,26 @@ static void write_section_data(MpegTSContext *ts, MpegTSFilter *tss1,
>          tss->section_index += len;
>      }
>  
> +    offset = 0;
> +    cur_section_buf = tss->section_buf;
> +    while (cur_section_buf - tss->section_buf < MAX_SECTION_SIZE && cur_section_buf[0] != 0xff) {
>      /* compute section length if possible */
> -    if (tss->section_h_size == -1 && tss->section_index >= 3) {
> -        len = (AV_RB16(tss->section_buf + 1) & 0xfff) + 3;
> +    if (tss->section_h_size == -1 && tss->section_index - offset >= 3) {
> +        len = (AV_RB16(cur_section_buf + 1) & 0xfff) + 3;
>          if (len > MAX_SECTION_SIZE)
>              return;
>          tss->section_h_size = len;
>      }
>  
>      if (tss->section_h_size != -1 &&
> -        tss->section_index >= tss->section_h_size) {
> +        tss->section_index >= offset + tss->section_h_size) {
>          int crc_valid = 1;
>          tss->end_of_section_reached = 1;
>  
>          if (tss->check_crc) {
> -            crc_valid = !av_crc(av_crc_get_table(AV_CRC_32_IEEE), -1, tss->section_buf, tss->section_h_size);
> +            crc_valid = !av_crc(av_crc_get_table(AV_CRC_32_IEEE), -1, cur_section_buf, tss->section_h_size);
>              if (tss->section_h_size >= 4)
> -                tss->crc = AV_RB32(tss->section_buf + tss->section_h_size - 4);
> +                tss->crc = AV_RB32(cur_section_buf + tss->section_h_size - 4);
>  
>              if (crc_valid) {
>                  ts->crc_validity[ tss1->pid ] = 100;
> @@ -434,10 +438,18 @@ static void write_section_data(MpegTSContext *ts, MpegTSFilter *tss1,
>                  crc_valid = 2;
>          }
>          if (crc_valid) {
> -            tss->section_cb(tss1, tss->section_buf, tss->section_h_size);
> +            tss->section_cb(tss1, cur_section_buf, tss->section_h_size);
>              if (crc_valid != 1)
>                  tss->last_ver = -1;
>          }
> +
> +        cur_section_buf += tss->section_h_size;
> +        offset += tss->section_h_size;
> +        tss->section_h_size = -1;
> +    } else {
> +        tss->end_of_section_reached = 0;
> +        break;
> +    }
>      }
>  }

Valgrind seems to complain about this change (Conditional jump or move
depends on uninitialised value).

http://fate.ffmpeg.org/report.cgi?slot=x86_64-archlinux-gcc-valgrind&time=20180513001958