diff mbox

[FFmpeg-devel] avformat/mpegts: parse large PMTs with multiple tables

Message ID 20180611074331.28844-1-ffmpeg@tmm1.net
State Accepted
Commit cd86c5dbcca5e79c979a6a04da25155ccf17f843
Headers show

Commit Message

Aman Karmani June 11, 2018, 7:43 a.m. UTC 
From: Aman Gupta <aman@tmm1.net>

In 9152c1e4955, the mpegts parser was taught how to parse
PMT sections which contained multiple tables. That commit
fixed parsing of PMT packets from some cable providers,
which included a special SCTE table (0xc0) before the
standard program map table (0x2).

Sometimes, however, the combined 0xc0 and 0x2 tables are
larger than a single TS packet (188 bytes). The mpegts parser
already attempts to parse sections which span multiple packets,
but still assumed that the split section only contained one
table.

This patch fixes parsing of such a sample[1].

Before:

    Input #0, mpegts, from 'combined-pmt-tids-split.ts':
      Duration: 00:00:01.26, start: 39188.931756, bitrate: 597 kb/s
      Program 1
      No Program
        Stream #0:0[0xeff]: Audio: ac3, 48000 Hz, mono, fltp, 64 kb/s
        Stream #0:1[0xefd]: Audio: mp3, 0 channels, fltp
        Stream #0:2[0xefe]: Unknown: none

After:

    Input #0, mpegts, from 'combined-pmt-tids-split.ts':
      Duration: 00:00:01.27, start: 39188.931756, bitrate: 589 kb/s
      Program 1
        Stream #0:0[0xefd]: Video: h264 ([27][0][0][0] / 0x001B), none, 59.94 fps, 59.94 tbr, 90k tbn, 180k tbc
        Stream #0:1[0xefe](eng): Audio: ac3 ([129][0][0][0] / 0x0081), 48000 Hz, stereo, fltp, 384 kb/s
        Stream #0:2[0xeff](spa): Audio: ac3 ([129][0][0][0] / 0x0081), 48000 Hz, mono, fltp, 64 kb/s
        Stream #0:3[0xf00]: Data: scte_35
        Stream #0:4[0xf01]: Unknown: none (ETV1 / 0x31565445)
        Stream #0:5[0xf02]: Unknown: none (ETV1 / 0x31565445)
        Stream #0:6[0xf03]: Unknown: none ([192][0][0][0] / 0x00C0)

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] https://s3.amazonaws.com/tmm1/combined-pmt-tids-split.ts

Signed-off-by: Aman Gupta <aman@tmm1.net>
---
 libavformat/mpegts.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Michael Niedermayer June 12, 2018, 10:17 p.m. UTC | #1 
On Mon, Jun 11, 2018 at 12:43:31AM -0700, Aman Gupta wrote:
> From: Aman Gupta <aman@tmm1.net>
> 
> In 9152c1e4955, the mpegts parser was taught how to parse
> PMT sections which contained multiple tables. That commit
> fixed parsing of PMT packets from some cable providers,
> which included a special SCTE table (0xc0) before the
> standard program map table (0x2).
> 
> Sometimes, however, the combined 0xc0 and 0x2 tables are
> larger than a single TS packet (188 bytes). The mpegts parser
> already attempts to parse sections which span multiple packets,
> but still assumed that the split section only contained one
> table.
> 
> This patch fixes parsing of such a sample[1].
> 
> Before:
> 
>     Input #0, mpegts, from 'combined-pmt-tids-split.ts':
>       Duration: 00:00:01.26, start: 39188.931756, bitrate: 597 kb/s
>       Program 1
>       No Program
>         Stream #0:0[0xeff]: Audio: ac3, 48000 Hz, mono, fltp, 64 kb/s
>         Stream #0:1[0xefd]: Audio: mp3, 0 channels, fltp
>         Stream #0:2[0xefe]: Unknown: none
> 
> After:
> 
>     Input #0, mpegts, from 'combined-pmt-tids-split.ts':
>       Duration: 00:00:01.27, start: 39188.931756, bitrate: 589 kb/s
>       Program 1
>         Stream #0:0[0xefd]: Video: h264 ([27][0][0][0] / 0x001B), none, 59.94 fps, 59.94 tbr, 90k tbn, 180k tbc
>         Stream #0:1[0xefe](eng): Audio: ac3 ([129][0][0][0] / 0x0081), 48000 Hz, stereo, fltp, 384 kb/s
>         Stream #0:2[0xeff](spa): Audio: ac3 ([129][0][0][0] / 0x0081), 48000 Hz, mono, fltp, 64 kb/s
>         Stream #0:3[0xf00]: Data: scte_35
>         Stream #0:4[0xf01]: Unknown: none (ETV1 / 0x31565445)
>         Stream #0:5[0xf02]: Unknown: none (ETV1 / 0x31565445)
>         Stream #0:6[0xf03]: Unknown: none ([192][0][0][0] / 0x00C0)
> 
> 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] https://s3.amazonaws.com/tmm1/combined-pmt-tids-split.ts
> 
> Signed-off-by: Aman Gupta <aman@tmm1.net>
> ---
>  libavformat/mpegts.c | 1 +
>  1 file changed, 1 insertion(+)

with this change section_h_size becomes almost a local variable
is that intended ?

[...]
Aman Karmani June 13, 2018, 4:23 a.m. UTC | #2 
On Tue, Jun 12, 2018 at 3:17 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Mon, Jun 11, 2018 at 12:43:31AM -0700, Aman Gupta wrote:
> > From: Aman Gupta <aman@tmm1.net>
> >
> > In 9152c1e4955, the mpegts parser was taught how to parse
> > PMT sections which contained multiple tables. That commit
> > fixed parsing of PMT packets from some cable providers,
> > which included a special SCTE table (0xc0) before the
> > standard program map table (0x2).
> >
> > Sometimes, however, the combined 0xc0 and 0x2 tables are
> > larger than a single TS packet (188 bytes). The mpegts parser
> > already attempts to parse sections which span multiple packets,
> > but still assumed that the split section only contained one
> > table.
> >
> > This patch fixes parsing of such a sample[1].
> >
> > Before:
> >
> >     Input #0, mpegts, from 'combined-pmt-tids-split.ts':
> >       Duration: 00:00:01.26, start: 39188.931756, bitrate: 597 kb/s
> >       Program 1
> >       No Program
> >         Stream #0:0[0xeff]: Audio: ac3, 48000 Hz, mono, fltp, 64 kb/s
> >         Stream #0:1[0xefd]: Audio: mp3, 0 channels, fltp
> >         Stream #0:2[0xefe]: Unknown: none
> >
> > After:
> >
> >     Input #0, mpegts, from 'combined-pmt-tids-split.ts':
> >       Duration: 00:00:01.27, start: 39188.931756, bitrate: 589 kb/s
> >       Program 1
> >         Stream #0:0[0xefd]: Video: h264 ([27][0][0][0] / 0x001B), none,
> 59.94 fps, 59.94 tbr, 90k tbn, 180k tbc
> >         Stream #0:1[0xefe](eng): Audio: ac3 ([129][0][0][0] / 0x0081),
> 48000 Hz, stereo, fltp, 384 kb/s
> >         Stream #0:2[0xeff](spa): Audio: ac3 ([129][0][0][0] / 0x0081),
> 48000 Hz, mono, fltp, 64 kb/s
> >         Stream #0:3[0xf00]: Data: scte_35
> >         Stream #0:4[0xf01]: Unknown: none (ETV1 / 0x31565445)
> >         Stream #0:5[0xf02]: Unknown: none (ETV1 / 0x31565445)
> >         Stream #0:6[0xf03]: Unknown: none ([192][0][0][0] / 0x00C0)
> >
> > 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] https://s3.amazonaws.com/tmm1/combined-pmt-tids-split.ts
> >
> > Signed-off-by: Aman Gupta <aman@tmm1.net>
> > ---
> >  libavformat/mpegts.c | 1 +
> >  1 file changed, 1 insertion(+)
>
> with this change section_h_size becomes almost a local variable
> is that intended ?


Yes that is the intended behavior. Previously the size was cached as an
optimization, but that assumed that there was only one size value per
section (whereas these sections have multiple tables and thus multiple
sizes).

I will follow up with a refactor to turn it into a local variable and
remove the field from the struct.

Aman


>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The worst form of inequality is to try to make unequal things equal.
> -- Aristotle
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer June 13, 2018, 12:34 p.m. UTC | #3 
On Tue, Jun 12, 2018 at 09:23:25PM -0700, Aman Gupta wrote:
> On Tue, Jun 12, 2018 at 3:17 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Mon, Jun 11, 2018 at 12:43:31AM -0700, Aman Gupta wrote:
> > > From: Aman Gupta <aman@tmm1.net>
> > >
> > > In 9152c1e4955, the mpegts parser was taught how to parse
> > > PMT sections which contained multiple tables. That commit
> > > fixed parsing of PMT packets from some cable providers,
> > > which included a special SCTE table (0xc0) before the
> > > standard program map table (0x2).
> > >
> > > Sometimes, however, the combined 0xc0 and 0x2 tables are
> > > larger than a single TS packet (188 bytes). The mpegts parser
> > > already attempts to parse sections which span multiple packets,
> > > but still assumed that the split section only contained one
> > > table.
> > >
> > > This patch fixes parsing of such a sample[1].
> > >
> > > Before:
> > >
> > >     Input #0, mpegts, from 'combined-pmt-tids-split.ts':
> > >       Duration: 00:00:01.26, start: 39188.931756, bitrate: 597 kb/s
> > >       Program 1
> > >       No Program
> > >         Stream #0:0[0xeff]: Audio: ac3, 48000 Hz, mono, fltp, 64 kb/s
> > >         Stream #0:1[0xefd]: Audio: mp3, 0 channels, fltp
> > >         Stream #0:2[0xefe]: Unknown: none
> > >
> > > After:
> > >
> > >     Input #0, mpegts, from 'combined-pmt-tids-split.ts':
> > >       Duration: 00:00:01.27, start: 39188.931756, bitrate: 589 kb/s
> > >       Program 1
> > >         Stream #0:0[0xefd]: Video: h264 ([27][0][0][0] / 0x001B), none,
> > 59.94 fps, 59.94 tbr, 90k tbn, 180k tbc
> > >         Stream #0:1[0xefe](eng): Audio: ac3 ([129][0][0][0] / 0x0081),
> > 48000 Hz, stereo, fltp, 384 kb/s
> > >         Stream #0:2[0xeff](spa): Audio: ac3 ([129][0][0][0] / 0x0081),
> > 48000 Hz, mono, fltp, 64 kb/s
> > >         Stream #0:3[0xf00]: Data: scte_35
> > >         Stream #0:4[0xf01]: Unknown: none (ETV1 / 0x31565445)
> > >         Stream #0:5[0xf02]: Unknown: none (ETV1 / 0x31565445)
> > >         Stream #0:6[0xf03]: Unknown: none ([192][0][0][0] / 0x00C0)
> > >
> > > 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] https://s3.amazonaws.com/tmm1/combined-pmt-tids-split.ts
> > >
> > > Signed-off-by: Aman Gupta <aman@tmm1.net>
> > > ---
> > >  libavformat/mpegts.c | 1 +
> > >  1 file changed, 1 insertion(+)
> >
> > with this change section_h_size becomes almost a local variable
> > is that intended ?
> 
> 
> Yes that is the intended behavior. Previously the size was cached as an
> optimization, but that assumed that there was only one size value per
> section (whereas these sections have multiple tables and thus multiple
> sizes).
> 
> I will follow up with a refactor to turn it into a local variable and
> remove the field from the struct.

ok, perfectly fine

thx

[...]
diff mbox

Patch

diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
index 5f53f77d89..a5cb17ac16 100644
--- a/libavformat/mpegts.c
+++ b/libavformat/mpegts.c
@@ -465,6 +465,7 @@  static void write_section_data(MpegTSContext *ts, MpegTSFilter *tss1,
             offset += tss->section_h_size;
             tss->section_h_size = -1;
         } else {
+            tss->section_h_size = -1;
             tss->end_of_section_reached = 0;
             break;
         }