[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