diff mbox series

[FFmpeg-devel,v2,1/2] avcodec/noise: don't force non-zero value for amount

Message ID 20210723103450.11265-1-ffmpeg@gyani.pro
State New
Headers show
Series [FFmpeg-devel,v2,1/2] avcodec/noise: don't force non-zero value for amount | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Gyan Doshi July 23, 2021, 10:34 a.m. UTC 
Currently, the user is forced to accept some noise in
packet payload, even if not requested.
---
 doc/bitstream_filters.texi                    | 13 +++++-------
 libavcodec/noise_bsf.c                        | 21 +++++++++++++------
 tests/fate/matroska.mak                       |  2 +-
 .../fate/matroska-mastering-display-metadata  | 10 ++++-----
 4 files changed, 26 insertions(+), 20 deletions(-)

Comments

Michael Niedermayer July 23, 2021, 7:44 p.m. UTC | #1 
On Fri, Jul 23, 2021 at 04:04:49PM +0530, Gyan Doshi wrote:
> Currently, the user is forced to accept some noise in
> packet payload, even if not requested.
> ---
>  doc/bitstream_filters.texi                    | 13 +++++-------
>  libavcodec/noise_bsf.c                        | 21 +++++++++++++------
>  tests/fate/matroska.mak                       |  2 +-
>  .../fate/matroska-mastering-display-metadata  | 10 ++++-----
>  4 files changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
> index d10842ae47..2b84bda1fc 100644
> --- a/doc/bitstream_filters.texi
> +++ b/doc/bitstream_filters.texi
> @@ -534,20 +534,17 @@ container. Can be used for fuzzing or testing error resilience/concealment.
>  Parameters:
>  @table @option
>  @item amount
> -A numeral string, whose value is related to how often output bytes will
> -be modified. Therefore, values below or equal to 0 are forbidden, and
> -the lower the more frequent bytes will be modified, with 1 meaning
> -every byte is modified.
> +Accepts a positive integer. Lower the value, more frequently bytes will be modified,
> +with @var{1} meaning every byte is modified. Default is @var{0}.
>  @item dropamount
> -A numeral string, whose value is related to how often packets will be dropped.
> -Therefore, values below or equal to 0 are forbidden, and the lower the more
> -frequent packets will be dropped, with 1 meaning every packet is dropped.
> +Accepts a positive integer. Lower the value, more frequently packets will be dropped,
> +with @var{1} meaning every packet is dropped. Default is @var{0}.
>  @end table
>  
>  The following example applies the modification to every byte but does not drop
>  any packets.
>  @example
> -ffmpeg -i INPUT -c copy -bsf noise[=1] output.mkv
> +ffmpeg -i INPUT -c copy -bsf noise=amount=1 output.mkv
>  @end example
>  
>  @section null
> diff --git a/libavcodec/noise_bsf.c b/libavcodec/noise_bsf.c
> index 6ebd369633..c1b0203442 100644
> --- a/libavcodec/noise_bsf.c
> +++ b/libavcodec/noise_bsf.c
> @@ -33,20 +33,28 @@ typedef struct NoiseContext {
>      unsigned int state;
>  } NoiseContext;
>  
> -static int noise(AVBSFContext *ctx, AVPacket *pkt)
> +static int noise_init(AVBSFContext *ctx)
>  {
>      NoiseContext *s = ctx->priv_data;
> -    int amount = s->amount > 0 ? s->amount : (s->state % 10001 + 1);
> -    int i, ret;

you are droping the support for variable noise

this also breaks a simple plain "-bsf noise"


[...]
Gyan Doshi July 23, 2021, 7:54 p.m. UTC | #2 
On 2021-07-24 01:14, Michael Niedermayer wrote:
> On Fri, Jul 23, 2021 at 04:04:49PM +0530, Gyan Doshi wrote:
>> Currently, the user is forced to accept some noise in
>> packet payload, even if not requested.
>> ---
>>   doc/bitstream_filters.texi                    | 13 +++++-------
>>   libavcodec/noise_bsf.c                        | 21 +++++++++++++------
>>   tests/fate/matroska.mak                       |  2 +-
>>   .../fate/matroska-mastering-display-metadata  | 10 ++++-----
>>   4 files changed, 26 insertions(+), 20 deletions(-)
>>
>> diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
>> index d10842ae47..2b84bda1fc 100644
>> --- a/doc/bitstream_filters.texi
>> +++ b/doc/bitstream_filters.texi
>> @@ -534,20 +534,17 @@ container. Can be used for fuzzing or testing error resilience/concealment.
>>   Parameters:
>>   @table @option
>>   @item amount
>> -A numeral string, whose value is related to how often output bytes will
>> -be modified. Therefore, values below or equal to 0 are forbidden, and
>> -the lower the more frequent bytes will be modified, with 1 meaning
>> -every byte is modified.
>> +Accepts a positive integer. Lower the value, more frequently bytes will be modified,
>> +with @var{1} meaning every byte is modified. Default is @var{0}.
>>   @item dropamount
>> -A numeral string, whose value is related to how often packets will be dropped.
>> -Therefore, values below or equal to 0 are forbidden, and the lower the more
>> -frequent packets will be dropped, with 1 meaning every packet is dropped.
>> +Accepts a positive integer. Lower the value, more frequently packets will be dropped,
>> +with @var{1} meaning every packet is dropped. Default is @var{0}.
>>   @end table
>>   
>>   The following example applies the modification to every byte but does not drop
>>   any packets.
>>   @example
>> -ffmpeg -i INPUT -c copy -bsf noise[=1] output.mkv
>> +ffmpeg -i INPUT -c copy -bsf noise=amount=1 output.mkv
>>   @end example
>>   
>>   @section null
>> diff --git a/libavcodec/noise_bsf.c b/libavcodec/noise_bsf.c
>> index 6ebd369633..c1b0203442 100644
>> --- a/libavcodec/noise_bsf.c
>> +++ b/libavcodec/noise_bsf.c
>> @@ -33,20 +33,28 @@ typedef struct NoiseContext {
>>       unsigned int state;
>>   } NoiseContext;
>>   
>> -static int noise(AVBSFContext *ctx, AVPacket *pkt)
>> +static int noise_init(AVBSFContext *ctx)
>>   {
>>       NoiseContext *s = ctx->priv_data;
>> -    int amount = s->amount > 0 ? s->amount : (s->state % 10001 + 1);
>> -    int i, ret;
> you are droping the support for variable noise
>
> this also breaks a simple plain "-bsf noise"

Will re-add it. Is making it default required?

Gyan
Michael Niedermayer July 23, 2021, 9:49 p.m. UTC | #3 
On Sat, Jul 24, 2021 at 01:24:54AM +0530, Gyan Doshi wrote:
> 
> 
> On 2021-07-24 01:14, Michael Niedermayer wrote:
> > On Fri, Jul 23, 2021 at 04:04:49PM +0530, Gyan Doshi wrote:
> > > Currently, the user is forced to accept some noise in
> > > packet payload, even if not requested.
> > > ---
> > >   doc/bitstream_filters.texi                    | 13 +++++-------
> > >   libavcodec/noise_bsf.c                        | 21 +++++++++++++------
> > >   tests/fate/matroska.mak                       |  2 +-
> > >   .../fate/matroska-mastering-display-metadata  | 10 ++++-----
> > >   4 files changed, 26 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
> > > index d10842ae47..2b84bda1fc 100644
> > > --- a/doc/bitstream_filters.texi
> > > +++ b/doc/bitstream_filters.texi
> > > @@ -534,20 +534,17 @@ container. Can be used for fuzzing or testing error resilience/concealment.
> > >   Parameters:
> > >   @table @option
> > >   @item amount
> > > -A numeral string, whose value is related to how often output bytes will
> > > -be modified. Therefore, values below or equal to 0 are forbidden, and
> > > -the lower the more frequent bytes will be modified, with 1 meaning
> > > -every byte is modified.
> > > +Accepts a positive integer. Lower the value, more frequently bytes will be modified,
> > > +with @var{1} meaning every byte is modified. Default is @var{0}.
> > >   @item dropamount
> > > -A numeral string, whose value is related to how often packets will be dropped.
> > > -Therefore, values below or equal to 0 are forbidden, and the lower the more
> > > -frequent packets will be dropped, with 1 meaning every packet is dropped.
> > > +Accepts a positive integer. Lower the value, more frequently packets will be dropped,
> > > +with @var{1} meaning every packet is dropped. Default is @var{0}.
> > >   @end table
> > >   The following example applies the modification to every byte but does not drop
> > >   any packets.
> > >   @example
> > > -ffmpeg -i INPUT -c copy -bsf noise[=1] output.mkv
> > > +ffmpeg -i INPUT -c copy -bsf noise=amount=1 output.mkv
> > >   @end example
> > >   @section null
> > > diff --git a/libavcodec/noise_bsf.c b/libavcodec/noise_bsf.c
> > > index 6ebd369633..c1b0203442 100644
> > > --- a/libavcodec/noise_bsf.c
> > > +++ b/libavcodec/noise_bsf.c
> > > @@ -33,20 +33,28 @@ typedef struct NoiseContext {
> > >       unsigned int state;
> > >   } NoiseContext;
> > > -static int noise(AVBSFContext *ctx, AVPacket *pkt)
> > > +static int noise_init(AVBSFContext *ctx)
> > >   {
> > >       NoiseContext *s = ctx->priv_data;
> > > -    int amount = s->amount > 0 ? s->amount : (s->state % 10001 + 1);
> > > -    int i, ret;
> > you are droping the support for variable noise
> > 
> > this also breaks a simple plain "-bsf noise"
> 
> Will re-add it. Is making it default required?

what else would -bsf noise do ?
I think someone using "-bsf noise" might be surprised if what it does changes
Is there a disadvantge in leaving the default ?

thx
[...]
Gyan Doshi July 24, 2021, 4:12 a.m. UTC | #4 
On 2021-07-24 03:19, Michael Niedermayer wrote:
> On Sat, Jul 24, 2021 at 01:24:54AM +0530, Gyan Doshi wrote:
>>
>> On 2021-07-24 01:14, Michael Niedermayer wrote:
>>> On Fri, Jul 23, 2021 at 04:04:49PM +0530, Gyan Doshi wrote:
>>>> Currently, the user is forced to accept some noise in
>>>> packet payload, even if not requested.
>>>> ---
>>>>    doc/bitstream_filters.texi                    | 13 +++++-------
>>>>    libavcodec/noise_bsf.c                        | 21 +++++++++++++------
>>>>    tests/fate/matroska.mak                       |  2 +-
>>>>    .../fate/matroska-mastering-display-metadata  | 10 ++++-----
>>>>    4 files changed, 26 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
>>>> index d10842ae47..2b84bda1fc 100644
>>>> --- a/doc/bitstream_filters.texi
>>>> +++ b/doc/bitstream_filters.texi
>>>> @@ -534,20 +534,17 @@ container. Can be used for fuzzing or testing error resilience/concealment.
>>>>    Parameters:
>>>>    @table @option
>>>>    @item amount
>>>> -A numeral string, whose value is related to how often output bytes will
>>>> -be modified. Therefore, values below or equal to 0 are forbidden, and
>>>> -the lower the more frequent bytes will be modified, with 1 meaning
>>>> -every byte is modified.
>>>> +Accepts a positive integer. Lower the value, more frequently bytes will be modified,
>>>> +with @var{1} meaning every byte is modified. Default is @var{0}.
>>>>    @item dropamount
>>>> -A numeral string, whose value is related to how often packets will be dropped.
>>>> -Therefore, values below or equal to 0 are forbidden, and the lower the more
>>>> -frequent packets will be dropped, with 1 meaning every packet is dropped.
>>>> +Accepts a positive integer. Lower the value, more frequently packets will be dropped,
>>>> +with @var{1} meaning every packet is dropped. Default is @var{0}.
>>>>    @end table
>>>>    The following example applies the modification to every byte but does not drop
>>>>    any packets.
>>>>    @example
>>>> -ffmpeg -i INPUT -c copy -bsf noise[=1] output.mkv
>>>> +ffmpeg -i INPUT -c copy -bsf noise=amount=1 output.mkv
>>>>    @end example
>>>>    @section null
>>>> diff --git a/libavcodec/noise_bsf.c b/libavcodec/noise_bsf.c
>>>> index 6ebd369633..c1b0203442 100644
>>>> --- a/libavcodec/noise_bsf.c
>>>> +++ b/libavcodec/noise_bsf.c
>>>> @@ -33,20 +33,28 @@ typedef struct NoiseContext {
>>>>        unsigned int state;
>>>>    } NoiseContext;
>>>> -static int noise(AVBSFContext *ctx, AVPacket *pkt)
>>>> +static int noise_init(AVBSFContext *ctx)
>>>>    {
>>>>        NoiseContext *s = ctx->priv_data;
>>>> -    int amount = s->amount > 0 ? s->amount : (s->state % 10001 + 1);
>>>> -    int i, ret;
>>> you are droping the support for variable noise
>>>
>>> this also breaks a simple plain "-bsf noise"
>> Will re-add it. Is making it default required?
> what else would -bsf noise do ?

Originally, noise could only alter packet data, so it made sense that 
even with the default of amount=0. the filter shouldn't be a no-op, 
despite the intuitive semantics of amount=0 being no alteration .

Once dropping packets was added, there can't be an expectation that with 
amount=0. the user still wants to alter data. After all, the opt parser 
cannot distinguish between `-bsf noise` and `-bsf 
noise=amount=0[:dropamount=non-zero]`. This is how many other filters 
work - the default behaviour for setpts, scale, select, crop, pad..etc 
is a no-op not some arbitrary adjustment to the payload. That's not good UX.

Regards,
Gyan
Andreas Rheinhardt July 24, 2021, 4:25 a.m. UTC | #5 
Gyan Doshi:
> 
> 
> On 2021-07-24 03:19, Michael Niedermayer wrote:
>> On Sat, Jul 24, 2021 at 01:24:54AM +0530, Gyan Doshi wrote:
>>>
>>> On 2021-07-24 01:14, Michael Niedermayer wrote:
>>>> On Fri, Jul 23, 2021 at 04:04:49PM +0530, Gyan Doshi wrote:
>>>>> Currently, the user is forced to accept some noise in
>>>>> packet payload, even if not requested.
>>>>> ---
>>>>>    doc/bitstream_filters.texi                    | 13 +++++-------
>>>>>    libavcodec/noise_bsf.c                        | 21
>>>>> +++++++++++++------
>>>>>    tests/fate/matroska.mak                       |  2 +-
>>>>>    .../fate/matroska-mastering-display-metadata  | 10 ++++-----
>>>>>    4 files changed, 26 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
>>>>> index d10842ae47..2b84bda1fc 100644
>>>>> --- a/doc/bitstream_filters.texi
>>>>> +++ b/doc/bitstream_filters.texi
>>>>> @@ -534,20 +534,17 @@ container. Can be used for fuzzing or testing
>>>>> error resilience/concealment.
>>>>>    Parameters:
>>>>>    @table @option
>>>>>    @item amount
>>>>> -A numeral string, whose value is related to how often output bytes
>>>>> will
>>>>> -be modified. Therefore, values below or equal to 0 are forbidden, and
>>>>> -the lower the more frequent bytes will be modified, with 1 meaning
>>>>> -every byte is modified.
>>>>> +Accepts a positive integer. Lower the value, more frequently bytes
>>>>> will be modified,
>>>>> +with @var{1} meaning every byte is modified. Default is @var{0}.
>>>>>    @item dropamount
>>>>> -A numeral string, whose value is related to how often packets will
>>>>> be dropped.
>>>>> -Therefore, values below or equal to 0 are forbidden, and the lower
>>>>> the more
>>>>> -frequent packets will be dropped, with 1 meaning every packet is
>>>>> dropped.
>>>>> +Accepts a positive integer. Lower the value, more frequently
>>>>> packets will be dropped,
>>>>> +with @var{1} meaning every packet is dropped. Default is @var{0}.
>>>>>    @end table
>>>>>    The following example applies the modification to every byte but
>>>>> does not drop
>>>>>    any packets.
>>>>>    @example
>>>>> -ffmpeg -i INPUT -c copy -bsf noise[=1] output.mkv
>>>>> +ffmpeg -i INPUT -c copy -bsf noise=amount=1 output.mkv
>>>>>    @end example
>>>>>    @section null
>>>>> diff --git a/libavcodec/noise_bsf.c b/libavcodec/noise_bsf.c
>>>>> index 6ebd369633..c1b0203442 100644
>>>>> --- a/libavcodec/noise_bsf.c
>>>>> +++ b/libavcodec/noise_bsf.c
>>>>> @@ -33,20 +33,28 @@ typedef struct NoiseContext {
>>>>>        unsigned int state;
>>>>>    } NoiseContext;
>>>>> -static int noise(AVBSFContext *ctx, AVPacket *pkt)
>>>>> +static int noise_init(AVBSFContext *ctx)
>>>>>    {
>>>>>        NoiseContext *s = ctx->priv_data;
>>>>> -    int amount = s->amount > 0 ? s->amount : (s->state % 10001 + 1);
>>>>> -    int i, ret;
>>>> you are droping the support for variable noise
>>>>
>>>> this also breaks a simple plain "-bsf noise"
>>> Will re-add it. Is making it default required?
>> what else would -bsf noise do ?
> 
> Originally, noise could only alter packet data, so it made sense that
> even with the default of amount=0. the filter shouldn't be a no-op,
> despite the intuitive semantics of amount=0 being no alteration .
> 
> Once dropping packets was added, there can't be an expectation that with
> amount=0. the user still wants to alter data. After all, the opt parser
> cannot distinguish between `-bsf noise` and `-bsf
> noise=amount=0[:dropamount=non-zero]`. This is how many other filters
> work - the default behaviour for setpts, scale, select, crop, pad..etc
> is a no-op not some arbitrary adjustment to the payload. That's not good
> UX.
> 
Wrong: You can just set the default value of amount to -1 (meaning auto)
which uses the current behaviour for zero. This is not an API change
given that the doc disallowed setting amount to zero. Then one can allow
(and document) setting amount to zero (in the sense that packets that
are not dropped are unchanged).

- Andreas
Gyan Doshi July 24, 2021, 4:43 a.m. UTC | #6 
On 2021-07-24 09:55, Andreas Rheinhardt wrote:
> Gyan Doshi:
>>
>> On 2021-07-24 03:19, Michael Niedermayer wrote:
>>> On Sat, Jul 24, 2021 at 01:24:54AM +0530, Gyan Doshi wrote:
>>>> On 2021-07-24 01:14, Michael Niedermayer wrote:
>>>>> On Fri, Jul 23, 2021 at 04:04:49PM +0530, Gyan Doshi wrote:
>>>>>> Currently, the user is forced to accept some noise in
>>>>>> packet payload, even if not requested.
>>>>>> ---
>>>>>>     doc/bitstream_filters.texi                    | 13 +++++-------
>>>>>>     libavcodec/noise_bsf.c                        | 21
>>>>>> +++++++++++++------
>>>>>>     tests/fate/matroska.mak                       |  2 +-
>>>>>>     .../fate/matroska-mastering-display-metadata  | 10 ++++-----
>>>>>>     4 files changed, 26 insertions(+), 20 deletions(-)
>>>>>>
>>>>>> diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
>>>>>> index d10842ae47..2b84bda1fc 100644
>>>>>> --- a/doc/bitstream_filters.texi
>>>>>> +++ b/doc/bitstream_filters.texi
>>>>>> @@ -534,20 +534,17 @@ container. Can be used for fuzzing or testing
>>>>>> error resilience/concealment.
>>>>>>     Parameters:
>>>>>>     @table @option
>>>>>>     @item amount
>>>>>> -A numeral string, whose value is related to how often output bytes
>>>>>> will
>>>>>> -be modified. Therefore, values below or equal to 0 are forbidden, and
>>>>>> -the lower the more frequent bytes will be modified, with 1 meaning
>>>>>> -every byte is modified.
>>>>>> +Accepts a positive integer. Lower the value, more frequently bytes
>>>>>> will be modified,
>>>>>> +with @var{1} meaning every byte is modified. Default is @var{0}.
>>>>>>     @item dropamount
>>>>>> -A numeral string, whose value is related to how often packets will
>>>>>> be dropped.
>>>>>> -Therefore, values below or equal to 0 are forbidden, and the lower
>>>>>> the more
>>>>>> -frequent packets will be dropped, with 1 meaning every packet is
>>>>>> dropped.
>>>>>> +Accepts a positive integer. Lower the value, more frequently
>>>>>> packets will be dropped,
>>>>>> +with @var{1} meaning every packet is dropped. Default is @var{0}.
>>>>>>     @end table
>>>>>>     The following example applies the modification to every byte but
>>>>>> does not drop
>>>>>>     any packets.
>>>>>>     @example
>>>>>> -ffmpeg -i INPUT -c copy -bsf noise[=1] output.mkv
>>>>>> +ffmpeg -i INPUT -c copy -bsf noise=amount=1 output.mkv
>>>>>>     @end example
>>>>>>     @section null
>>>>>> diff --git a/libavcodec/noise_bsf.c b/libavcodec/noise_bsf.c
>>>>>> index 6ebd369633..c1b0203442 100644
>>>>>> --- a/libavcodec/noise_bsf.c
>>>>>> +++ b/libavcodec/noise_bsf.c
>>>>>> @@ -33,20 +33,28 @@ typedef struct NoiseContext {
>>>>>>         unsigned int state;
>>>>>>     } NoiseContext;
>>>>>> -static int noise(AVBSFContext *ctx, AVPacket *pkt)
>>>>>> +static int noise_init(AVBSFContext *ctx)
>>>>>>     {
>>>>>>         NoiseContext *s = ctx->priv_data;
>>>>>> -    int amount = s->amount > 0 ? s->amount : (s->state % 10001 + 1);
>>>>>> -    int i, ret;
>>>>> you are droping the support for variable noise
>>>>>
>>>>> this also breaks a simple plain "-bsf noise"
>>>> Will re-add it. Is making it default required?
>>> what else would -bsf noise do ?
>> Originally, noise could only alter packet data, so it made sense that
>> even with the default of amount=0. the filter shouldn't be a no-op,
>> despite the intuitive semantics of amount=0 being no alteration .
>>
>> Once dropping packets was added, there can't be an expectation that with
>> amount=0. the user still wants to alter data. After all, the opt parser
>> cannot distinguish between `-bsf noise` and `-bsf
>> noise=amount=0[:dropamount=non-zero]`. This is how many other filters
>> work - the default behaviour for setpts, scale, select, crop, pad..etc
>> is a no-op not some arbitrary adjustment to the payload. That's not good
>> UX.
>>
> Wrong: You can just set the default value of amount to -1 (meaning auto)
> which uses the current behaviour for zero. This is not an API change
> given that the doc disallowed setting amount to zero. Then one can allow
> (and document) setting amount to zero (in the sense that packets that
> are not dropped are unchanged).
I did intend to add -1 within the current range for amount. But I was 
talking about UX, not whether the current behaviour could be remapped to 
a new implicit default.

Regards,
Gyan
diff mbox series

Patch

diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
index d10842ae47..2b84bda1fc 100644
--- a/doc/bitstream_filters.texi
+++ b/doc/bitstream_filters.texi
@@ -534,20 +534,17 @@  container. Can be used for fuzzing or testing error resilience/concealment.
 Parameters:
 @table @option
 @item amount
-A numeral string, whose value is related to how often output bytes will
-be modified. Therefore, values below or equal to 0 are forbidden, and
-the lower the more frequent bytes will be modified, with 1 meaning
-every byte is modified.
+Accepts a positive integer. Lower the value, more frequently bytes will be modified,
+with @var{1} meaning every byte is modified. Default is @var{0}.
 @item dropamount
-A numeral string, whose value is related to how often packets will be dropped.
-Therefore, values below or equal to 0 are forbidden, and the lower the more
-frequent packets will be dropped, with 1 meaning every packet is dropped.
+Accepts a positive integer. Lower the value, more frequently packets will be dropped,
+with @var{1} meaning every packet is dropped. Default is @var{0}.
 @end table
 
 The following example applies the modification to every byte but does not drop
 any packets.
 @example
-ffmpeg -i INPUT -c copy -bsf noise[=1] output.mkv
+ffmpeg -i INPUT -c copy -bsf noise=amount=1 output.mkv
 @end example
 
 @section null
diff --git a/libavcodec/noise_bsf.c b/libavcodec/noise_bsf.c
index 6ebd369633..c1b0203442 100644
--- a/libavcodec/noise_bsf.c
+++ b/libavcodec/noise_bsf.c
@@ -33,20 +33,28 @@  typedef struct NoiseContext {
     unsigned int state;
 } NoiseContext;
 
-static int noise(AVBSFContext *ctx, AVPacket *pkt)
+static int noise_init(AVBSFContext *ctx)
 {
     NoiseContext *s = ctx->priv_data;
-    int amount = s->amount > 0 ? s->amount : (s->state % 10001 + 1);
-    int i, ret;
 
-    if (amount <= 0)
+    if (!s->amount && !s->dropamount) {
+        av_log(ctx, AV_LOG_ERROR, "At least one of amount and dropamount must be set.\n");
         return AVERROR(EINVAL);
+    }
+
+    return 0;
+}
+
+static int noise(AVBSFContext *ctx, AVPacket *pkt)
+{
+    NoiseContext *s = ctx->priv_data;
+    int i, ret;
 
     ret = ff_bsf_get_packet_ref(ctx, pkt);
     if (ret < 0)
         return ret;
 
-    if (s->dropamount > 0 && s->state % s->dropamount == 0) {
+    if (s->dropamount && s->state % s->dropamount == 0) {
         s->state++;
         av_packet_unref(pkt);
         return AVERROR(EAGAIN);
@@ -60,7 +68,7 @@  static int noise(AVBSFContext *ctx, AVPacket *pkt)
 
     for (i = 0; i < pkt->size; i++) {
         s->state += pkt->data[i] + 1;
-        if (s->state % amount == 0)
+        if (s->amount && s->state % s->amount == 0)
             pkt->data[i] = s->state;
     }
 
@@ -86,5 +94,6 @@  const AVBitStreamFilter ff_noise_bsf = {
     .name           = "noise",
     .priv_data_size = sizeof(NoiseContext),
     .priv_class     = &noise_class,
+    .init           = noise_init,
     .filter         = noise,
 };
diff --git a/tests/fate/matroska.mak b/tests/fate/matroska.mak
index ca7193a055..8dba371ff4 100644
--- a/tests/fate/matroska.mak
+++ b/tests/fate/matroska.mak
@@ -88,7 +88,7 @@  FATE_MATROSKA_FFMPEG_FFPROBE-$(call ALLYES, FILE_PROTOCOL MXF_DEMUXER        \
                                             MATROSKA_MUXER MATROSKA_DEMUXER  \
                                             FRAMECRC_MUXER PIPE_PROTOCOL)    \
                                += fate-matroska-mastering-display-metadata
-fate-matroska-mastering-display-metadata: CMD = transcode mxf $(TARGET_SAMPLES)/mxf/Meridian-Apple_ProResProxy-HDR10.mxf matroska "-map 0 -map 0:0 -c:v:0 copy -c:v:1 ffv1 -c:a:0 copy -bsf:a:0 noise=amount=3 -filter:a:1 aresample -c:a:1 pcm_s16be -bsf:a:1 noise=dropamount=4" "-map 0 -c copy" "" "-show_entries stream_side_data_list:stream=index,codec_name"
+fate-matroska-mastering-display-metadata: CMD = transcode mxf $(TARGET_SAMPLES)/mxf/Meridian-Apple_ProResProxy-HDR10.mxf matroska "-map 0 -map 0:0 -c:v:0 copy -c:v:1 ffv1 -c:a:0 copy -bsf:a:0 noise=amount=3 -filter:a:1 aresample -c:a:1 pcm_s16be -bsf:a:1 noise=amount=100:dropamount=4" "-map 0 -c copy" "" "-show_entries stream_side_data_list:stream=index,codec_name"
 
 # This test tests remuxing annex B H.264 into Matroska. It also tests writing
 # the correct interlaced flags and overriding the sample aspect ratio, leading
diff --git a/tests/ref/fate/matroska-mastering-display-metadata b/tests/ref/fate/matroska-mastering-display-metadata
index 8f5d7b6a22..bd8d7cb2c9 100644
--- a/tests/ref/fate/matroska-mastering-display-metadata
+++ b/tests/ref/fate/matroska-mastering-display-metadata
@@ -1,4 +1,4 @@ 
-542ababe5c088ab925ee49373d8b8a85 *tests/data/fate/matroska-mastering-display-metadata.matroska
+61c080b57ef81a6e3bc971857a6973dc *tests/data/fate/matroska-mastering-display-metadata.matroska
 1669695 tests/data/fate/matroska-mastering-display-metadata.matroska
 #extradata 0:        4, 0x040901a3
 #extradata 3:      200, 0x506463a8
@@ -29,19 +29,19 @@ 
 3,          0,          0,       16,   274117, 0xc439610f, S=2,        8,       88
 0,         17,         17,       16,    57248, 0xa06cd7b5
 1,         17,         17,       16,     2403, 0xe1a991e5
-2,         17,         17,       16,     1602, 0x5d868171
+2,         17,         17,       16,     1602, 0x98680820
 3,         17,         17,       16,   273691, 0x5a3b88a5, F=0x0
 0,         33,         33,       16,    57200, 0x5623da10
 1,         33,         33,       16,     2400, 0x6650907f
-2,         33,         33,       16,     1600, 0xa90f0044
+2,         33,         33,       16,     1600, 0xd0590820
 3,         33,         33,       16,   272987, 0x48c443e7, F=0x0
 0,         50,         50,       16,    57152, 0x52d89d3f
 1,         50,         50,       16,     2403, 0x43398a08
-2,         50,         50,       16,     1602, 0x3a350084
+2,         50,         50,       16,     1602, 0x18990820
 3,         50,         50,       16,   271465, 0x251b9cbe, F=0x0
 0,         67,         67,       16,    56960, 0x431d5189
 1,         67,         67,       16,     2403, 0x61cd96cb
-2,         67,         67,       16,     1602, 0xd74800c6
+2,         67,         67,       16,     1602, 0x58ca0720
 3,         67,         67,       16,   270800, 0x8fb2e217, F=0x0
 [STREAM]
 index=0