[FFmpeg-devel] issue 1483
gordon pendleton
wgordonw1 at gmail.com
Tue Jul 3 01:19:12 CEST 2012
On Fri, Jun 29, 2012 at 4:21 AM, Stefano Sabatini <stefasab at gmail.com>wrote:
> On date Thursday 2012-06-28 22:36:22 -0400, gordon pendleton encoded:
> > On Thu, Jun 28, 2012 at 2:04 AM, Carl Eugen Hoyos <cehoyos at ag.or.at>
> wrote:
> >
> > > gordon pendleton <wgordonw1 <at> gmail.com> writes:
> > >
> > > > This is my first contribution to FFmpeg, but I tried to follow:
> > > > http://ffmpeg.org/developer.html#patch-submission-checklist
> > >
> > > Purely cosmetic comments, I cannot comment on the actual code:
> > > Indentation is 4, some blocks look misaligned, you can try
> > > to use tools/patcheck to check your patch.
>
> Note that you can use tools/patcheck for checkig such issues. On the
> other hand I don't believe we should be very strict with such
> cosmetical stuff especially with newcomers and occasional
> contributors, for which the committer can the nits when committing and
> waste less time of both him/herself and the contributor.
>
> > >
> > > - 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_compare_ts(pkt->pts, st->time_base, end_pts,
> > > AV_TIME_BASE_Q) >= 0) {
> > >
> > > You can change this block to reduce the diff, since the new
> > > line is too long, this is wanted.
> > >
> > > Carl Eugen
> > >
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel at ffmpeg.org
> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> >
> >
> > Thanks for looking at it Carl. I don't know why but my eclipse settings
> > weren't set to replace tabs with spaces. I ran the patch check tool and
> > this seems to look okay now.
> >
> > I amended my git commit:
> >
> https://github.com/thenewguy/FFmpeg/commit/db7fbe2fa4160c0fb9e33ea8061f880fdcb58b97
> >
> > Updated patch copied below:
> >
> >
> > From db7fbe2fa4160c0fb9e33ea8061f880fdcb58b97 Mon Sep 17 00:00:00 2001
> > From: gordon <wgordonw1 at gmail.com>
> > Date: Wed, 27 Jun 2012 20:39:36 -0400
> > Subject: [PATCH] support a keyframe white list in the segment format. if
> > present, segments will only start with the provided frames.
> > Signed-off-by: Gordon <wgordonw1 at gmail.com>
>
> I have a local branch with a similar feature (support a list of
> *times* rather than a list of frames).
>
> >
> > ---
> > libavformat/segment.c | 64
> > ++++++++++++++++++++++++++++++++++++++++--------
> > 1 files changed, 53 insertions(+), 11 deletions(-)
>
> Please mention the option in doc/muxers.texi, or people won't be able
> to figure it out.
>
> > 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).
>
> > float time; /**< Set by a private option. */
> > int size; /**< Set by a private option. */
> > int wrap; /**< Set by a private option. */
> > @@ -43,6 +45,10 @@ typedef struct {
> > int64_t recording_time;
> > int has_video;
> > AVIOContext *pb;
> > + int64_t next_valid_frame;
> > + char *next_valid_frame_ptr;
> > + int64_t frame_count;
> > + int observe_valid_frames;
> > } SegmentContext;
> >
> > static int segment_start(AVFormatContext *s)
> > @@ -107,10 +113,26 @@ static int seg_write_header(AVFormatContext *s)
> > SegmentContext *seg = s->priv_data;
> > AVFormatContext *oc;
> > int ret, i;
> > + char* token;
> >
> > seg->number = 0;
> > seg->offset_time = 0;
> > seg->recording_time = seg->time * 1000000;
> > + seg->frame_count = 0;
> > + seg->valid_frame_delimiter = ",";
> > + seg->next_valid_frame_ptr = NULL;
> > +
> > + if (seg->valid_frames) {
> > + token = av_strtok(seg->valid_frames, seg->valid_frame_delimiter,
> > &seg->next_valid_frame_ptr);
> > + if (token) {
> > + seg->next_valid_frame = strtol(token, NULL, 10);
> > + }
> > + seg->observe_valid_frames = 1;
> > + } else {
> > + seg->next_valid_frame = 0;
> > + seg->observe_valid_frames = 0;
> > + }
> > +
>
> I'd prefer to do all the parsing in the init stage (and add more
> validation), so that in case the provided list is not valid you fail
> as soon as possible, should also simplify the logic.
>
> Check attached patch for an example of how I'm doing it in a local
> patchset.
>
> >
> > oc = avformat_alloc_context();
> >
> > @@ -189,14 +211,29 @@ 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;
> > + char* token;
> > + int can_split;
> > +
> > + can_split = seg->has_video && st->codec->codec_type ==
> > AVMEDIA_TYPE_VIDEO && (pkt->flags & AV_PKT_FLAG_KEY);
> > +
> > + if (seg->observe_valid_frames) {
> > + if (seg->next_valid_frame < seg->frame_count) {
> > + token = av_strtok(NULL, seg->valid_frame_delimiter,
> > &seg->next_valid_frame_ptr);
> > + if (token) {
> > + seg->next_valid_frame = strtol(token, NULL, 10);
> > + }
> > + }
> > + 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_compare_ts(pkt->pts, st->time_base, end_pts,
> > AV_TIME_BASE_Q) >= 0) {
> >
> > - 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);
> > +
> > + seg->next_valid_frame = -1;
> >
> > ret = segment_end(oc);
> >
> > @@ -218,6 +255,10 @@ static int seg_write_packet(AVFormatContext *s,
> > AVPacket *pkt)
> > }
> > }
> >
> > + if (st->codec->codec_type == AVMEDIA_TYPE_VIDEO) {
> > + seg->frame_count++;
> > + }
> > +
> > ret = oc->oformat->write_packet(oc, pkt);
> >
> > fail:
> > @@ -248,11 +289,12 @@ static int seg_write_trailer(struct AVFormatContext
> > *s)
> > #define OFFSET(x) offsetof(SegmentContext, x)
> > #define E AV_OPT_FLAG_ENCODING_PARAM
> > static const AVOption options[] = {
> > - { "segment_format", "container format used for the segments",
> > OFFSET(format), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, E },
> > - { "segment_time", "segment length in seconds",
> > OFFSET(time), AV_OPT_TYPE_FLOAT, {.dbl = 2}, 0, FLT_MAX, E },
> > - { "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_format", "container format used for the
> > segments", OFFSET(format),
> > AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, E },
> > + { "segment_time", "segment length in
> > seconds", OFFSET(time),
> > AV_OPT_TYPE_FLOAT, {.dbl = 2}, 0, FLT_MAX, E },
> > + { "segment_valid_frames", "comma separated list of frame numbers
> > allowed to start segments", OFFSET(valid_frames),
> > AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, E },
> > + { "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 },
> > { NULL },
> > };
>
> Mangled patch, please attach it as an attachment rather than inline.
> --
> FFmpeg = Faithful & Frenzy Moronic Purposeless Ecletic Gorilla
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
I forgot to send this out to the list yesterday. I reworked the patch to
do validation and parsing immediately and to provide feedback when parsing
fails.
I used Stefano's repo as a guideline.
My github repo is here: https://github.com/thenewguy/ffmpeg_segment_frames
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg_segment_frames_201207011906.patch
Type: application/octet-stream
Size: 37491 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120702/243ae577/attachment.obj>
More information about the ffmpeg-devel
mailing list