diff mbox series

[FFmpeg-devel,1/3] lavc/avcodec: Add caps for the support of variable dimension encoding

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
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Fu, Linjie June 8, 2020, 8:58 a.m. UTC
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(-)

Comments

Anton Khirnov June 8, 2020, 10:50 a.m. UTC | #1
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.
Linjie Fu June 8, 2020, 3:10 p.m. UTC | #2
> 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
Fu, Linjie June 9, 2020, 9:02 a.m. UTC | #3
> 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
Nicolas George June 9, 2020, 9:48 a.m. UTC | #4
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,
Anton Khirnov June 12, 2020, 10:15 a.m. UTC | #5
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.
Fu, Linjie June 18, 2020, 3:02 a.m. UTC | #6
> 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
Lynne June 18, 2020, 8:03 a.m. UTC | #7
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.
Fu, Linjie June 18, 2020, 8:26 a.m. UTC | #8
> 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 mbox series

Patch

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 = {