> > > -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf Of > Nicolas George > Sent: Sunday, April 30, 2017 2:40 PM > To: FFmpeg development discussions and patches <ffmpeg-devel@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, { > > ©_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
0001-ffmpeg-add-enc_time_base-option.patch
Description: 0001-ffmpeg-add-enc_time_base-option.patch
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
