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

[Andreas Rheinhardt]

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