| Message ID | 1591606685-4450-1-git-send-email-linjie.fu@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] lavc/avcodec: Add caps for the support of variable dimension encoding | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
Quoting Linjie Fu (2020-06-08 10:58:03) > And declare AV_CODEC_CAP_VARIABLE_DIMENSIONS for rawvideo and > wrapped_avframe. > > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > --- > doc/APIchanges | 3 +++ > fftools/cmdutils.c | 2 ++ > libavcodec/avcodec.h | 4 +++- > libavcodec/codec.h | 5 +++++ > libavcodec/rawenc.c | 1 + > libavcodec/version.h | 2 +- > libavcodec/wrapped_avframe.c | 1 + > 7 files changed, 16 insertions(+), 2 deletions(-) During the last iteration, I asked how is this preferable to just making a new encoder instance. Don't think I got a sufficient reply.
> From: "Anton Khirnov" <anton@khirnov.net> > Sent Time: 2020-06-08 18:50:43 (Monday) > To: "FFmpeg development discussions and patches" <ffmpeg-devel@ffmpeg.org> > Cc: > Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the support of variable dimension encoding > > Quoting Linjie Fu (2020-06-08 10:58:03) > > And declare AV_CODEC_CAP_VARIABLE_DIMENSIONS for rawvideo and > > wrapped_avframe. > > > > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > > --- > > doc/APIchanges | 3 +++ > > fftools/cmdutils.c | 2 ++ > > libavcodec/avcodec.h | 4 +++- > > libavcodec/codec.h | 5 +++++ > > libavcodec/rawenc.c | 1 + > > libavcodec/version.h | 2 +- > > libavcodec/wrapped_avframe.c | 1 + > > 7 files changed, 16 insertions(+), 2 deletions(-) > > During the last iteration, I asked how is this preferable to just making > a new encoder instance. Don't think I got a sufficient reply. Thanks Anton for the remind, indeed making a new encoder instance would be more general and suitable for all encoders, with resolution changing in key frames. In this patch set, I prefer to restrict the implementation/support to rawvideo and wrapped_avframe encoders, since they don't need to be recreated when resolution/dimension changes and are required. at this moment. Also as we've discussed about whether it's worthwhile: >> Do we get sufficient benefits from having parameter change capability inside >> encoders over just creating a new encoder instance when needed? Do people >> really need to change resolution mid-GOP? This implementation didn't touch the logic of encoders which supports variable resolution encoding on inter frames(vp9, av1), since such enhancement/requirement seems to be rare for now. Please help to comment whether it's acceptable. (missed this mail due to mail server problem, hence using an alternative account to reply) - Linjie
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Linjie Fu > Sent: Monday, June 8, 2020 23:11 > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the > support of variable dimension encoding > > > From: "Anton Khirnov" <anton@khirnov.net> > > Sent Time: 2020-06-08 18:50:43 (Monday) > > To: "FFmpeg development discussions and patches" <ffmpeg- > devel@ffmpeg.org> > > Cc: > > Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the > support of variable dimension encoding > > > > Quoting Linjie Fu (2020-06-08 10:58:03) > > > And declare AV_CODEC_CAP_VARIABLE_DIMENSIONS for rawvideo and > > > wrapped_avframe. > > > > > > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > > > --- > > > doc/APIchanges | 3 +++ > > > fftools/cmdutils.c | 2 ++ > > > libavcodec/avcodec.h | 4 +++- > > > libavcodec/codec.h | 5 +++++ > > > libavcodec/rawenc.c | 1 + > > > libavcodec/version.h | 2 +- > > > libavcodec/wrapped_avframe.c | 1 + > > > 7 files changed, 16 insertions(+), 2 deletions(-) > > > > During the last iteration, I asked how is this preferable to just making > > a new encoder instance. Don't think I got a sufficient reply. > > Thanks Anton for the remind, indeed making a new encoder instance would > be > more general and suitable for all encoders, with resolution changing in key > frames. > > In this patch set, I prefer to restrict the implementation/support to > rawvideo and wrapped_avframe encoders, since they don't need to be > recreated > when resolution/dimension changes and are required. at this moment. > > Also as we've discussed about whether it's worthwhile: > > >> Do we get sufficient benefits from having parameter change capability > inside > >> encoders over just creating a new encoder instance when needed? Do > people > >> really need to change resolution mid-GOP? > > This implementation didn't touch the logic of encoders which supports > variable > resolution encoding on inter frames(vp9, av1), since such > enhancement/requirement > seems to be rare for now. Sent the encoder instance recreate patch set for review as a follow-up step, please help to review: https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1450 - Linjie
Anton Khirnov (12020-06-08): > During the last iteration, I asked how is this preferable to just making > a new encoder instance. Don't think I got a sufficient reply. How do we know that we can just put the packets of the new instance after the packets of the old instance and it will work? It will work for image codecs, of course. It will not work for raw video codecs, since the frame size and characteristics are global. It may work for some codecs. It may work if we generate side data to renew the extra data. Clearly, making a new encoder instance is not an universal solution. Regards,
Quoting Nicolas George (2020-06-09 11:48:19) > Anton Khirnov (12020-06-08): > > During the last iteration, I asked how is this preferable to just making > > a new encoder instance. Don't think I got a sufficient reply. > > How do we know that we can just put the packets of the new instance > after the packets of the old instance and it will work? The definition of "work" depends on what you do with the data. It is not a given that it will be passed into lavf to be muxed into a file. > > It will work for image codecs, of course. > > It will not work for raw video codecs, since the frame size and > characteristics are global. And yet the patches specifically concern raw video. > > It may work for some codecs. > > It may work if we generate side data to renew the extra data. > > Clearly, making a new encoder instance is not an universal solution. There is no universal solution. My point is that creating a new encoder instance is conceptually simpler and therefore safer than trying to reinitialize all the codec internals.
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Fu, > Linjie > Sent: Tuesday, June 9, 2020 17:02 > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the > support of variable dimension encoding > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > Linjie Fu > > Sent: Monday, June 8, 2020 23:11 > > To: FFmpeg development discussions and patches <ffmpeg- > > devel@ffmpeg.org> > > Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the > > support of variable dimension encoding > > > > > From: "Anton Khirnov" <anton@khirnov.net> > > > Sent Time: 2020-06-08 18:50:43 (Monday) > > > To: "FFmpeg development discussions and patches" <ffmpeg- > > devel@ffmpeg.org> > > > Cc: > > > Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the > > support of variable dimension encoding > > > > > > Quoting Linjie Fu (2020-06-08 10:58:03) > > > > And declare AV_CODEC_CAP_VARIABLE_DIMENSIONS for rawvideo > and > > > > wrapped_avframe. > > > > > > > > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > > > > --- > > > > doc/APIchanges | 3 +++ > > > > fftools/cmdutils.c | 2 ++ > > > > libavcodec/avcodec.h | 4 +++- > > > > libavcodec/codec.h | 5 +++++ > > > > libavcodec/rawenc.c | 1 + > > > > libavcodec/version.h | 2 +- > > > > libavcodec/wrapped_avframe.c | 1 + > > > > 7 files changed, 16 insertions(+), 2 deletions(-) > > > > > > During the last iteration, I asked how is this preferable to just making > > > a new encoder instance. Don't think I got a sufficient reply. > > > > Thanks Anton for the remind, indeed making a new encoder instance > would > > be > > more general and suitable for all encoders, with resolution changing in key > > frames. > > > > In this patch set, I prefer to restrict the implementation/support to > > rawvideo and wrapped_avframe encoders, since they don't need to be > > recreated > > when resolution/dimension changes and are required. at this moment. > > > > Also as we've discussed about whether it's worthwhile: > > > > >> Do we get sufficient benefits from having parameter change capability > > inside > > >> encoders over just creating a new encoder instance when needed? Do > > people > > >> really need to change resolution mid-GOP? > > > > This implementation didn't touch the logic of encoders which supports > > variable > > resolution encoding on inter frames(vp9, av1), since such > > enhancement/requirement > > seems to be rare for now. > > Sent the encoder instance recreate patch set for review as a follow-up step, > please > help to review: > https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1450 Will apply this patch set (1-3) as discussed in: https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2020-June/264649.html Thx. - Linjie
Jun 18, 2020, 04:02 by linjie.fu@intel.com: >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Fu, >> Linjie >> Sent: Tuesday, June 9, 2020 17:02 >> To: FFmpeg development discussions and patches <ffmpeg- >> devel@ffmpeg.org> >> Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the >> support of variable dimension encoding >> >> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of >> > Linjie Fu >> > Sent: Monday, June 8, 2020 23:11 >> > To: FFmpeg development discussions and patches <ffmpeg- >> > devel@ffmpeg.org> >> > Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the >> > support of variable dimension encoding >> > >> > > From: "Anton Khirnov" <anton@khirnov.net> >> > > Sent Time: 2020-06-08 18:50:43 (Monday) >> > > To: "FFmpeg development discussions and patches" <ffmpeg- >> > devel@ffmpeg.org> >> > > Cc: >> > > Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the >> > support of variable dimension encoding >> > > >> > > Quoting Linjie Fu (2020-06-08 10:58:03) >> > > > And declare AV_CODEC_CAP_VARIABLE_DIMENSIONS for rawvideo >> and >> > > > wrapped_avframe. >> > > > >> > > > Signed-off-by: Linjie Fu <linjie.fu@intel.com> >> > > > --- >> > > > doc/APIchanges | 3 +++ >> > > > fftools/cmdutils.c | 2 ++ >> > > > libavcodec/avcodec.h | 4 +++- >> > > > libavcodec/codec.h | 5 +++++ >> > > > libavcodec/rawenc.c | 1 + >> > > > libavcodec/version.h | 2 +- >> > > > libavcodec/wrapped_avframe.c | 1 + >> > > > 7 files changed, 16 insertions(+), 2 deletions(-) >> > > >> > > During the last iteration, I asked how is this preferable to just making >> > > a new encoder instance. Don't think I got a sufficient reply. >> > >> > Thanks Anton for the remind, indeed making a new encoder instance >> would >> > be >> > more general and suitable for all encoders, with resolution changing in key >> > frames. >> > >> > In this patch set, I prefer to restrict the implementation/support to >> > rawvideo and wrapped_avframe encoders, since they don't need to be >> > recreated >> > when resolution/dimension changes and are required. at this moment. >> > >> > Also as we've discussed about whether it's worthwhile: >> > >> > >> Do we get sufficient benefits from having parameter change capability >> > inside >> > >> encoders over just creating a new encoder instance when needed? Do >> > people >> > >> really need to change resolution mid-GOP? >> > >> > This implementation didn't touch the logic of encoders which supports >> > variable >> > resolution encoding on inter frames(vp9, av1), since such >> > enhancement/requirement >> > seems to be rare for now. >> >> Sent the encoder instance recreate patch set for review as a follow-up step, >> please >> help to review: >> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1450 >> > > Will apply this patch set (1-3) as discussed in: > https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2020-June/264649.html > You said you'd only apply the first patch, not the entire patchset in your previous email. I'd like to wait to have a well engineered solution we can agree on instead of rushing this in, I too have an interest in variable frame size encoding.
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Lynne > Sent: Thursday, June 18, 2020 16:04 > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the > support of variable dimension encoding > > Jun 18, 2020, 04:02 by linjie.fu@intel.com: > > >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Fu, > >> Linjie > >> Sent: Tuesday, June 9, 2020 17:02 > >> To: FFmpeg development discussions and patches <ffmpeg- > >> devel@ffmpeg.org> > >> Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the > >> support of variable dimension encoding > >> > >> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf > Of > >> > Linjie Fu > >> > Sent: Monday, June 8, 2020 23:11 > >> > To: FFmpeg development discussions and patches <ffmpeg- > >> > devel@ffmpeg.org> > >> > Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the > >> > support of variable dimension encoding > >> > > >> > > From: "Anton Khirnov" <anton@khirnov.net> > >> > > Sent Time: 2020-06-08 18:50:43 (Monday) > >> > > To: "FFmpeg development discussions and patches" <ffmpeg- > >> > devel@ffmpeg.org> > >> > > Cc: > >> > > Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for > the > >> > support of variable dimension encoding > >> > > > >> > > Quoting Linjie Fu (2020-06-08 10:58:03) > >> > > > And declare AV_CODEC_CAP_VARIABLE_DIMENSIONS for rawvideo > >> and > >> > > > wrapped_avframe. > >> > > > > >> > > > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > >> > > > --- > >> > > > doc/APIchanges | 3 +++ > >> > > > fftools/cmdutils.c | 2 ++ > >> > > > libavcodec/avcodec.h | 4 +++- > >> > > > libavcodec/codec.h | 5 +++++ > >> > > > libavcodec/rawenc.c | 1 + > >> > > > libavcodec/version.h | 2 +- > >> > > > libavcodec/wrapped_avframe.c | 1 + > >> > > > 7 files changed, 16 insertions(+), 2 deletions(-) > >> > > > >> > > During the last iteration, I asked how is this preferable to just making > >> > > a new encoder instance. Don't think I got a sufficient reply. > >> > > >> > Thanks Anton for the remind, indeed making a new encoder instance > >> would > >> > be > >> > more general and suitable for all encoders, with resolution changing in > key > >> > frames. > >> > > >> > In this patch set, I prefer to restrict the implementation/support to > >> > rawvideo and wrapped_avframe encoders, since they don't need to be > >> > recreated > >> > when resolution/dimension changes and are required. at this moment. > >> > > >> > Also as we've discussed about whether it's worthwhile: > >> > > >> > >> Do we get sufficient benefits from having parameter change > capability > >> > inside > >> > >> encoders over just creating a new encoder instance when needed? > Do > >> > people > >> > >> really need to change resolution mid-GOP? > >> > > >> > This implementation didn't touch the logic of encoders which supports > >> > variable > >> > resolution encoding on inter frames(vp9, av1), since such > >> > enhancement/requirement > >> > seems to be rare for now. > >> > >> Sent the encoder instance recreate patch set for review as a follow-up > step, > >> please > >> help to review: > >> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1450 > >> > > > > Will apply this patch set (1-3) as discussed in: > > https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2020-June/264649.html > > > > You said you'd only apply the first patch, not the entire patchset in your > previous email. > I'd like to wait to have a well engineered solution we can agree on instead of > rushing this in, I too have an interest in variable frame size encoding. Sure and totally agree, will only apply auto scale patch. Thanks for the input. - Linjie
diff --git a/doc/APIchanges b/doc/APIchanges index c7e4ce3..f4d350c 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,9 @@ libavutil: 2017-10-21 API changes, most recent first: +2020-06-xx - xxxxxxxxxx - lavc 58.90.101 - codec.h + Add AV_CODEC_CAP_VARIABLE_DIMENSIONS. + 2020-06-xx - xxxxxxxxxx - lavu 56.50.100 - buffer.h Passing NULL as alloc argument to av_buffer_pool_init2() is now allowed. diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index 13567a7..e0469cd 100644 --- a/fftools/cmdutils.c +++ b/fftools/cmdutils.c @@ -1424,6 +1424,8 @@ static void print_codec(const AVCodec *c) printf("hardware "); if (c->capabilities & AV_CODEC_CAP_HYBRID) printf("hybrid "); + if (c->capabilities & AV_CODEC_CAP_VARIABLE_DIMENSIONS) + printf("multidimension "); if (!c->capabilities) printf("none"); printf("\n"); diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index c91b2fd..b435ce6 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -97,7 +97,9 @@ * - For decoding, call avcodec_send_packet() to give the decoder raw * compressed data in an AVPacket. * - For encoding, call avcodec_send_frame() to give the encoder an AVFrame - * containing uncompressed audio or video. + * containing uncompressed audio or video. Video encoder requires input + * frames to be in constant dimensions unless it declare the capability + * of AV_CODEC_CAP_VARIABLE_DIMENSIONS. * * In both cases, it is recommended that AVPackets and AVFrames are * refcounted, or libavcodec might have to copy the input data. (libavformat diff --git a/libavcodec/codec.h b/libavcodec/codec.h index 1fda619..9855477 100644 --- a/libavcodec/codec.h +++ b/libavcodec/codec.h @@ -169,6 +169,11 @@ * remain pending. */ #define AV_CODEC_CAP_ENCODER_FLUSH (1 << 21) +/** + * Codec supports variable dimensions encoding. This indicates that input frames are + * allowed to be in variable dimensions/resolutions, otherwise they have to keep constant. + */ +#define AV_CODEC_CAP_VARIABLE_DIMENSIONS (1 << 22) /** * AVProfile. diff --git a/libavcodec/rawenc.c b/libavcodec/rawenc.c index d181b74..486c0d7 100644 --- a/libavcodec/rawenc.c +++ b/libavcodec/rawenc.c @@ -92,4 +92,5 @@ AVCodec ff_rawvideo_encoder = { .id = AV_CODEC_ID_RAWVIDEO, .init = raw_encode_init, .encode2 = raw_encode, + .capabilities = AV_CODEC_CAP_VARIABLE_DIMENSIONS, }; diff --git a/libavcodec/version.h b/libavcodec/version.h index 524fbc3..4e2cc5d 100644 --- a/libavcodec/version.h +++ b/libavcodec/version.h @@ -29,7 +29,7 @@ #define LIBAVCODEC_VERSION_MAJOR 58 #define LIBAVCODEC_VERSION_MINOR 90 -#define LIBAVCODEC_VERSION_MICRO 100 +#define LIBAVCODEC_VERSION_MICRO 101 #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \ LIBAVCODEC_VERSION_MINOR, \ diff --git a/libavcodec/wrapped_avframe.c b/libavcodec/wrapped_avframe.c index 85ff32d..ae29328 100644 --- a/libavcodec/wrapped_avframe.c +++ b/libavcodec/wrapped_avframe.c @@ -116,6 +116,7 @@ AVCodec ff_wrapped_avframe_encoder = { .id = AV_CODEC_ID_WRAPPED_AVFRAME, .encode2 = wrapped_avframe_encode, .caps_internal = FF_CODEC_CAP_INIT_THREADSAFE, + .capabilities = AV_CODEC_CAP_VARIABLE_DIMENSIONS, }; AVCodec ff_wrapped_avframe_decoder = {
And declare AV_CODEC_CAP_VARIABLE_DIMENSIONS for rawvideo and wrapped_avframe. Signed-off-by: Linjie Fu <linjie.fu@intel.com> --- doc/APIchanges | 3 +++ fftools/cmdutils.c | 2 ++ libavcodec/avcodec.h | 4 +++- libavcodec/codec.h | 5 +++++ libavcodec/rawenc.c | 1 + libavcodec/version.h | 2 +- libavcodec/wrapped_avframe.c | 1 + 7 files changed, 16 insertions(+), 2 deletions(-)