[FFmpeg-devel] issue 1483

Stefano Sabatini stefasab at gmail.com
Thu Jul 5 00:55:57 CEST 2012


On date Monday 2012-07-02 21:49:56 -0400, gordon pendleton encoded:
> My apologies for the awful patch files.  I think this one should be good
> now though.  Patcheck only gives me a mergable call warning and a warning
> about unused option.
> 
> On Mon, Jul 2, 2012 at 7:34 PM, gordon pendleton <wgordonw1 at gmail.com>wrote:
> 
> >
> > > diff --git a/libavformat/segment.c b/libavformat/segment.c
> >> > index 6025aad..7a32e4e 100644
> >> > --- a/libavformat/segment.c
> >> > +++ b/libavformat/segment.c
> >> > @@ -36,6 +36,8 @@ typedef struct {
> >> >      AVFormatContext *avf;
> >> >      char *format;          /**< Set by a private option. */
> >> >      char *list;            /**< Set by a private option. */
> >> > +    char *valid_frames;    /**< Set by a private option. */
> >>
> >> > +    const char *valid_frame_delimiter;
> >>
> >> Why this (IIRC the list is separated by commas).
> >>
> >>
> > I did that because I meant to make an option for providing a delimiter
> > string (possibly more than one char).  It wasn't very important to me so I
> > left it out when I reworked the patches.
> >

> From 3fa87094c15a8e1edabd5a0d9f6c022ccf7a98bc Mon Sep 17 00:00:00 2001
> From: gordon <wgordonw1 at gmail.com>
> Date: Sun, 1 Jul 2012 15:25:15 -0400
> Subject: [PATCH] implement #1483 -- optionally allow whitelist of keyframes
>  which can be used to start a new segment
> 
> 
> Signed-off-by: gordon <wgordonw1 at gmail.com>
> ---
>  libavformat/segment.c |   88 +++++++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 82 insertions(+), 6 deletions(-)
> 
> diff --git a/libavformat/segment.c b/libavformat/segment.c
> index da23626..ecd96c6 100644
> --- a/libavformat/segment.c
> +++ b/libavformat/segment.c
> @@ -43,8 +43,50 @@ typedef struct {
>      int64_t recording_time;
>      int has_video;
>      AVIOContext *pb;
> +    char *valid_frames_str;    /**< delimited list of valid frames to start a new segment */

split_frames_str or simply frames_str is fine ("valid" is rather
ambiguous/confusing).

> +    int64_t *valid_frames; /**< holds parsed valid_frames_str */
> +    int64_t nb_valid_frames; /**< count of valid frames */
> +    int64_t next_valid_frame; /**< the next frame number that is valid for starting a new segment */
> +    int64_t next_valid_frame_index; /**< array index of the current next_valid_frame */
> +    int64_t frame_count; /**< current video frame count */
>  } SegmentContext;
>  
> +static int parse_valid_frames(void *log_ctx, int64_t **valid_frames, char *valid_frames_str, int64_t *nb_valid_frames)
> +{
> +    char *p;
> +    int64_t i;
> +    char *frame = NULL;
> +
> +    i = 0;
> +    for (p = (char *) valid_frames_str; *p; p++)
> +        if (*p == ',')
> +            i++;
> +    *nb_valid_frames = (int64_t) (i+1);
> +
> +    *valid_frames = (int64_t *) av_realloc_f(NULL, sizeof(**valid_frames), i);
> +    if (!*valid_frames) {
> +        av_log(log_ctx, AV_LOG_ERROR, "Could not allocate valid frames array.\n");
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    i = 0;
> +    frame = av_strtok(valid_frames_str, ",", &p);
> +    while (frame) {
> +        (*valid_frames)[i] = strtol(frame, NULL, 10);
> +
> +        if (i && (*valid_frames)[i] <= (*valid_frames)[i-1]) {
> +            av_log(log_ctx, AV_LOG_ERROR,
> +                    "Valid frames must be specified in ascending order without duplicate values.\n");
> +            return AVERROR(EINVAL);
> +        }
> +
> +        frame = av_strtok(NULL, ",", &p);
> +        i++;
> +    }
> +
> +    return 0;
> +}
> +
>  static int segment_start(AVFormatContext *s)
>  {
>      SegmentContext *seg = s->priv_data;
> @@ -117,6 +159,8 @@ static int seg_write_header(AVFormatContext *s)
>      seg->number = 0;
>      seg->offset_time = 0;
>      seg->recording_time = seg->time * 1000000;
> +    seg->nb_valid_frames = 0;
> +    seg->frame_count = 0;
>  
>      oc = avformat_alloc_context();
>  
> @@ -127,6 +171,20 @@ static int seg_write_header(AVFormatContext *s)
>          if ((ret = avio_open2(&seg->pb, seg->list, AVIO_FLAG_WRITE,
>                                &s->interrupt_callback, NULL)) < 0)
>              goto fail;
> +
> +    if (seg->valid_frames_str) {
> +        if ((ret = parse_valid_frames(s, &seg->valid_frames, seg->valid_frames_str, &seg->nb_valid_frames)) < 0)
> +            return ret;
> +

> +        if (seg->nb_valid_frames) {
> +            if (seg->valid_frames[0]) {
> +                seg->next_valid_frame_index = 0;
> +            } else {
> +                seg->next_valid_frame_index = 1;
> +            }

IMO you can avoid the special case, the user is expected to specify
a split frame != 0.

> +            seg->next_valid_frame = seg->valid_frames[seg->next_valid_frame_index];
> +        }
> +    }
>  
>      for (i = 0; i< s->nb_streams; i++)
>          seg->has_video +=
> @@ -195,14 +253,28 @@ static int seg_write_packet(AVFormatContext *s, AVPacket *pkt)
>      AVStream *st = oc->streams[pkt->stream_index];
>      int64_t end_pts = seg->recording_time * seg->number;
>      int ret;
> +    int can_split = (

> +            seg->has_video
> +            && st->codec->codec_type == AVMEDIA_TYPE_VIDEO

this is redundant (if the stream is video => seg->has_video)

> +            && pkt->flags & AV_PKT_FLAG_KEY
> +    );
> +
> +    if (can_split && av_compare_ts(pkt->pts, st->time_base, end_pts, AV_TIME_BASE_Q) < 0)
> +        can_split = 0;
> +
> +    if (can_split && seg->nb_valid_frames) {
> +        if (seg->next_valid_frame < seg->frame_count && (seg->next_valid_frame_index + 1) < seg->nb_valid_frames) {

Why not seg->frame_count >= next_valid_frame?

In order to simplify the logic you can set the last next_split_frame
to INT64_MAX.

> +            seg->next_valid_frame_index++;
> +            seg->next_valid_frame = seg->valid_frames[seg->next_valid_frame_index];

can be merged

> +        }
> +        if (seg->next_valid_frame != seg->frame_count)
> +            can_split = 0;
> +    }
>  
> -    if ((seg->has_video && st->codec->codec_type == AVMEDIA_TYPE_VIDEO) &&
> -        av_compare_ts(pkt->pts, st->time_base,
> -                      end_pts, AV_TIME_BASE_Q) >= 0 &&
> -        pkt->flags & AV_PKT_FLAG_KEY) {
> +    if (can_split) {
>  
> -        av_log(s, AV_LOG_DEBUG, "Next segment starts at %d %"PRId64"\n",
> -               pkt->stream_index, pkt->pts);
> +        av_log(s, AV_LOG_DEBUG, "Next segment starts at %d %"PRId64" with frame count of %"PRId64" \n",
> +                       pkt->stream_index, pkt->pts, seg->frame_count);
>  
>          ret = segment_end(oc);
>  
> @@ -224,6 +296,9 @@ static int seg_write_packet(AVFormatContext *s, AVPacket *pkt)
>          }
>      }

> +    if (st->codec->codec_type == AVMEDIA_TYPE_VIDEO)
> +        seg->frame_count++;
> +

I wonder if it is possible to make use of the feature also for
splitting audio-only files.

>      ret = oc->oformat->write_packet(oc, pkt);
>  
>  fail:
> @@ -259,6 +334,7 @@ static const AVOption options[] = {
>      { "segment_list",      "output the segment list",                 OFFSET(list),    AV_OPT_TYPE_STRING, {.str = NULL},  0, 0,       E },
>      { "segment_list_size", "maximum number of playlist entries",      OFFSET(size),    AV_OPT_TYPE_INT,    {.dbl = 5},     0, INT_MAX, E },
>      { "segment_wrap",      "number after which the index wraps",      OFFSET(wrap),    AV_OPT_TYPE_INT,    {.dbl = 0},     0, INT_MAX, E },
> +    { "segment_valid_frames",     "set valid segment split frames",        OFFSET(valid_frames_str), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0,      E },
>      { NULL },
>  };

Note, from the ticket:

|My feature request will allow the encoder to optimize keyframe
|placement for different quality levels of the encode in variant
|streams without risking playback getting out of sync between the
|variant streams which makes swapping between streams impossible.
|

|Adding a command line option to input a comma separated list of frame
|numbers would be an easy way to implement this. Then the list can be
|checked against the current frame number when outputting packets into
|segments.

Do you mean something along -force_key_frames (but accepting frames
indices rather than times)?

Also missing doc update.

BTW you know I did many changes to the segmenter, so this patch
doesn't apply, I'd ask you to wait still a few days before I merge the
other patches I sent for review, or we'll waste time fixing conflicts.
-- 
FFmpeg = Fabulous and Freak Mega Political Enchanting Gigant


More information about the ffmpeg-devel mailing list