[FFmpeg-devel] Match source video timestamp

Eran Kornblau eran.kornblau at kaltura.com
Mon May 8 17:45:55 EEST 2017


Ping

Eran

-----Original Message-----
From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf Of Eran Kornblau
Sent: Wednesday, May 3, 2017 11:59 AM
To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
Subject: Re: [FFmpeg-devel] Match source video timestamp

[This sender failed our fraud detection checks and may not be who they appear to be. Learn about spoofing at http://aka.ms/LearnAboutSpoofing]

>
>
> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf 
> Of Nicolas George
> Sent: Sunday, April 30, 2017 2:40 PM
> To: FFmpeg development discussions and patches 
> <ffmpeg-devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] Match source video timestamp
>
> Le primidi 11 floréal, an CCXXV, Eran Kornblau a écrit :
> > Ping, re-attaching the same patch
>
> Hi. Thanks for the bug report and patch. I do not know the issue well enough to address the core validity in principle, but I have a few design comments that will need to be addressed.
>
> > Subject: [PATCH] ffmpeg: add video/audio_timescale options
> >
> > video/audio_timescale set the encoding time base -
> >  0 - for video, uses 1/frame_rate, for audio uses 1/sample_rate (this is
> >   the default)
> > -1 - matches the input time base (when possible)
> > >0 - sets the time base to 1/(the provided timescale value)
>
> First, I think that "timescale" is slang specific to the MOV/MP4 formats. It looks similar to what FFmpeg calls "time base" all over the place, but I am not entirely sure it is exactly the same thing. For the sake of readability, it would be better to use a consistent vocabulary within FFmpeg and not import new terms.
>
> Second, this interface relies on magic numbers, we try to avoid that.
> Internally, you should define the magic numbers as symbolic constants, either using #define or enums. For the user interface, you should use AV_OPT_TYPE_CONST and a named constant.
>
> > ---
> >  ffmpeg.c     | 36 ++++++++++++++++++++++++++++++++++--
> >  ffmpeg.h     |  2 ++
> >  ffmpeg_opt.c |  8 ++++++++
> >  3 files changed, 44 insertions(+), 2 deletions(-)
>
> This is missing the documentation for the new options.
>
> >
> > diff --git a/ffmpeg.c b/ffmpeg.c
> > index 70431e8..d18f923 100644
> > --- a/ffmpeg.c
> > +++ b/ffmpeg.c
> > @@ -3301,10 +3301,42 @@ static int init_output_stream_encode(OutputStream *ost)
> >          enc_ctx->sample_rate    = av_buffersink_get_sample_rate(ost->filter->filter);
> >          enc_ctx->channel_layout = av_buffersink_get_channel_layout(ost->filter->filter);
> >          enc_ctx->channels       = av_buffersink_get_channels(ost->filter->filter);
>
> > -        enc_ctx->time_base      = (AVRational){ 1, enc_ctx->sample_rate };
>
> I think the ability to control the time base chosen by FFmpeg may be useful. But note that there are two time bases at play here: the encoder time base and the stream time base. The interaction between the twos is quite subtle at several places.
>
> > +
> > +        switch (audio_timescale) {
> > +        case -1:
> > +            if (ist) {
> > +                enc_ctx->time_base = ist->st->time_base;
> > +                break;
> > +            }
> > +            av_log(oc, AV_LOG_WARNING, "Input stream data not available, using the sample rate as the time base\n");
> > +            /* fall-through */
> > +        case 0:
>
> > +            enc_ctx->time_base = (AVRational){ 1, 
> > + enc_ctx->sample_rate };
>
> I would suggest to use av_make_q() in new code.
>
> > +            break;
> > +
> > +        default:
> > +            enc_ctx->time_base = (AVRational){ 1, audio_timescale };
> > +            break;
> > +        }
> >          break;
>
> >      case AVMEDIA_TYPE_VIDEO:
>
> The two branches of the switch look now very similar and non trivial, I think you should invert the tests to refactor the common code.
>
> > -        enc_ctx->time_base = av_inv_q(ost->frame_rate);
> > +        switch (video_timescale) {
> > +        case -1:
> > +            if (ist) {
> > +                enc_ctx->time_base = ist->st->time_base;
> > +                break;
> > +            }
> > +            av_log(oc, AV_LOG_WARNING, "Input stream data not available, using the frame rate as the time base\n");
> > +             /* fall-through */
> > +        case 0:
> > +            enc_ctx->time_base = av_inv_q(ost->frame_rate);
> > +            break;
> > +
> > +        default:
> > +            enc_ctx->time_base = (AVRational){ 1, video_timescale };
> > +            break;
> > +        }
> > +
> >          if (!(enc_ctx->time_base.num && enc_ctx->time_base.den))
> >              enc_ctx->time_base = av_buffersink_get_time_base(ost->filter->filter);
> >          if (   av_q2d(enc_ctx->time_base) < 0.001 && video_sync_method != VSYNC_PASSTHROUGH
> > diff --git a/ffmpeg.h b/ffmpeg.h
> > index 4d0456c..84c168a 100644
> > --- a/ffmpeg.h
> > +++ b/ffmpeg.h
> > @@ -594,6 +594,8 @@ extern int do_pkt_dump;  extern int copy_ts; 
> > extern int start_at_zero;  extern int copy_tb;
>
> > +extern int audio_timescale;
> > +extern int video_timescale;
>
> This can not do. The "timescale" do not apply to all audio and all video the same. Whenever you feel the need to add almost the same option, one for audio and one for video, odds are it should be a per-stream option instead. And I think it is the case here.
>
> >  extern int debug_ts;
> >  extern int exit_on_error;
> >  extern int abort_on_flags;
> > diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c index d1fe874..36a85c0 
> > 100644
> > --- a/ffmpeg_opt.c
> > +++ b/ffmpeg_opt.c
> > @@ -111,6 +111,8 @@ int do_pkt_dump       = 0;
> >  int copy_ts           = 0;
> >  int start_at_zero     = 0;
> >  int copy_tb           = -1;
> > +int audio_timescale   = 0;
> > +int video_timescale   = 0;
> >  int debug_ts          = 0;
> >  int exit_on_error     = 0;
> >  int abort_on_flags    = 0;
> > @@ -3392,6 +3394,12 @@ const OptionDef options[] = {
> >          "shift input timestamps to start at 0 when using copyts" },
> >      { "copytb",         HAS_ARG | OPT_INT | OPT_EXPERT,              { &copy_tb },
> >          "copy input stream time base when stream copying", "mode" 
> > },
> > +    { "video_timescale",HAS_ARG | OPT_INT | OPT_EXPERT,              { &video_timescale },
> > +        "time base for video encoding, two special values are defined -"
> > +        "0 = use frame rate, -1 = match source time base" },
> > +    { "audio_timescale",HAS_ARG | OPT_INT | OPT_EXPERT,              { &audio_timescale },
> > +        "time base for audio encoding, two special values are defined -"
> > +        "0 = use sample rate, -1 = match source time base" },
> >      { "shortest",       OPT_BOOL | OPT_EXPERT | OPT_OFFSET |
> >                          OPT_OUTPUT,                                  { .off = OFFSET(shortest) },
> >          "finish encoding within shortest input" },
>
> Regards,
>
> --
>   Nicolas George
>

Nicolas, thank you for your comments!

Please see updated patch attached.
I believe I addressed all the issues you raised, except the addition of new #define/enum constants, since the code now uses AVRational to save the timebase (I matched the existing -time_base option) it is less natural to do so.

Thanks,

Eran
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ffmpeg-add-enc_time_base-option.patch
Type: application/octet-stream
Size: 5935 bytes
Desc: 0001-ffmpeg-add-enc_time_base-option.patch
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170508/380c77f0/attachment.obj>


More information about the ffmpeg-devel mailing list