[FFmpeg-devel] [PATCH 2/4] V9 - SCTE-35 extraction from mpegts

[Marton Balint]

Hi,

Sorry for the delay. Below some comments.

On Thu, 1 Sep 2016, Carlos Fernandez Sanz wrote:

> From: Carlos Fernandez <carlos at ccextractor.org>
>
> Signed-off-by: Carlos Fernandez <carlos at ccextractor.org>
> ---
> libavformat/mpegts.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> index b31d233..d000269 100644
> --- a/libavformat/mpegts.c
> +++ b/libavformat/mpegts.c
> @@ -57,6 +57,7 @@ enum MpegTSFilterType {
>     MPEGTS_PES,
>     MPEGTS_SECTION,
>     MPEGTS_PCR,
> +    MPEGTS_DATA,

Do you really need a new data type? Can't you use MPEGTS_SECTION, and make 
use of the existing infrastructure already in place for parsing sections? 
CRC checking, and stuff is already in place, isn't that worth using? Or I 
misunderstand something, and SCTE sections don't fit here?

> };
> 
> typedef struct MpegTSFilter MpegTSFilter;
> @@ -510,6 +511,11 @@ static MpegTSFilter *mpegts_open_pcr_filter(MpegTSContext *ts, unsigned int pid)
>     return mpegts_open_filter(ts, pid, MPEGTS_PCR);
> }
> 
> +static MpegTSFilter *mpegts_open_data_filter(MpegTSContext *ts, unsigned int pid)
> +{
> +    return mpegts_open_filter(ts, pid, MPEGTS_DATA);
> +}
> +
> static void mpegts_close_filter(MpegTSContext *ts, MpegTSFilter *filter)
> {
>     int pid;
> @@ -725,6 +731,12 @@ static const StreamType HDMV_types[] = {
>     { 0 },
> };
> 
> +/* SCTE types */
> +static const StreamType SCTE_types[] = {
> +    { 0x86, AVMEDIA_TYPE_DATA,  AV_CODEC_ID_SCTE_35    },
> +    { 0 },
> +};
> +
> /* ATSC ? */
> static const StreamType MISC_types[] = {
>     { 0x81, AVMEDIA_TYPE_AUDIO, AV_CODEC_ID_AC3 },
> @@ -872,6 +884,13 @@ static void reset_pes_packet_state(PESContext *pes)
>     av_buffer_unref(&pes->buffer);
> }
> 
> +static void new_data_packet(const uint8_t *buffer, int len, AVPacket *pkt)
> +{
> +    av_init_packet(pkt);
> +    pkt->data = buffer;
> +    pkt->size = len;
> +}
> +
> static int new_pes_packet(PESContext *pes, AVPacket *pkt)
> {
>     char *sd;
> @@ -1975,6 +1994,19 @@ static void pmt_cb(MpegTSFilter *filter, const uint8_t *section, int section_len
>                 pes->st->id = pes->pid;
>             }
>             st = pes->st;
> +        } else if (stream_type == 0x86 && prog_reg_desc ==  AV_RL32("CUEI")) {
> +            int idx = ff_find_stream_index(ts->stream, pid);
> +            if (idx >= 0) {
> +                st = ts->stream->streams[idx];
> +            } else {
> +                st = avformat_new_stream(ts->stream, NULL);
> +                if (!st)
> +                    goto out;
> +                st->id = pid;
> +                st->codecpar->codec_type = AVMEDIA_TYPE_DATA;
> +                mpegts_find_stream_type(st, stream_type, SCTE_types);
> +                mpegts_open_data_filter(ts, pid);
> +            }

I'd factor this part into the generic data part, something like:

+static int is_pes_stream(int stream_type, uint32_t prog_reg_desc)
+{
+    return !(stream_type == 0x13 ||
+             stream_type == 0x86 && prog_reg_desc == AV_RL32("CUEI"));
+}
+
  static void pmt_cb(MpegTSFilter *filter, const uint8_t *section, int section_len)
  {
      MpegTSContext *ts = filter->u.section_filter.opaque;
@@ -1975,7 +2000,7 @@ static void pmt_cb(MpegTSFilter *filter, const uint8_t *section, int section_len
                  pes->st->id = pes->pid;
              }
              st = pes->st;
-        } else if (stream_type != 0x13) {
+        } else if (is_pes_stream(stream_type, prog_reg_desc)) {
              if (ts->pids[pid])
                  mpegts_close_filter(ts, ts->pids[pid]); // wrongly added sdt filter probably
              pes = add_pes_stream(ts, pid, pcr_pid);
@@ -1995,6 +2020,10 @@ static void pmt_cb(MpegTSFilter *filter, const uint8_t *section, int section_len
                      goto out;
                  st->id = pid;
                  st->codecpar->codec_type = AVMEDIA_TYPE_DATA;
+                if (stream_type == 0x86 && prog_reg_desc == AV_RL32("CUEI")) {
+                    mpegts_find_stream_type(st, stream_type, SCTE_types);
+                    mpegts_open_data_filter(ts, pid);
+                }
              }
          }

>         } else if (stream_type != 0x13) {
>             if (ts->pids[pid])
>                 mpegts_close_filter(ts, ts->pids[pid]); // wrongly added sdt filter probably
> @@ -2317,7 +2349,21 @@ static int handle_packet(MpegTSContext *ts, const uint8_t *packet)
>                 }
>             }
>         }
> -
> +    } else if (tss->type == MPEGTS_DATA) {
> +        AVProgram *prg = NULL;
> +        int idx = ff_find_stream_index(ts->stream, pid);
> +        p++;

What do you skip here exactly? Isn't it worth keeping it if MPEGTS_DATA 
type can be used for something other than SCTE?

> +        new_data_packet(p,p_end - p, ts->pkt);
> +        if (idx >= 0) {
> +            ts->pkt->stream_index = idx;
> +        }
> +        prg = av_find_program_from_stream(ts->stream, prg, idx);

Isn't prg always NULL in the parameters? If it is, then simply use a NULL 
constant.

> +        if (prg->pcr_pid != -1 && prg->discard != AVDISCARD_ALL) {

You might check if prg is NULL here before dereferencing it.

> +             MpegTSFilter *f = ts->pids[prg->pcr_pid];
> +             if(f)
> +                 ts->pkt->pts = ts->pkt->dts = f->last_pcr/300;
> +        }
> +        ts->stop_parse = 1;
>     } else {
>         int ret;
>         // Note: The position here points actually behind the current packet.
> @@ -2730,6 +2776,8 @@ static int mpegts_read_packet(AVFormatContext *s, AVPacket *pkt)
>                     ret = 0;
>                     break;
>                 }
> +            } else if (ts->pids[i] && ts->pids[i]->type == MPEGTS_DATA) {
> +                return ret;

Why do you need this? I think you break here flushing existing PES 
buffers at end of files for PIDs bigger than SCTE pids.

Regards,
Marton