[FFmpeg-devel] Match source video timestamp
Nicolas George
george at nsup.org
Sun Apr 30 14:40:12 EEST 2017
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170430/4c2861fc/attachment.sig>
More information about the ffmpeg-devel
mailing list