[FFmpeg-devel] Fwd: Fwd: libavformat/segment : add option to increment timecode

[Stefano Sabatini]

On date Friday 2016-02-05 11:41:06 +0100, Martin Vignali encoded:
> 2016-01-30 10:44 GMT+01:00 Stefano Sabatini <stefasab at gmail.com>:
> 
> > On date Friday 2016-01-29 17:38:13 +0100, Martin Vignali encoded:
> > > 2016-01-29 12:35 GMT+01:00 Stefano Sabatini <stefasab at gmail.com>:
> > [...]
> > > > +        AVDictionaryEntry * tcr = av_dict_get(s->metadata, "timecode",
> > > > NULL, 0);
> > > > > +        if (tcr){
> > > > > +            if (s->nb_streams > 0){
> > > >
> > > > > +                rate = s->streams[0]->avg_frame_rate;//Get fps from
> > > > first stream
> > > >
> > > > this looks a bit arbitrary, why the first stream?
> > > >
> > > > > +                err = av_timecode_init_from_string(&tc, rate,
> > > > tcr->value, s);
> > > > > +                if (err < 0)
> > > > > +                    return err;
> > > >
> > > > > +                tc.start += (int)(seg->time / 1000000.) *
> > > > av_q2d(rate);//second count * fps
> > > >
> > > > You're losing precision here. Consider using the rescaling utilities
> > > > in libavutil/mathematics.h or something like (seg->time * rate.num) /
> > > > (rate.den * 1000000).
> > [...]
> > > Thanks you for your comments, i will make the modifications.
> > >
> > > For the fps issue, you're right, i use the first stream because in my
> > tests
> > > it's always the video stream.
> > > Is there a better way, to get fps ? without choosing one of the stream ?
> >
> > My guess is that timecode is always related to a video stream, right?
> >
> > In this case you can loop through the streams and select the first
> > video stream (check also the select_reference_stream() function in
> > segment.c).
> >
> >
> I follow your proposal, and search the first video stream, to take fps.
> 
> In attach the new patch (and a documentation).
> 
> Best regards
> 
> Martin
> 
> 
> 
> ping

> From c266385a63b5f768e484e0aaccfa27de1f870663 Mon Sep 17 00:00:00 2001
> From: Martin Vignali <martin.vignali at gmail.com>
> Date: Sat, 30 Jan 2016 20:52:54 +0100
> Subject: [PATCH 1/2] add increment timecode option using segment v2

Nit: lavf/segment: add increment timecode option

You could describe a use case in the log (optional).

> 
> ---
>  libavformat/segment.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/libavformat/segment.c b/libavformat/segment.c
> index 9da5168..933abd1 100644
> --- a/libavformat/segment.c
> +++ b/libavformat/segment.c
> @@ -40,6 +40,7 @@
>  #include "libavutil/parseutils.h"
>  #include "libavutil/mathematics.h"
>  #include "libavutil/time.h"
> +#include "libavutil/timecode.h"
>  #include "libavutil/time_internal.h"
>  #include "libavutil/timestamp.h"
>  
> @@ -94,6 +95,7 @@ typedef struct SegmentContext {
>      char *time_str;        ///< segment duration specification string
>      int64_t time;          ///< segment duration
>      int use_strftime;      ///< flag to expand filename with strftime
> +    int increment_tc;      ///< flag to increment timecode if found
>  
>      char *times_str;       ///< segment times specification string
>      int64_t *times;        ///< list of segment interval specification
> @@ -223,6 +225,32 @@ static int segment_start(AVFormatContext *s, int write_header)
>      SegmentContext *seg = s->priv_data;
>      AVFormatContext *oc = seg->avf;
>      int err = 0;
> +    AVTimecode tc;
> +    AVRational rate;
> +    int i;
> +    
> +    if (seg->increment_tc) {
> +        AVDictionaryEntry * tcr = av_dict_get(s->metadata, "timecode", NULL, 0);

nit: *tcr

> +        if (tcr) {
> +            /* search the first video stream */
> +            for (i = 0; i < s->nb_streams; i++) {
> +                if (s->streams[i]->codec->codec_type == AVMEDIA_TYPE_VIDEO) {
> +                    rate = s->streams[i]->avg_frame_rate;/* Get fps from the video stream */
> +                    err = av_timecode_init_from_string(&tc, rate, tcr->value, s);
> +                    if (err < 0)
> +                        return err;

> +                    tc.start += (seg->time * rate.num) / (rate.den
> * 1000000);/* increment timecode */

This can overflow (yes I know you literally used my own
suggestion). You can use av_mul_q() to avoid that, or cast to floating
point numbers before the multiplication.

> +                    char buf[AV_TIMECODE_STR_SIZE];
> +                    av_dict_set(&s->metadata, "timecode",
> +                                av_timecode_make_string(&tc, buf, 0), 0);/* Set the new tc. */
> +                    break;
> +                }
> +            }

Please drop the comments in this section, since the code is
self-evident.

> +        }
> +        else{
> +            av_log(s, AV_LOG_ERROR, "can't increment timecode. No timecode found");


Style nit:

} else {

Also this should be an AV_LOG_WARNING since it's not an hard failure
(that is, the process will continue to run).

About the message, it could be more descriptive, something like:
"Could not increment timecode, no timecode metadata found"

> +        }
> +    }
>  
>      if (write_header) {
>          avformat_free_context(oc);
> @@ -946,6 +974,7 @@ static const AVOption options[] = {
>      { "segment_start_number", "set the sequence number of the first segment", OFFSET(segment_idx), AV_OPT_TYPE_INT, {.i64 = 0}, 0, INT_MAX, E },
>      { "segment_wrap_number", "set the number of wrap before the first segment", OFFSET(segment_idx_wrap_nb), AV_OPT_TYPE_INT, {.i64 = 0}, 0, INT_MAX, E },
>      { "strftime",          "set filename expansion with strftime at segment creation", OFFSET(use_strftime), AV_OPT_TYPE_BOOL, {.i64 = 0 }, 0, 1, E },
> +    { "increment_tc",          "increment timecode between each segment", OFFSET(increment_tc), AV_OPT_TYPE_INT, {.i64 = 0 }, 0, 1, E },
>      { "break_non_keyframes", "allow breaking segments on non-keyframes", OFFSET(break_non_keyframes), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, E },
>  
>      { "individual_header_trailer", "write header/trailer to each segment", OFFSET(individual_header_trailer), AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, E },
> -- 
> 1.9.3 (Apple Git-50)

> From 28844b00c5d80e236abfb719a348fc4be6a202ea Mon Sep 17 00:00:00 2001
> From: Martin Vignali <martin.vignali at gmail.com>
> Date: Sat, 30 Jan 2016 20:54:37 +0100
> Subject: [PATCH 2/2] doc for increment_tc in segment
> 
> ---
>  doc/muxers.texi | 7 +++++++
>  1 file changed, 7 insertions(+)

Please merge these two patches (use git rebase -i and squeeze the
document patch on top of the first one).

> 
> diff --git a/doc/muxers.texi b/doc/muxers.texi
> index 4ba8883..0a43dfc 100644
> --- a/doc/muxers.texi
> +++ b/doc/muxers.texi
> @@ -997,6 +997,13 @@ implementation for HLS segmentation.
>  The segment muxer supports the following options:
>  
>  @table @option
> +
> + at item increment_tc @var{1|0}

> +Use the @code{increment_tc} function to increment timecode
> +between each segment.

If set to 1, increment timecode between each segment.

> If this is selected, the input need to have
> +a timecode in the first video stream. Default value is
> + at code{0}.
> +

LGTM otherwise.
-- 
FFmpeg = Fierce & Fundamentalist Moronic Prodigious Enhanced Game