[FFmpeg-devel,3/5] avcodec: add max_samples

Submitted by Michael Niedermayer on Sept. 3, 2019, 12:14 a.m.

Details

Message ID 20190903001426.8957-3-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Sept. 3, 2019, 12:14 a.m.
TODO: APIChanges, bump version

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/avcodec.h                 | 8 ++++++++
 libavcodec/options_table.h           | 1 +
 tests/ref/fate/api-mjpeg-codec-param | 2 ++
 tests/ref/fate/api-png-codec-param   | 2 ++
 4 files changed, 13 insertions(+)

Comments

Paul B Mahol Sept. 3, 2019, 7:20 a.m.
NAK, you can use max_pixels for the same cause.

On 9/3/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> TODO: APIChanges, bump version
>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/avcodec.h                 | 8 ++++++++
>  libavcodec/options_table.h           | 1 +
>  tests/ref/fate/api-mjpeg-codec-param | 2 ++
>  tests/ref/fate/api-png-codec-param   | 2 ++
>  4 files changed, 13 insertions(+)
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index de4feb6a65..4b771a6ae6 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -3371,6 +3371,14 @@ typedef struct AVCodecContext {
>       * - encoding: unused
>       */
>      int discard_damaged_percentage;
> +
> +    /**
> +     * The number of samples per frame to maximally accept.
> +     *
> +     * - decoding: set by user
> +     * - encoding: set by user
> +     */
> +    int64_t max_samples;
>  } AVCodecContext;
>
>  #if FF_API_CODEC_GET_SET
> diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
> index c3a500a629..b56e653bf6 100644
> --- a/libavcodec/options_table.h
> +++ b/libavcodec/options_table.h
> @@ -476,6 +476,7 @@ static const AVOption avcodec_options[] = {
>  {"pixel_format", "set pixel format", OFFSET(pix_fmt),
> AV_OPT_TYPE_PIXEL_FMT, {.i64=AV_PIX_FMT_NONE}, -1, INT_MAX, 0 },
>  {"video_size", "set video size", OFFSET(width), AV_OPT_TYPE_IMAGE_SIZE,
> {.str=NULL}, 0, INT_MAX, 0 },
>  {"max_pixels", "Maximum number of pixels", OFFSET(max_pixels),
> AV_OPT_TYPE_INT64, {.i64 = INT_MAX }, 0, INT_MAX, A|V|S|D|E },
> +{"max_samples", "Maximum number of samples", OFFSET(max_samples),
> AV_OPT_TYPE_INT64, {.i64 = INT_MAX }, 0, INT_MAX, A|D|E },
>  {"hwaccel_flags", NULL, OFFSET(hwaccel_flags), AV_OPT_TYPE_FLAGS, {.i64 =
> AV_HWACCEL_FLAG_IGNORE_LEVEL }, 0, UINT_MAX, V|D, "hwaccel_flags"},
>  {"ignore_level", "ignore level even if the codec level used is unknown or
> higher than the maximum supported level reported by the hardware driver", 0,
> AV_OPT_TYPE_CONST, { .i64 = AV_HWACCEL_FLAG_IGNORE_LEVEL }, INT_MIN,
> INT_MAX, V | D, "hwaccel_flags" },
>  {"allow_high_depth", "allow to output YUV pixel formats with a different
> chroma sampling than 4:2:0 and/or other than 8 bits per component", 0,
> AV_OPT_TYPE_CONST, {.i64 = AV_HWACCEL_FLAG_ALLOW_HIGH_DEPTH }, INT_MIN,
> INT_MAX, V | D, "hwaccel_flags"},
> diff --git a/tests/ref/fate/api-mjpeg-codec-param
> b/tests/ref/fate/api-mjpeg-codec-param
> index 0815919d7d..e55cef0eb9 100644
> --- a/tests/ref/fate/api-mjpeg-codec-param
> +++ b/tests/ref/fate/api-mjpeg-codec-param
> @@ -136,6 +136,7 @@ stream=0, decode=0
>      pixel_format=yuvj422p
>      video_size=400x225
>      max_pixels=2147483647
> +    max_samples=2147483647
>      hwaccel_flags=0x00000001
>      extra_hw_frames=-1
>      discard_damaged_percentage=95
> @@ -277,6 +278,7 @@ stream=0, decode=1
>      pixel_format=yuvj422p
>      video_size=400x225
>      max_pixels=2147483647
> +    max_samples=2147483647
>      hwaccel_flags=0x00000001
>      extra_hw_frames=-1
>      discard_damaged_percentage=95
> diff --git a/tests/ref/fate/api-png-codec-param
> b/tests/ref/fate/api-png-codec-param
> index a47d0963da..c04c8cc7c1 100644
> --- a/tests/ref/fate/api-png-codec-param
> +++ b/tests/ref/fate/api-png-codec-param
> @@ -136,6 +136,7 @@ stream=0, decode=0
>      pixel_format=rgba
>      video_size=128x128
>      max_pixels=2147483647
> +    max_samples=2147483647
>      hwaccel_flags=0x00000001
>      extra_hw_frames=-1
>      discard_damaged_percentage=95
> @@ -277,6 +278,7 @@ stream=0, decode=1
>      pixel_format=rgba
>      video_size=128x128
>      max_pixels=2147483647
> +    max_samples=2147483647
>      hwaccel_flags=0x00000001
>      extra_hw_frames=-1
>      discard_damaged_percentage=95
> --
> 2.23.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Tomas Härdin Sept. 3, 2019, 10:21 a.m.
tis 2019-09-03 klockan 09:20 +0200 skrev Paul B Mahol:
> 
> On 9/3/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > TODO: APIChanges, bump version
> > 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/avcodec.h                 | 8 ++++++++
> >  libavcodec/options_table.h           | 1 +
> >  tests/ref/fate/api-mjpeg-codec-param | 2 ++
> >  tests/ref/fate/api-png-codec-param   | 2 ++
> >  4 files changed, 13 insertions(+)
> > 
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index de4feb6a65..4b771a6ae6 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -3371,6 +3371,14 @@ typedef struct AVCodecContext {
> >       * - encoding: unused
> >       */
> >      int discard_damaged_percentage;
> > +
> > +    /**
> > +     * The number of samples per frame to maximally accept.
> > +     *
> > +     * - decoding: set by user
> > +     * - encoding: set by user
> > +     */
> > +    int64_t max_samples;
> >  } AVCodecContext;
> 
> NAK, you can use max_pixels for the same cause.

Or the other way around. Has max_pixels made it to an official release
yet? Using it for samples is obviously confusing

Hot take: AVCodec is way overdue for a split into ACodec and VCodec
(and SCodec and so on), with AVCodec being a typed union

/Tomas
Michael Niedermayer Sept. 3, 2019, 10:57 a.m.
On Tue, Sep 03, 2019 at 09:20:52AM +0200, Paul B Mahol wrote:
> NAK, you can use max_pixels for the same cause.

yes, we can, its poor design though audio samples are not
pixels.

Anyone else has an opinion on this ?

Thanks

[...]
Michael Niedermayer Sept. 3, 2019, 11:10 a.m.
On Tue, Sep 03, 2019 at 12:21:26PM +0200, Tomas Härdin wrote:
> tis 2019-09-03 klockan 09:20 +0200 skrev Paul B Mahol:
> > 
> > On 9/3/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > > TODO: APIChanges, bump version
> > > 
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavcodec/avcodec.h                 | 8 ++++++++
> > >  libavcodec/options_table.h           | 1 +
> > >  tests/ref/fate/api-mjpeg-codec-param | 2 ++
> > >  tests/ref/fate/api-png-codec-param   | 2 ++
> > >  4 files changed, 13 insertions(+)
> > > 
> > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > index de4feb6a65..4b771a6ae6 100644
> > > --- a/libavcodec/avcodec.h
> > > +++ b/libavcodec/avcodec.h
> > > @@ -3371,6 +3371,14 @@ typedef struct AVCodecContext {
> > >       * - encoding: unused
> > >       */
> > >      int discard_damaged_percentage;
> > > +
> > > +    /**
> > > +     * The number of samples per frame to maximally accept.
> > > +     *
> > > +     * - decoding: set by user
> > > +     * - encoding: set by user
> > > +     */
> > > +    int64_t max_samples;
> > >  } AVCodecContext;
> > 
> > NAK, you can use max_pixels for the same cause.
> 
> Or the other way around. Has max_pixels made it to an official release
> yet? 

max_pixels is in AVCodecContext since 3.3

but even if it wasnt. while no question max_samples would be a much
better name for a unified field than max_pixels.

We do not unify other related fields like
width & height with for example nb_samples and channels

There is also a semantic difference between max_pixels and max_samples

the max_pixels are the samples at one instance of time
while max_samples cover a period of time depending on the duration of a packet

if ever a video codec had packets covering a period of time the same way
audio does (like maybe 0.5 seconds or 15 frames) we would not apply
max_pixels to the whole but to the 15 frames individually

So IMHO its possible to use the same field for both but it feels a
bit like bending 2 slightly different things onto each other.

But sure if people prefer it ill change the patches.


> Using it for samples is obviously confusing

yes

thanks


[...]
Tomas Härdin Sept. 3, 2019, 5:01 p.m.
tis 2019-09-03 klockan 13:10 +0200 skrev Michael Niedermayer:
> On Tue, Sep 03, 2019 at 12:21:26PM +0200, Tomas Härdin wrote:
> > tis 2019-09-03 klockan 09:20 +0200 skrev Paul B Mahol:
> > > On 9/3/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > > > TODO: APIChanges, bump version
> > > > 
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >  libavcodec/avcodec.h                 | 8 ++++++++
> > > >  libavcodec/options_table.h           | 1 +
> > > >  tests/ref/fate/api-mjpeg-codec-param | 2 ++
> > > >  tests/ref/fate/api-png-codec-param   | 2 ++
> > > >  4 files changed, 13 insertions(+)
> > > > 
> > > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > > index de4feb6a65..4b771a6ae6 100644
> > > > --- a/libavcodec/avcodec.h
> > > > +++ b/libavcodec/avcodec.h
> > > > @@ -3371,6 +3371,14 @@ typedef struct AVCodecContext {
> > > >       * - encoding: unused
> > > >       */
> > > >      int discard_damaged_percentage;
> > > > +
> > > > +    /**
> > > > +     * The number of samples per frame to maximally accept.
> > > > +     *
> > > > +     * - decoding: set by user
> > > > +     * - encoding: set by user
> > > > +     */
> > > > +    int64_t max_samples;
> > > >  } AVCodecContext;
> > > 
> > > NAK, you can use max_pixels for the same cause.
> > 
> > Or the other way around. Has max_pixels made it to an official release
> > yet? 
> 
> max_pixels is in AVCodecContext since 3.3
> 
> but even if it wasnt. while no question max_samples would be a much
> better name for a unified field than max_pixels.
> 
> We do not unify other related fields like
> width & height with for example nb_samples and channels
> 
> There is also a semantic difference between max_pixels and max_samples
> 
> the max_pixels are the samples at one instance of time
> while max_samples cover a period of time depending on the duration of a packet

This is a good point. max_pixels as applied to audio would then mean
maximum number of channels.

I still think putting audio and video stuff in the same struct is a bit
of a mistake, for just this reason

/Tomas
James Almer Sept. 3, 2019, 5:35 p.m.
On 9/3/2019 7:57 AM, Michael Niedermayer wrote:
> On Tue, Sep 03, 2019 at 09:20:52AM +0200, Paul B Mahol wrote:
>> NAK, you can use max_pixels for the same cause.
> 
> yes, we can, its poor design though audio samples are not
> pixels.
> 
> Anyone else has an opinion on this ?
> 
> Thanks

I prefer a separate field. If the existing field was called in a more
ambiguous way I'd be ok with reusing it, but not like this.
Nicolas George Sept. 3, 2019, 5:52 p.m.
James Almer (12019-09-03):
> I prefer a separate field. If the existing field was called in a more
> ambiguous way I'd be ok with reusing it, but not like this.

Same for me.

Regards,

Patch hide | download patch | download mbox

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index de4feb6a65..4b771a6ae6 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -3371,6 +3371,14 @@  typedef struct AVCodecContext {
      * - encoding: unused
      */
     int discard_damaged_percentage;
+
+    /**
+     * The number of samples per frame to maximally accept.
+     *
+     * - decoding: set by user
+     * - encoding: set by user
+     */
+    int64_t max_samples;
 } AVCodecContext;
 
 #if FF_API_CODEC_GET_SET
diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
index c3a500a629..b56e653bf6 100644
--- a/libavcodec/options_table.h
+++ b/libavcodec/options_table.h
@@ -476,6 +476,7 @@  static const AVOption avcodec_options[] = {
 {"pixel_format", "set pixel format", OFFSET(pix_fmt), AV_OPT_TYPE_PIXEL_FMT, {.i64=AV_PIX_FMT_NONE}, -1, INT_MAX, 0 },
 {"video_size", "set video size", OFFSET(width), AV_OPT_TYPE_IMAGE_SIZE, {.str=NULL}, 0, INT_MAX, 0 },
 {"max_pixels", "Maximum number of pixels", OFFSET(max_pixels), AV_OPT_TYPE_INT64, {.i64 = INT_MAX }, 0, INT_MAX, A|V|S|D|E },
+{"max_samples", "Maximum number of samples", OFFSET(max_samples), AV_OPT_TYPE_INT64, {.i64 = INT_MAX }, 0, INT_MAX, A|D|E },
 {"hwaccel_flags", NULL, OFFSET(hwaccel_flags), AV_OPT_TYPE_FLAGS, {.i64 = AV_HWACCEL_FLAG_IGNORE_LEVEL }, 0, UINT_MAX, V|D, "hwaccel_flags"},
 {"ignore_level", "ignore level even if the codec level used is unknown or higher than the maximum supported level reported by the hardware driver", 0, AV_OPT_TYPE_CONST, { .i64 = AV_HWACCEL_FLAG_IGNORE_LEVEL }, INT_MIN, INT_MAX, V | D, "hwaccel_flags" },
 {"allow_high_depth", "allow to output YUV pixel formats with a different chroma sampling than 4:2:0 and/or other than 8 bits per component", 0, AV_OPT_TYPE_CONST, {.i64 = AV_HWACCEL_FLAG_ALLOW_HIGH_DEPTH }, INT_MIN, INT_MAX, V | D, "hwaccel_flags"},
diff --git a/tests/ref/fate/api-mjpeg-codec-param b/tests/ref/fate/api-mjpeg-codec-param
index 0815919d7d..e55cef0eb9 100644
--- a/tests/ref/fate/api-mjpeg-codec-param
+++ b/tests/ref/fate/api-mjpeg-codec-param
@@ -136,6 +136,7 @@  stream=0, decode=0
     pixel_format=yuvj422p
     video_size=400x225
     max_pixels=2147483647
+    max_samples=2147483647
     hwaccel_flags=0x00000001
     extra_hw_frames=-1
     discard_damaged_percentage=95
@@ -277,6 +278,7 @@  stream=0, decode=1
     pixel_format=yuvj422p
     video_size=400x225
     max_pixels=2147483647
+    max_samples=2147483647
     hwaccel_flags=0x00000001
     extra_hw_frames=-1
     discard_damaged_percentage=95
diff --git a/tests/ref/fate/api-png-codec-param b/tests/ref/fate/api-png-codec-param
index a47d0963da..c04c8cc7c1 100644
--- a/tests/ref/fate/api-png-codec-param
+++ b/tests/ref/fate/api-png-codec-param
@@ -136,6 +136,7 @@  stream=0, decode=0
     pixel_format=rgba
     video_size=128x128
     max_pixels=2147483647
+    max_samples=2147483647
     hwaccel_flags=0x00000001
     extra_hw_frames=-1
     discard_damaged_percentage=95
@@ -277,6 +278,7 @@  stream=0, decode=1
     pixel_format=rgba
     video_size=128x128
     max_pixels=2147483647
+    max_samples=2147483647
     hwaccel_flags=0x00000001
     extra_hw_frames=-1
     discard_damaged_percentage=95