| Message ID | 20160913084402.10841-1-u@pkh.me |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Sep 13, 2016 at 10:44:02AM +0200, Clément Bœsch wrote: > From: Clément Bœsch <clement@stupeflix.com> > > In lavf we have access to st->internal->avctx so it's a better place > than in ffmpeg*.c and will allow moving to codecpar. > --- > doc/APIchanges | 4 ++++ > ffmpeg.c | 52 ++++-------------------------------------- > libavformat/avformat.h | 21 +++++++++++++++++ > libavformat/utils.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++ > libavformat/version.h | 4 ++-- > 5 files changed, 92 insertions(+), 50 deletions(-) LGTM, maybe give others a day or so to comment before pushing thx [...]
On 9/13/2016 5:44 AM, Clément Bœsch wrote: > From: Clément Bœsch <clement@stupeflix.com> > > In lavf we have access to st->internal->avctx so it's a better place > than in ffmpeg*.c and will allow moving to codecpar. > --- > doc/APIchanges | 4 ++++ > ffmpeg.c | 52 ++++-------------------------------------- > libavformat/avformat.h | 21 +++++++++++++++++ > libavformat/utils.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++ > libavformat/version.h | 4 ++-- > 5 files changed, 92 insertions(+), 50 deletions(-) > > diff --git a/doc/APIchanges b/doc/APIchanges > index 7ac809c..158a0b2 100644 > --- a/doc/APIchanges > +++ b/doc/APIchanges > @@ -15,6 +15,10 @@ libavutil: 2015-08-28 > > API changes, most recent first: > > +2016-09-xx - xxxxxxx - lavf 57.49.100 - avformat.h > + Add avformat_transfer_internal_stream_timing_info helper to help with stream > + copy. > + > 2016-08-29 - 4493390 - lavfi 6.58.100 - avfilter.h > Add AVFilterContext.nb_threads. > > diff --git a/ffmpeg.c b/ffmpeg.c > index d858407..b16b4ad 100644 > --- a/ffmpeg.c > +++ b/ffmpeg.c > @@ -2909,57 +2909,13 @@ static int transcode_init(void) > enc_ctx->bits_per_coded_sample = dec_ctx->bits_per_coded_sample; > enc_ctx->bits_per_raw_sample = dec_ctx->bits_per_raw_sample; > > - enc_ctx->time_base = ist->st->time_base; > - /* > - * Avi is a special case here because it supports variable fps but > - * having the fps and timebase differe significantly adds quite some > - * overhead > - */ > - if(!strcmp(oc->oformat->name, "avi")) { > - if ( copy_tb<0 && ist->st->r_frame_rate.num > - && av_q2d(ist->st->r_frame_rate) >= av_q2d(ist->st->avg_frame_rate) > - && 0.5/av_q2d(ist->st->r_frame_rate) > av_q2d(ist->st->time_base) > - && 0.5/av_q2d(ist->st->r_frame_rate) > av_q2d(dec_ctx->time_base) > - && av_q2d(ist->st->time_base) < 1.0/500 && av_q2d(dec_ctx->time_base) < 1.0/500 > - || copy_tb==2){ > - enc_ctx->time_base.num = ist->st->r_frame_rate.den; > - enc_ctx->time_base.den = 2*ist->st->r_frame_rate.num; > - enc_ctx->ticks_per_frame = 2; > - } else if ( copy_tb<0 && av_q2d(dec_ctx->time_base)*dec_ctx->ticks_per_frame > 2*av_q2d(ist->st->time_base) > - && av_q2d(ist->st->time_base) < 1.0/500 > - || copy_tb==0){ > - enc_ctx->time_base = dec_ctx->time_base; > - enc_ctx->time_base.num *= dec_ctx->ticks_per_frame; > - enc_ctx->time_base.den *= 2; > - enc_ctx->ticks_per_frame = 2; > - } > - } else if(!(oc->oformat->flags & AVFMT_VARIABLE_FPS) > - && strcmp(oc->oformat->name, "mov") && strcmp(oc->oformat->name, "mp4") && strcmp(oc->oformat->name, "3gp") > - && strcmp(oc->oformat->name, "3g2") && strcmp(oc->oformat->name, "psp") && strcmp(oc->oformat->name, "ipod") > - && strcmp(oc->oformat->name, "f4v") > - ) { > - if( copy_tb<0 && dec_ctx->time_base.den > - && av_q2d(dec_ctx->time_base)*dec_ctx->ticks_per_frame > av_q2d(ist->st->time_base) > - && av_q2d(ist->st->time_base) < 1.0/500 > - || copy_tb==0){ > - enc_ctx->time_base = dec_ctx->time_base; > - enc_ctx->time_base.num *= dec_ctx->ticks_per_frame; > - } > - } > - if ( enc_ctx->codec_tag == AV_RL32("tmcd") > - && dec_ctx->time_base.num < dec_ctx->time_base.den > - && dec_ctx->time_base.num > 0 > - && 121LL*dec_ctx->time_base.num > dec_ctx->time_base.den) { > - enc_ctx->time_base = dec_ctx->time_base; > - } > - > if (!ost->frame_rate.num) > ost->frame_rate = ist->framerate; > - if(ost->frame_rate.num) > - enc_ctx->time_base = av_inv_q(ost->frame_rate); > + ost->st->avg_frame_rate = ost->frame_rate; > > - av_reduce(&enc_ctx->time_base.num, &enc_ctx->time_base.den, > - enc_ctx->time_base.num, enc_ctx->time_base.den, INT_MAX); > + ret = avformat_transfer_internal_stream_timing_info(oc->oformat, ost->st, ist->st, copy_tb); > + if (ret < 0) > + return ret; > > if (ist->st->nb_side_data) { > ost->st->side_data = av_realloc_array(NULL, ist->st->nb_side_data, > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > index 74915a1..389d985 100644 > --- a/libavformat/avformat.h > +++ b/libavformat/avformat.h > @@ -2894,6 +2894,27 @@ int av_apply_bitstream_filters(AVCodecContext *codec, AVPacket *pkt, > AVBitStreamFilterContext *bsfc); > #endif > > +enum AVTimebaseCopyFrom { AVTimebaseSource? CopySource? CopyFrom sounds kinda weird. > + AVFMT_TBCF_AUTO = -1, > + AVFMT_TBCF_DECODER, > + AVFMT_TBCF_DEMUXER, > + AVFMT_TBCF_R_FRAMERATE, What's the deal with r_frame_rate anyway? There's an FF_API define for it as if it were deprecated, yet the field is not tagged as such and said define is not wrapping all its uses. LGTM with or without taking the above nit into consideration. Thanks a lot.
On Tue, Sep 13, 2016 at 02:36:46PM -0300, James Almer wrote: [...] > > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > > index 74915a1..389d985 100644 > > --- a/libavformat/avformat.h > > +++ b/libavformat/avformat.h > > @@ -2894,6 +2894,27 @@ int av_apply_bitstream_filters(AVCodecContext *codec, AVPacket *pkt, > > AVBitStreamFilterContext *bsfc); > > #endif > > > > +enum AVTimebaseCopyFrom { > > AVTimebaseSource? CopySource? CopyFrom sounds kinda weird. > Sure, anything you prefer > > + AVFMT_TBCF_AUTO = -1, > > + AVFMT_TBCF_DECODER, > > + AVFMT_TBCF_DEMUXER, > > + AVFMT_TBCF_R_FRAMERATE, > > What's the deal with r_frame_rate anyway? There's an FF_API define for it as > if it were deprecated, yet the field is not tagged as such and said define is > not wrapping all its uses. > I have no idea. Should I drop it from the public enum and keep the ==2 hack in the function until we find a clean way? > LGTM with or without taking the above nit into consideration. Thanks a lot. > Thanks for the review
On 9/13/2016 2:42 PM, Clément Bœsch wrote: > On Tue, Sep 13, 2016 at 02:36:46PM -0300, James Almer wrote: > [...] >>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h >>> index 74915a1..389d985 100644 >>> --- a/libavformat/avformat.h >>> +++ b/libavformat/avformat.h >>> @@ -2894,6 +2894,27 @@ int av_apply_bitstream_filters(AVCodecContext *codec, AVPacket *pkt, >>> AVBitStreamFilterContext *bsfc); >>> #endif >>> >>> +enum AVTimebaseCopyFrom { >> >> AVTimebaseSource? CopySource? CopyFrom sounds kinda weird. >> > > Sure, anything you prefer > >>> + AVFMT_TBCF_AUTO = -1, >>> + AVFMT_TBCF_DECODER, >>> + AVFMT_TBCF_DEMUXER, >>> + AVFMT_TBCF_R_FRAMERATE, >> >> What's the deal with r_frame_rate anyway? There's an FF_API define for it as >> if it were deprecated, yet the field is not tagged as such and said define is >> not wrapping all its uses. >> > > I have no idea. Should I drop it from the public enum and keep the ==2 > hack in the function until we find a clean way? You could also wrap the enum inside a FF_API_R_FRAME_RATE check, i guess. But in any case i was just wondering why the field (or the functionality) is supposedly deprecated but not really. Your code is fine as is. > >> LGTM with or without taking the above nit into consideration. Thanks a lot. >> > > Thanks for the review >
On Tue, Sep 13, 2016 at 02:56:16PM -0300, James Almer wrote: > On 9/13/2016 2:42 PM, Clément Bœsch wrote: > > On Tue, Sep 13, 2016 at 02:36:46PM -0300, James Almer wrote: > > [...] > >>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h > >>> index 74915a1..389d985 100644 > >>> --- a/libavformat/avformat.h > >>> +++ b/libavformat/avformat.h > >>> @@ -2894,6 +2894,27 @@ int av_apply_bitstream_filters(AVCodecContext *codec, AVPacket *pkt, > >>> AVBitStreamFilterContext *bsfc); > >>> #endif > >>> > >>> +enum AVTimebaseCopyFrom { > >> > >> AVTimebaseSource? CopySource? CopyFrom sounds kinda weird. > >> > > > > Sure, anything you prefer > > > >>> + AVFMT_TBCF_AUTO = -1, > >>> + AVFMT_TBCF_DECODER, > >>> + AVFMT_TBCF_DEMUXER, > >>> + AVFMT_TBCF_R_FRAMERATE, > >> > >> What's the deal with r_frame_rate anyway? There's an FF_API define for it as > >> if it were deprecated, yet the field is not tagged as such and said define is > >> not wrapping all its uses. > >> > > > > I have no idea. Should I drop it from the public enum and keep the ==2 > > hack in the function until we find a clean way? > > You could also wrap the enum inside a FF_API_R_FRAME_RATE check, i guess. > But in any case i was just wondering why the field (or the functionality) > is supposedly deprecated but not really. Your code is fine as is. > Applied with FF_API_R_FRAME_RATE check and AVTimebaseSource Thanks,
diff --git a/doc/APIchanges b/doc/APIchanges index 7ac809c..158a0b2 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,10 @@ libavutil: 2015-08-28 API changes, most recent first: +2016-09-xx - xxxxxxx - lavf 57.49.100 - avformat.h + Add avformat_transfer_internal_stream_timing_info helper to help with stream + copy. + 2016-08-29 - 4493390 - lavfi 6.58.100 - avfilter.h Add AVFilterContext.nb_threads. diff --git a/ffmpeg.c b/ffmpeg.c index d858407..b16b4ad 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -2909,57 +2909,13 @@ static int transcode_init(void) enc_ctx->bits_per_coded_sample = dec_ctx->bits_per_coded_sample; enc_ctx->bits_per_raw_sample = dec_ctx->bits_per_raw_sample; - enc_ctx->time_base = ist->st->time_base; - /* - * Avi is a special case here because it supports variable fps but - * having the fps and timebase differe significantly adds quite some - * overhead - */ - if(!strcmp(oc->oformat->name, "avi")) { - if ( copy_tb<0 && ist->st->r_frame_rate.num - && av_q2d(ist->st->r_frame_rate) >= av_q2d(ist->st->avg_frame_rate) - && 0.5/av_q2d(ist->st->r_frame_rate) > av_q2d(ist->st->time_base) - && 0.5/av_q2d(ist->st->r_frame_rate) > av_q2d(dec_ctx->time_base) - && av_q2d(ist->st->time_base) < 1.0/500 && av_q2d(dec_ctx->time_base) < 1.0/500 - || copy_tb==2){ - enc_ctx->time_base.num = ist->st->r_frame_rate.den; - enc_ctx->time_base.den = 2*ist->st->r_frame_rate.num; - enc_ctx->ticks_per_frame = 2; - } else if ( copy_tb<0 && av_q2d(dec_ctx->time_base)*dec_ctx->ticks_per_frame > 2*av_q2d(ist->st->time_base) - && av_q2d(ist->st->time_base) < 1.0/500 - || copy_tb==0){ - enc_ctx->time_base = dec_ctx->time_base; - enc_ctx->time_base.num *= dec_ctx->ticks_per_frame; - enc_ctx->time_base.den *= 2; - enc_ctx->ticks_per_frame = 2; - } - } else if(!(oc->oformat->flags & AVFMT_VARIABLE_FPS) - && strcmp(oc->oformat->name, "mov") && strcmp(oc->oformat->name, "mp4") && strcmp(oc->oformat->name, "3gp") - && strcmp(oc->oformat->name, "3g2") && strcmp(oc->oformat->name, "psp") && strcmp(oc->oformat->name, "ipod") - && strcmp(oc->oformat->name, "f4v") - ) { - if( copy_tb<0 && dec_ctx->time_base.den - && av_q2d(dec_ctx->time_base)*dec_ctx->ticks_per_frame > av_q2d(ist->st->time_base) - && av_q2d(ist->st->time_base) < 1.0/500 - || copy_tb==0){ - enc_ctx->time_base = dec_ctx->time_base; - enc_ctx->time_base.num *= dec_ctx->ticks_per_frame; - } - } - if ( enc_ctx->codec_tag == AV_RL32("tmcd") - && dec_ctx->time_base.num < dec_ctx->time_base.den - && dec_ctx->time_base.num > 0 - && 121LL*dec_ctx->time_base.num > dec_ctx->time_base.den) { - enc_ctx->time_base = dec_ctx->time_base; - } - if (!ost->frame_rate.num) ost->frame_rate = ist->framerate; - if(ost->frame_rate.num) - enc_ctx->time_base = av_inv_q(ost->frame_rate); + ost->st->avg_frame_rate = ost->frame_rate; - av_reduce(&enc_ctx->time_base.num, &enc_ctx->time_base.den, - enc_ctx->time_base.num, enc_ctx->time_base.den, INT_MAX); + ret = avformat_transfer_internal_stream_timing_info(oc->oformat, ost->st, ist->st, copy_tb); + if (ret < 0) + return ret; if (ist->st->nb_side_data) { ost->st->side_data = av_realloc_array(NULL, ist->st->nb_side_data, diff --git a/libavformat/avformat.h b/libavformat/avformat.h index 74915a1..389d985 100644 --- a/libavformat/avformat.h +++ b/libavformat/avformat.h @@ -2894,6 +2894,27 @@ int av_apply_bitstream_filters(AVCodecContext *codec, AVPacket *pkt, AVBitStreamFilterContext *bsfc); #endif +enum AVTimebaseCopyFrom { + AVFMT_TBCF_AUTO = -1, + AVFMT_TBCF_DECODER, + AVFMT_TBCF_DEMUXER, + AVFMT_TBCF_R_FRAMERATE, +}; + +/** + * Transfer internal timing information from one stream to another. + * + * This function is useful when doing stream copy. + * + * @param ofmt target output format for ost + * @param ost output stream which needs timings copy and adjustments + * @param ist reference input stream to copy timings from + * @param copy_tb define from where the stream codec timebase needs to be imported + */ +int avformat_transfer_internal_stream_timing_info(const AVOutputFormat *ofmt, + AVStream *ost, const AVStream *ist, + enum AVTimebaseCopyFrom copy_tb); + /** * @} */ diff --git a/libavformat/utils.c b/libavformat/utils.c index 76cbff4..92a29ae 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -5281,3 +5281,64 @@ int ff_bprint_to_codecpar_extradata(AVCodecParameters *par, struct AVBPrint *buf par->extradata_size = buf->len; return 0; } + +int avformat_transfer_internal_stream_timing_info(const AVOutputFormat *ofmt, + AVStream *ost, const AVStream *ist, + enum AVTimebaseCopyFrom copy_tb) +{ + //TODO: use [io]st->internal->avctx + const AVCodecContext *dec_ctx = ist->codec; + AVCodecContext *enc_ctx = ost->codec; + + enc_ctx->time_base = ist->time_base; + /* + * Avi is a special case here because it supports variable fps but + * having the fps and timebase differe significantly adds quite some + * overhead + */ + if (!strcmp(ofmt->name, "avi")) { + if (copy_tb == AVFMT_TBCF_AUTO && ist->r_frame_rate.num + && av_q2d(ist->r_frame_rate) >= av_q2d(ist->avg_frame_rate) + && 0.5/av_q2d(ist->r_frame_rate) > av_q2d(ist->time_base) + && 0.5/av_q2d(ist->r_frame_rate) > av_q2d(dec_ctx->time_base) + && av_q2d(ist->time_base) < 1.0/500 && av_q2d(dec_ctx->time_base) < 1.0/500 + || copy_tb == AVFMT_TBCF_R_FRAMERATE) { + enc_ctx->time_base.num = ist->r_frame_rate.den; + enc_ctx->time_base.den = 2*ist->r_frame_rate.num; + enc_ctx->ticks_per_frame = 2; + } else if (copy_tb == AVFMT_TBCF_AUTO && av_q2d(dec_ctx->time_base)*dec_ctx->ticks_per_frame > 2*av_q2d(ist->time_base) + && av_q2d(ist->time_base) < 1.0/500 + || copy_tb == AVFMT_TBCF_DECODER) { + enc_ctx->time_base = dec_ctx->time_base; + enc_ctx->time_base.num *= dec_ctx->ticks_per_frame; + enc_ctx->time_base.den *= 2; + enc_ctx->ticks_per_frame = 2; + } + } else if (!(ofmt->flags & AVFMT_VARIABLE_FPS) + && strcmp(ofmt->name, "mov") && strcmp(ofmt->name, "mp4") && strcmp(ofmt->name, "3gp") + && strcmp(ofmt->name, "3g2") && strcmp(ofmt->name, "psp") && strcmp(ofmt->name, "ipod") + && strcmp(ofmt->name, "f4v")) { + if (copy_tb == AVFMT_TBCF_AUTO && dec_ctx->time_base.den + && av_q2d(dec_ctx->time_base)*dec_ctx->ticks_per_frame > av_q2d(ist->time_base) + && av_q2d(ist->time_base) < 1.0/500 + || copy_tb == AVFMT_TBCF_DECODER) { + enc_ctx->time_base = dec_ctx->time_base; + enc_ctx->time_base.num *= dec_ctx->ticks_per_frame; + } + } + + if (enc_ctx->codec_tag == AV_RL32("tmcd") + && dec_ctx->time_base.num < dec_ctx->time_base.den + && dec_ctx->time_base.num > 0 + && 121LL*dec_ctx->time_base.num > dec_ctx->time_base.den) { + enc_ctx->time_base = dec_ctx->time_base; + } + + if (ost->avg_frame_rate.num) + enc_ctx->time_base = av_inv_q(ost->avg_frame_rate); + + av_reduce(&enc_ctx->time_base.num, &enc_ctx->time_base.den, + enc_ctx->time_base.num, enc_ctx->time_base.den, INT_MAX); + + return 0; +} diff --git a/libavformat/version.h b/libavformat/version.h index 90ac534..34226ca 100644 --- a/libavformat/version.h +++ b/libavformat/version.h @@ -32,8 +32,8 @@ // Major bumping may affect Ticket5467, 5421, 5451(compatibility with Chromium) // Also please add any ticket numbers that you believe might be affected here #define LIBAVFORMAT_VERSION_MAJOR 57 -#define LIBAVFORMAT_VERSION_MINOR 48 -#define LIBAVFORMAT_VERSION_MICRO 103 +#define LIBAVFORMAT_VERSION_MINOR 49 +#define LIBAVFORMAT_VERSION_MICRO 100 #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \ LIBAVFORMAT_VERSION_MINOR, \
From: Clément Bœsch <clement@stupeflix.com> In lavf we have access to st->internal->avctx so it's a better place than in ffmpeg*.c and will allow moving to codecpar. --- doc/APIchanges | 4 ++++ ffmpeg.c | 52 ++++-------------------------------------- libavformat/avformat.h | 21 +++++++++++++++++ libavformat/utils.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++ libavformat/version.h | 4 ++-- 5 files changed, 92 insertions(+), 50 deletions(-)