[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