[FFmpeg-devel] issue 1483
Stefano Sabatini
stefasab at gmail.com
Fri Jun 29 10:21:19 CEST 2012
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0019-lavf-segment-add-segment_times-option.patch
Type: text/x-diff
Size: 4963 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120629/8b503777/attachment.bin>
More information about the ffmpeg-devel
mailing list