[FFmpeg-devel] [PATCH 2/2] v4 - SCTE-35 support in hlsenc

Michael Niedermayer michael at niedermayer.cc
Wed Aug 10 00:29:53 EEST 2016


On Mon, Aug 08, 2016 at 11:45:38AM -0700, Carlos Fernandez Sanz wrote:

[...]
> @@ -349,11 +359,25 @@ static int hls_append_segment(struct AVFormatContext *s, HLSContext *hls, double
>      else
>          en->sub_filename[0] = '\0';
>  
> -    en->duration = duration;
> -    en->pos      = pos;
> -    en->size     = size;
> +    en->duration   = duration;
> +    en->pos        = pos;
> +    en->event      = event;
> +    en->size       = size;
> +    en->start_pts  = start_pts;
>      en->next     = NULL;

this reindention is inconsistent, also you can if you like keep
reindentions in a seperate patch. But if done they should be consistent


>  
> +    if (hls->scte_iface) {
> +        if (hls->scte_iface->event_out == EVENT_OUT_CONT) {
> +            en->adv_count = hls->scte_iface->adv_count;
> +            hls->scte_iface->adv_count++;
> +            en->out = hls->scte_iface->event_out;
> +        } else {
> +            hls->scte_iface->adv_count = 0;
> +            en->out = hls->scte_iface->event_out;
> +        }
> +    }
> +
> +
>      if (hls->key_info_file) {
>          av_strlcpy(en->key_uri, hls->key_uri, sizeof(en->key_uri));
>          av_strlcpy(en->iv_string, hls->iv_string, sizeof(en->iv_string));
> @@ -475,9 +499,23 @@ static int hls_window(AVFormatContext *s, int last)
>          if (hls->flags & HLS_SINGLE_FILE)
>               avio_printf(out, "#EXT-X-BYTERANGE:%"PRIi64"@%"PRIi64"\n",
>                           en->size, en->pos);
> -        if (hls->baseurl)
> -            avio_printf(out, "%s", hls->baseurl);
> -        avio_printf(out, "%s\n", en->filename);
> +        if (hls->scte_iface && (en->event || en->out) ) {
> +            char *str;
> +            char fname[1024] = "";
> +            if (hls->adv_filename) {
> +                str = hls->scte_iface->get_hls_string(hls->scte_iface, en->event, hls->adv_filename, en->out, en->adv_count, en->start_pts);
> +            } else {
> +                if (hls->baseurl)

> +                    strncat(fname, hls->baseurl, 1024);
> +                strncat(fname, en->filename, 1024);

duplicate hardcoded sizes, these should use sizeof()


> +                str = hls->scte_iface->get_hls_string(hls->scte_iface, en->event, fname, en->out, -1, en->start_pts);
> +            }
> +            avio_printf(out, "%s", str);
> +        } else {
> +            if (hls->baseurl)
> +                avio_printf(out, "%s", hls->baseurl);
> +            avio_printf(out, "%s\n", en->filename);
> +        }
>      }
>  
>      if (last && (hls->flags & HLS_OMIT_ENDLIST)==0)
> @@ -502,9 +540,15 @@ static int hls_window(AVFormatContext *s, int last)
>              if (hls->flags & HLS_SINGLE_FILE)
>                   avio_printf(sub_out, "#EXT-X-BYTERANGE:%"PRIi64"@%"PRIi64"\n",
>                           en->size, en->pos);
> -            if (hls->baseurl)
> -                avio_printf(sub_out, "%s", hls->baseurl);
> -            avio_printf(sub_out, "%s\n", en->sub_filename);
> +            if (hls->scte_iface && (en->event || en->out) ) {
> +                char *str = hls->scte_iface->get_hls_string(hls->scte_iface, en->event, hls->adv_subfilename, en->out, en->adv_count, en->pos);
> +                avio_printf(out, "%s", str);
> +            } else {
> +                if (hls->baseurl)
> +                    avio_printf(out, "%s", hls->baseurl);
> +                avio_printf(sub_out, "%s\n", en->sub_filename);
> +            }
> +
>          }
>  
>          if (last)
> @@ -647,6 +691,7 @@ static int hls_write_header(AVFormatContext *s)
>      AVDictionary *options = NULL;
>      int basename_size;
>      int vtt_basename_size;
> +    int ts_index = 0;
>  
>      hls->sequence       = hls->start_sequence;
>      hls->recording_time = hls->time * AV_TIME_BASE;
> @@ -763,19 +808,21 @@ static int hls_write_header(AVFormatContext *s)
>          goto fail;
>      }
>      //av_assert0(s->nb_streams == hls->avf->nb_streams);
> -    for (i = 0; i < s->nb_streams; i++) {
> +    for (ts_index = 0, i = 0; i < s->nb_streams; i++) {
>          AVStream *inner_st;
>          AVStream *outer_st = s->streams[i];
> -        if (outer_st->codecpar->codec_type != AVMEDIA_TYPE_SUBTITLE)
> -            inner_st = hls->avf->streams[i];
> -        else if (hls->vtt_avf)
> +        if (hls->vtt_avf && outer_st->codecpar->codec_type == AVMEDIA_TYPE_SUBTITLE) {
>              inner_st = hls->vtt_avf->streams[0];
> -        else {
> -            /* We have a subtitle stream, when the user does not want one */
> -            inner_st = NULL;
> +            avpriv_set_pts_info(outer_st, inner_st->pts_wrap_bits, inner_st->time_base.num, inner_st->time_base.den);
> +        } else if (outer_st->codecpar->codec_type == AVMEDIA_TYPE_DATA) {
> +            inner_st = hls->avf->streams[ts_index];

> +            hls->scte_iface = ff_alloc_scte35_parser(hls, outer_st->time_base);

missing failure check


[...]
> +static char* get_hls_string(struct scte_35_interface *iface, struct scte_35_event *event,
> +                 const char *filename, int out_state, int seg_count, int64_t pos)
> +{
> +    int ret;
> +    av_bprint_clear(&iface->avbstr);
> +    if (out_state == EVENT_IN ) {
> +        av_bprintf(&iface->avbstr, "#EXT-OATCLS-SCTE35:%s\n", iface->pkt_base64);
> +        av_bprintf(&iface->avbstr, "#EXT-X-CUE-IN\n");
> +        av_bprintf(&iface->avbstr, "#EXT-X-DISCONTINUITY\n");
> +    } else if (out_state == EVENT_OUT) {
> +        if (event)
> +        {

inconsistent { placement


> +            av_bprintf(&iface->avbstr, "#EXT-OATCLS-SCTE35:%s\n", iface->pkt_base64);
> +            if(event->duration != AV_NOPTS_VALUE) {
> +                int64_t dur = ceil(((double)event->duration * iface->timebase.num) /iface->timebase.den);
> +                av_bprintf(&iface->avbstr, "#EXT-X-CUE-OUT:%"PRIu64"\n", dur);
> +            } else {
> +                av_bprintf(&iface->avbstr, "#EXT-X-CUE-OUT\n");
> +            }
> +            av_bprintf(&iface->avbstr, "#EXT-X-DISCONTINUITY\n");
> +        }
> +    } else if (out_state == EVENT_OUT_CONT) {
> +        if(event && event->duration != AV_NOPTS_VALUE) {
> +            int64_t dur = ceil(((double)event->duration * iface->timebase.num) /iface->timebase.den);
> +            int64_t elapsed_time = ceil(((double)pos * iface->timebase.num) /iface->timebase.den) - event->out_pts;
> +            av_bprintf(&iface->avbstr, "#EXT-X-CUE-OUT-CONT:ElapsedTime=%"PRIu64",Duration=%"PRIu64",SCTE35=%s\n",
> +                elapsed_time,  dur, iface->pkt_base64);
> +        } else {
> +            av_bprintf(&iface->avbstr, "#EXT-X-CUE-OUT-CONT:SCTE35=%s\n", iface->pkt_base64);
> +        }
> +    }
> +    if (seg_count >= 0)
> +        av_bprintf(&iface->avbstr, filename, seg_count);
> +    else
> +        av_bprintf(&iface->avbstr, "%s", filename);
> +    av_bprintf(&iface->avbstr, "\n");
> +
> +    ret = av_bprint_is_complete(&iface->avbstr);
> +    if( ret == 0) {

> +        av_log(NULL, AV_LOG_DEBUG, "Out of Memory");

Missing log context, a log context is important so the user or app
know from where a message originated


> +        return NULL;
> +    }
> +
> +    av_log(iface->parent, AV_LOG_DEBUG, "%s", iface->avbstr.str);
> +    return iface->avbstr.str;
> +}
> +

> +static struct scte_35_event* alloc_scte35_event(int id)
> +{
> +    struct scte_35_event* event = av_malloc(sizeof(struct scte_35_event));
> +    event->id = id;

missing malloc failure check


> +    event->in_pts = AV_NOPTS_VALUE;
> +    event->nearest_in_pts = AV_NOPTS_VALUE;
> +    event->out_pts = AV_NOPTS_VALUE;
> +    event->lock = 0;
> +    event->cancel = 1;
> +    event->next = NULL;
> +    event->prev = NULL;

av_mallocz() can simplify this


[...]
> +struct scte_35_interface* ff_alloc_scte35_parser(void *parent, AVRational timebase)
> +{
> +    struct scte_35_interface* iface = av_mallocz(sizeof(struct scte_35_interface));
> +
> +    iface->parent = parent;

missing malloc failure check


> +    iface->timebase = timebase;
> +    iface->get_event_pts = get_event_pts;
> +    iface->get_event_cache = get_event_cache;
> +    av_bprint_init(&iface->avbstr, 0, AV_BPRINT_SIZE_UNLIMITED);
> +    iface->get_hls_string = get_hls_string;
> +    iface->unref_scte35_event = unref_scte35_event;
> +    iface->ref_scte35_event = ref_scte35_event;
> +    iface->event_out = EVENT_NONE;
> +    iface->prev_event_state = EVENT_NONE;
> +    return iface;
> +}
> +
> +void ff_delete_scte35_parser(struct scte_35_interface* iface)
> +{
> +    av_freep(&iface);
> +}

> diff --git a/libavformat/scte_35.h b/libavformat/scte_35.h
> new file mode 100644
> index 0000000..35a84c9
> --- /dev/null
> +++ b/libavformat/scte_35.h
> @@ -0,0 +1,76 @@
> +/*
> + * SCTE-35 parser
> + * Copyright (c) 2016 Carlos Fernandez
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +#ifndef AVFORMAT_SCTE_35_H
> +#define AVFORMAT_SCTE_35_H
> +
> +#include "libavutil/bprint.h"
> +
> +struct scte_35_event {
> +    int32_t id;
> +    uint64_t in_pts;
> +    uint64_t nearest_in_pts;
> +    uint64_t out_pts;
> +    int64_t duration;
> +    int64_t start_pos;
> +    int cancel;

> +    /*if advertisement have already started cancel command can't delete advertisement */
> +    volatile int lock;
> +    volatile int ref_count;

iam not sure what you are trying to do here but the volatile is
very likely not achieving it
can you explain what the volatile is intended to do ?


> +    struct scte_35_event *next;
> +    struct scte_35_event *prev;
> +};
> +struct scte_35_interface {
> +    struct scte_35_event *event_list;
> +    char adv_filename[1024];
> +    char filename[1024];
> +    int event_out;
> +    AVRational timebase;
> +    int adv_count;
> +    struct scte_35_event *cache_event;
> +    int prev_event_state;
> +    //TODO use AV_BASE64_SIZE to dynamically allocate the array
> +    char pkt_base64[1024];
> +    /* keep context of its parent for log */
> +    void *parent;
> +
> +    struct scte_35_event* (*get_event_pts)(struct scte_35_interface *iface, uint64_t pts);
> +    struct scte_35_event* (*get_event_cache)(struct scte_35_interface *iface);
> +    /* general purpose str */
> +    AVBPrint avbstr;
> +    char* (*get_hls_string)(struct scte_35_interface *iface, struct scte_35_event *event,
> +               const char *adv_filename, int out_state, int seg_count, int64_t pos);
> +
> +    void (*unref_scte35_event)(struct scte_35_event **event);
> +    void (*ref_scte35_event)(struct scte_35_event *event);

These function pointers are only initialized to the same functions
so they wouldnt not be needed if iam not missing anything.
But please explain what the idea here is ?


> +};
> +
> +enum event_state {
> +    EVENT_NONE,
> +    EVENT_IN,
> +    EVENT_OUT,
> +    EVENT_OUT_CONT,
> +};
> +
> +int ff_parse_scte35_pkt(struct scte_35_interface *iface, const AVPacket *avpkt);
> +
> +struct scte_35_interface* ff_alloc_scte35_parser(void *parent, AVRational timebase);
> +void ff_delete_scte35_parser(struct scte_35_interface* iface);

most of the functions and structure fields in the header are lacking
documentation

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who would give up essential Liberty, to purchase a little
temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160809/a0e85fc8/attachment.sig>


More information about the ffmpeg-devel mailing list